-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add fast_type_map, use it authoritatively for local types and as a hint for global types (ABI breaking) #5842
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.
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
Changes from 5 commits
cc0b053
1cd2b55
abf237f
b87e762
c333e85
8651580
3ca29e8
48f831b
08c09ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,8 +39,13 @@ | |
/// further ABI-incompatible changes may be made before the ABI is officially | ||
/// changed to the new version. | ||
#ifndef PYBIND11_INTERNALS_VERSION | ||
// REMINDER for next version bump: remove loader_life_support_tls | ||
# define PYBIND11_INTERNALS_VERSION 11 | ||
# if PY_VERSION_HEX >= 0x030E0000 | ||
// Get test coverage for ABI version 12 without breaking existing | ||
// Python versions. | ||
# define PYBIND11_INTERNALS_VERSION 12 | ||
# else | ||
# define PYBIND11_INTERNALS_VERSION 11 | ||
# endif | ||
#endif | ||
|
||
#if PYBIND11_INTERNALS_VERSION < 11 | ||
|
@@ -177,6 +182,12 @@ struct type_equal_to { | |
}; | ||
#endif | ||
|
||
template <typename value_type> | ||
// REVIEW: do we need to add a fancy hash for pointers or is the | ||
// possible identity hash function from the standard library (e.g., | ||
// libstdc++) sufficient? | ||
|
||
using fast_type_map = std::unordered_map<const std::type_info *, value_type>; | ||
|
||
template <typename value_type> | ||
using type_map = std::unordered_map<std::type_index, value_type, type_hash, type_equal_to>; | ||
|
||
|
@@ -237,6 +248,14 @@ struct internals { | |
pymutex mutex; | ||
pymutex exception_translator_mutex; | ||
#endif | ||
#if PYBIND11_INTERNALS_VERSION >= 12 | ||
// non-normative but fast "hint" for | ||
// registered_types_cpp. Successful lookups are correct; | ||
// unsuccessful lookups need to try registered_types_cpp and then | ||
// backfill this map if they find anything. | ||
fast_type_map<type_info *> registered_types_cpp_fast; | ||
#endif | ||
|
||
// std::type_index -> pybind11's type information | ||
type_map<type_info *> registered_types_cpp; | ||
// PyTypeObject* -> base type_info(s) | ||
|
@@ -261,7 +280,9 @@ struct internals { | |
PyObject *instance_base = nullptr; | ||
// Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: | ||
thread_specific_storage<PyThreadState> tstate; | ||
#if PYBIND11_INTERNALS_VERSION <= 11 | ||
thread_specific_storage<loader_life_support> loader_life_support_tls; // OBSOLETE (PR #5830) | ||
#endif | ||
// Unused if PYBIND11_SIMPLE_GIL_MANAGEMENT is defined: | ||
PyInterpreterState *istate = nullptr; | ||
|
||
|
@@ -302,7 +323,11 @@ struct internals { | |
// impact any other modules, because the only things accessing the local internals is the | ||
// module that contains them. | ||
struct local_internals { | ||
type_map<type_info *> registered_types_cpp; | ||
// It should be safe to use fast_type_map here because this entire | ||
// data structure is scoped to our single module, and thus a single | ||
// DSO and single instance of type_info for any particular type. | ||
fast_type_map<type_info *> registered_types_cpp; | ||
|
||
std::forward_list<ExceptionTranslator> registered_exception_translators; | ||
PyTypeObject *function_record_py_type = nullptr; | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,21 +205,39 @@ PYBIND11_NOINLINE detail::type_info *get_type_info(PyTypeObject *type) { | |
return bases.front(); | ||
} | ||
|
||
inline detail::type_info *get_local_type_info(const std::type_index &tp) { | ||
auto &locals = get_local_internals().registered_types_cpp; | ||
auto it = locals.find(tp); | ||
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_index &tp) { | ||
inline detail::type_info *get_global_type_info(const std::type_info &tp) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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; | ||
auto it = types.find(tp); | ||
#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); | ||
#endif | ||
type_info = it->second; | ||
} | ||
return type_info; | ||
|
@@ -228,7 +246,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_index &tp, | ||
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.
Sorry I just realized this a bad idea and we shouldn't do this (although we've actually done this before and I'm not aware of complaints):
E.g. if someone builds extensions with Python 3.14 and pybind11 v3.0.0 or v3.0.1 (internals version 11), then other extensions with Python 3.14 and pybind11>3.0.1 (assuming internals version 12), they will not fully interoperate.
Preemptively bumping the internals version for Python 3.14 would be OK only if v3.0.0 or v3.0.1 didn't work with Python 3.14, but I believe they do (although I'm not sure, but I don't have time to find out for sure).
To avoid surprising users, could you please undo the
PY_VERSION_HEX >= 0x030E0000
change here?We need to make a couple surgical changes to
ci.yml
to get test coverage. See my older comment.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.
OK. I took a guess and used the inplace build for the smoke test