Skip to content

[interop] Remember templated names resolved by GetScope #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jmcarcell wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[interop] Remember templated names resolved by GetScope #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/interop/interop_wrapper.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,14 +666,24 @@ interop::TCppScope_t interop::GetUnderlyingScope(TCppScope_t scope) {
interop::TCppScope_t interop::GetScope(const std::string& name,
TCppScope_t parent_scope) {
std::lock_guard<std::recursive_mutex> Lock(InterOpMutex);
// Resolving a templated name below declares a new variable in the
// interpreter (see AppendTypesSlow), so remember the result to not declare
// one, and grow the memory, on every lookup of the same name.
static std::map<std::pair<void*, std::string>, TCppScope_t> template_scopes;

@jmcarcell jmcarcell Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::unordered_map probably not worth it if there aren't ton of templates

@aaronj0 aaronj0 Oct 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't make an assumption on the size of loaded libraries/TU's which can grow exponentially e.g boost, eigen. So that would probably need to be an unordered_map

@jmcarcell jmcarcell Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the cache doesn't grow with the size of the loaded libraries. It only holds templated names that someone actually looked up by string, failed both direct lookups and then resolved through AppendTypesSlow. The thing is that there is no hash function for std::pair, so I either have to implement one or use a nested std::unordered_map.

@aaronj0 aaronj0 Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I meant the number of templates that can come from loaded libraries. Some TU's have lots of templates that can come back for a name. But I missed the fact that this is only the ones that are resolved with AppendTypesSlow. For that the current map is fine.

const bool is_template = name.find('<') != std::string::npos;
const auto key = std::make_pair(parent_scope.data, name);
if (is_template)
if (auto it = template_scopes.find(key); it != template_scopes.end())
return it->second;

if (interop::TCppScope_t scope = Cpp::GetScope(name, parent_scope))
return scope;
if (!parent_scope || parent_scope == Cpp::GetGlobalScope())
if (interop::TCppScope_t scope = Cpp::GetScopeFromCompleteName(name))
return scope;

// FIXME: avoid string parsing here
if (name.find('<') != std::string::npos) {
if (is_template) {
// Templated type; may need instantiation. Resolve the whole type
// expression (e.g. "std::array<float, 3>") and read back its scope.
// Splitting off the argument list and resolving it directly cannot
Expand All @@ -696,8 +706,10 @@ interop::TCppScope_t interop::GetScope(const std::string& name,
// it, so the specialization may still be declared-but-undefined.
// Force its definition: callers expect a complete scope, e.g. to
// walk its base classes.
if (scope)
if (scope) {
Cpp::IsComplete(scope);
template_scopes[key] = scope;
}
return scope;
}
}
Expand Down
20 changes: 20 additions & 0 deletions test/test_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1772,3 +1772,23 @@ def test57_function_pointer_types_from_ast(self):
# member function pointers are not function pointers; they are not
# supported, but must not crash either
ns.get_mf()

def test58_repeated_template_scope_lookup(self):
"""Binding to a templated class by name does not grow the memory"""

from pytest import importorskip

psutil = importorskip("psutil")

import cppjit

v = cppjit.gbl.std.vector[int]()
addr = cppjit.addressof(v)
process = psutil.Process()
cppjit.bind_object(addr, "std::vector<int>")
before = process.memory_info().rss
for _ in range(2000):
assert type(cppjit.bind_object(addr, "std::vector<int>")) is type(v)
# resolving the name used to declare code in the interpreter on every
# call, growing the memory by several kB each time
assert process.memory_info().rss - before < 4 * 1024 * 1024
Loading