From b29e01b92e3e25eadea5d9c2c2ecf0c5f2dff528 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 13 Oct 2026 23:35:11 +0000 Subject: [PATCH 1/2] Memoize type names resolved through the AppendTypesSlow trampoline AppendTypesSlow() resolves type names that Cpp::GetScope() cannot look up directly (template specializations spelled as strings, names relative to a parent scope) by declaring a trampoline variable in the interpreter: __cppjit_interop_AppendTypesSlow __cppjit_interop_sN; Each call parses and compiles code, commits a transaction and leaves one more declaration in the AST, so every call costs more than the one before. Nothing cached the outcome, while the same names come back constantly: overload ranking (CPPMethod::GetPriority) resolves every argument type of every candidate, and it runs each time an overload set is not memoized. Memoize successful resolutions by name and parent scope, as GetType() already does for its __typeof__ trampoline. A name that fails may still resolve once more declarations are available, so failures are retried. The integral values of resolved arguments are strings whose ownership passes to the caller, so the cache keeps and hands out copies. Motivation: in ROOT, every task of a distributed RDataFrame converts a Python tuple into a std::pair&& argument. The implicit conversion goes through the pair's converting constructor template, whose overloads are ranked on every call; each ranking declared the trampoline for std::pair several times. The conversion took 26 ms on the first call and 250 ms after 200 calls, and the distributed RDataFrame test suite spent 536 s out of 686 s of worker time building RDataFrame objects (root-project/root#23675). Reproducer: import time import ROOT df = ROOT.RDataFrame(100) node = ROOT.RDF.AsRNode(df) for block in range(4): start = time.perf_counter() for _ in range(50): ROOT.Internal.RDF.ChangeEmptyEntryRange(node, (0, 10)) print(f"{(time.perf_counter() - start) * 1000 / 50:.1f} ms per call") --- src/interop/interop_wrapper.cxx | 36 ++++++++++++++++++++++++++++++++- test/test_regression.py | 20 ++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/interop/interop_wrapper.cxx b/src/interop/interop_wrapper.cxx index d26697c8..62faa9d8 100644 --- a/src/interop/interop_wrapper.cxx +++ b/src/interop/interop_wrapper.cxx @@ -524,6 +524,36 @@ bool interop::AppendTypesSlow(const std::string& name, std::lock_guard Lock(InterOpMutex); + // Every lookup below declares code through the interpreter, which parses, + // commits a transaction and leaves a new declaration in the AST, so each one + // costs more than the last. The same names come back constantly (overload + // ranking resolves every argument type of every candidate on each call that + // is not memoized), so remember what a name resolved to. A name that fails + // may still resolve once more declarations are available, so only successes + // are cached. + using SlowTypesKey = std::pair; + static std::map> s_cache; + const SlowTypesKey cache_key{name, parent.data}; + // the integral values are strings whose ownership passes to the caller + // (which frees them), so the cache keeps and hands out copies of its own + auto copy_of = [](const Cpp::TemplateArgInfo& info) { + return Cpp::TemplateArgInfo(info.m_Type, info.m_IntegralValue + ? strdup(info.m_IntegralValue) + : nullptr); + }; + if (auto cached = s_cache.find(cache_key); cached != s_cache.end()) { + for (const Cpp::TemplateArgInfo& info : cached->second) + types.push_back(copy_of(info)); + return false; + } + const size_t n_types_before = types.size(); + auto remember = [&] { + std::vector resolved; + for (size_t i = n_types_before; i < types.size(); ++i) + resolved.push_back(copy_of(types[i])); + s_cache.emplace(cache_key, std::move(resolved)); + }; + // We might have an entire expression such as int, double. static unsigned long long struct_count = 0; std::string code = @@ -554,7 +584,10 @@ bool interop::AppendTypesSlow(const std::string& name, TCppScope_t instance_class = Cpp::GetScopeFromType(varN); size_t oldSize = types.size(); Cpp::GetClassTemplateInstantiationArgs(instance_class, types); - return oldSize == types.size(); + if (oldSize == types.size()) + return true; + remember(); + return false; } } @@ -593,6 +626,7 @@ bool interop::AppendTypesSlow(const std::string& name, strdup(std::to_string(Cpp::GetEnumConstantValue(scope)).c_str()); types.emplace_back(type.data, integral_value); } + remember(); return false; } diff --git a/test/test_regression.py b/test/test_regression.py index 2a2bda4a..1b3061aa 100644 --- a/test/test_regression.py +++ b/test/test_regression.py @@ -1727,3 +1727,23 @@ def test56_enum_arg_typedefed_underlying_type(self): assert ns.pick(ns.S.A) == 1 assert ns.pick(ns.S.B) == 5 + + def test57_repeated_enum_constant_template_argument(self): + """A template argument spelled as an enum constant resolves repeatedly""" + + import cppjit + + cppjit.cppdef("""\ + namespace RepeatedEnumArg { + enum E { A = 1, B = 2 }; + template struct T1 { static constexpr int v = (int)e; }; + template struct T2 { static constexpr int v = (int)e; }; + }""") + + ns = cppjit.gbl.RepeatedEnumArg + + # the resolved argument is memoized by name; its integral value is a + # string the first instantiation frees, so the second needs its own + assert ns.T1["RepeatedEnumArg::E::A"].v == 1 + assert ns.T2["RepeatedEnumArg::E::A"].v == 1 + assert ns.T2["RepeatedEnumArg::E::B"].v == 2 From 4ee29fe12e854d19b50a452869addec1f5004961 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Tue, 13 Oct 2026 23:35:11 +0000 Subject: [PATCH 2/2] Do not adopt an already known method into a template overload TemplateProxy::Instantiate() caches the instantiated method on the class under the template's name before the call is attempted. When the call then fails, the next call instantiates the same method again, and the cached overload adopted it a second time: the overload set grew by one duplicate per call, and since adopting a method invalidates the sorting, the whole set was ranked anew (CPPMethod::GetPriority on every argument type of every candidate) on every call. This happens systematically for implicit conversions: ConvertImplicit() first tries the argument as a whole, with nested implicit conversions disabled, and only then unpacks a tuple into constructor arguments. For a tuple converted to a std::pair, the first form always fails after instantiating the pair's converting constructor template. Skip the adoption when the overload already holds the method, so the overload set stays sorted and the repeated call only pays for the lookup. Makes CPPMethod::GetMethod() public to compare the method handles. In ROOT, converting a tuple to std::pair&& went from 3 ms rising to 11 ms per call (with the interop lookup cache) to a flat 1 ms per call (root-project/root#23675). The behaviour is inherited from CPyCppyy, where the same call grows from 2 ms to 8 ms per call. --- src/cpyrt/CPPMethod.h | 3 ++- src/cpyrt/TemplateProxy.cxx | 20 +++++++++++++++++--- test/test_regression.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/src/cpyrt/CPPMethod.h b/src/cpyrt/CPPMethod.h index 4fdd792d..74ec31b2 100644 --- a/src/cpyrt/CPPMethod.h +++ b/src/cpyrt/CPPMethod.h @@ -74,6 +74,8 @@ class CPPMethod : public PyCallable { PyObject* Call(CPPInstance*& self, cpyrt_PyArgs_t args, size_t nargsf, PyObject* kwds, CallContext* ctxt = nullptr) override; + interop::TCppMethod_t GetMethod() const { return fMethod; } + protected: virtual bool ProcessArgs(PyCallArgs& args); @@ -83,7 +85,6 @@ class CPPMethod : public PyCallable { CallContext* ctxt = nullptr); PyObject* Execute(void* self, ptrdiff_t offset, CallContext* ctxt = nullptr); - interop::TCppMethod_t GetMethod() { return fMethod; } interop::TCppScope_t GetScope() { return fScope; } Executor* GetExecutor() { return fExecutor; } std::string GetSignatureString(bool show_formalargs = true); diff --git a/src/cpyrt/TemplateProxy.cxx b/src/cpyrt/TemplateProxy.cxx index d826c00f..1a48a224 100644 --- a/src/cpyrt/TemplateProxy.cxx +++ b/src/cpyrt/TemplateProxy.cxx @@ -283,9 +283,23 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname, // Case 3/4: pre-existing method that was either not found b/c the full // templated name was constructed in this call or it failed as overload else if (bIsCppOL) { - // TODO: see above, since the call hasn't happened yet, this overload may - // already exist and fail again. - ((CPPOverload*)pyol)->AdoptMethod(meth); // takes ownership + // The lookup is cached before the call, so when the call then fails + // (e.g. an implicit conversion attempt that is not allowed to nest), the + // next call instantiates the same method again. Adopting it again would + // grow the overload set with duplicates and force the whole set to be + // ranked anew on every call. + bool known = false; + for (PyCallable* pc : ((CPPOverload*)pyol)->fMethodInfo->fMethods) { + auto* existing = dynamic_cast(pc); + if (existing && existing->GetMethod() == cppmeth) { + known = true; + break; + } + } + if (known) + delete meth; + else + ((CPPOverload*)pyol)->AdoptMethod(meth); // takes ownership } // Case 5: must be a template proxy, meaning that current template name is diff --git a/test/test_regression.py b/test/test_regression.py index 1b3061aa..4a24033c 100644 --- a/test/test_regression.py +++ b/test/test_regression.py @@ -1747,3 +1747,31 @@ def test57_repeated_enum_constant_template_argument(self): assert ns.T1["RepeatedEnumArg::E::A"].v == 1 assert ns.T2["RepeatedEnumArg::E::A"].v == 1 assert ns.T2["RepeatedEnumArg::E::B"].v == 2 + + def test58_repeated_implicit_tuple_conversion(self): + """Repeated implicit tuple-to-pair conversion does not grow the overload set""" + + import cppjit + + cppjit.cppdef("""\ + #include + namespace RepeatedImplicitTuple { + int first(std::pair&& p) { return p.first; } + }""") + + ns = cppjit.gbl.RepeatedImplicitTuple + pair = cppjit.gbl.std.pair["int", "int"] + + def n_ctor_overloads(): + # the instantiated converting constructor is cached on the class + # under the constructor's own name + ol = pair.__dict__.get("pair") + return len(ol.__doc__.split("\n")) if ol is not None else 0 + + assert ns.first((1, 2)) == 1 + n = n_ctor_overloads() + assert n > 0 + + for i in range(20): + assert ns.first((i, 2)) == i + assert n_ctor_overloads() == n