Add fast_type_map, use it authoritatively for local types and as a hint for global types (ABI breaking) #5842
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,21 +205,43 @@ PYBIND11_NOINLINE detail::type_info *get_type_info(PyTypeObject *type) { | |
| return bases.front(); | ||
| } | ||
|
|
||
| inline detail::type_info *get_local_type_info(const std::type_info &tp) { | ||
| const auto &locals = get_local_internals().registered_types_cpp; | ||
| auto it = locals.find(&tp); | ||
| if (it != locals.end()) { | ||
| return it->second; | ||
| } | ||
| return nullptr; | ||
| } | ||
|
|
||
| inline detail::type_info *get_global_type_info(const std::type_info &tp) { | ||
|
There was a problem hiding this comment. Could you please add a comment — somewhere between terse and concise — to explain that this function implements a two-level lookup? Maybe end with "For more details see PR #5842." (I'd add the comment inside the function, at the top.) Ideally make it play nicely with the comment you're adding in internals.h: Mentioning "two-level lookup" there, too, would be nice. |
||
| // This is a two-level lookup. Hopefully we find the type info in | ||
| // registered_types_cpp_fast, but if not we try | ||
| // registered_types_cpp and fill registered_types_cpp_fast for | ||
| // next time. | ||
| return with_internals([&](internals &internals) { | ||
| detail::type_info *type_info = nullptr; | ||
| #if PYBIND11_INTERNALS_VERSION >= 12 | ||
| auto &fast_types = internals.registered_types_cpp_fast; | ||
| #endif | ||
| auto &types = internals.registered_types_cpp; | ||
| #if PYBIND11_INTERNALS_VERSION >= 12 | ||
| auto fast_it = fast_types.find(&tp); | ||
| if (fast_it != fast_types.end()) { | ||
| # ifndef NDEBUG | ||
| auto types_it = types.find(std::type_index(tp)); | ||
| assert(types_it != types.end()); | ||
| assert(types_it->second == fast_it->second); | ||
| # endif | ||
| return fast_it->second; | ||
| } | ||
| #endif // PYBIND11_INTERNALS_VERSION >= 12 | ||
|
|
||
| auto it = types.find(std::type_index(tp)); | ||
| if (it != types.end()) { | ||
| #if PYBIND11_INTERNALS_VERSION >= 12 | ||
| fast_types.emplace(&tp, it->second); | ||
|
There was a problem hiding this comment. How are you going to cause this entry to be cleaned up when the type it points to is destroyed? Currently I think it will just dangle, which won't go well when you try to use the result. nanobind solves this problem by having its equivalent of There was a problem hiding this comment. I'm not sure I follow. Why does the fast map have different cleanup timing requirements than the slow map? We clean up both at the same time. There was a problem hiding this comment. The slow map has one entry per C++ type. The fast map potentially has multiple. You’re cleaning up the fast map entry that was added by the module that defined the binding, but not the ones that may have been added by other DSOs (with different typeinfo pointers for that type) when they looked it up. I left this comment on the location in the code where those secondary entries are being added; I don’t see anywhere where they can be removed. There was a problem hiding this comment.
Naive solution: when a type is deallocated, can't we just loop over the whole map and remove all entries with the same value as the one we want to remove? The nanobind solution seems fine, but the first thing that needs to be done is writing a test that will expose this problem, since we clearly don't have one (or it's not a problem for some reason). There was a problem hiding this comment. not 100% clear on how to write a test that will result in classes being destroyed, but it looks like test_class does it by registering into temporary modules so I'll try that. I think we want something like:
theory is that currently we will see a crash; before this PR it should've worked? There was a problem hiding this comment. Your summary looks good to me, except that Python can't really unload a whole DSO. Instead, you can You can steal this function from the tests for #5800: After the type has been destroyed, you can bind it again with a new Be aware that extension types are immortal on non-CPython VMs (PyPy, GraalPy) and all types are immortal on free-threading CPython 3.13 (3.14+ have proper GC for them), so you'll want to disable the test for those platforms. There was a problem hiding this comment. OK. I will try to get the tests + fix up today. |
||
| #endif | ||
| type_info = it->second; | ||
| } | ||
| return type_info; | ||
|
|
@@ -228,7 +250,7 @@ inline detail::type_info *get_global_type_info(const std::type_index &tp) { | |
|
|
||
| /// Return the type info for a given C++ type; on lookup failure can either throw or return | ||
| /// nullptr. | ||
| PYBIND11_NOINLINE detail::type_info *get_type_info(const std::type_info &tp, | ||
| bool throw_if_missing = false) { | ||
| if (auto *ltype = get_local_type_info(tp)) { | ||
| return ltype; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in the similar logic for registration, you're fetching the local internals even if you don't need them. Possibly a regression due to the history where the fast type map was in local internals even for non-local types?