Skip to content

Update 'inactive_override_cache' key #2772

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

Closed
wants to merge 2 commits into from
Closed

Update 'inactive_override_cache' key #2772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24e6f88
Update 'inactive_override_cache' key
BetsyMcPhail Jan 15, 2020
7e991aa
fixup! address review
EricCousineau-TRI Jan 8, 2021
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
14 changes: 10 additions & 4 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,16 @@ struct type_equal_to {
template <typename value_type>
using type_map = std::unordered_map<std::type_index, value_type, type_hash, type_equal_to>;

// Adapted from boost::hash_combine(). See also:
// https://stackoverflow.com/a/27952689/7829525.
inline void hash_combine(size_t& lhs, size_t rhs ) {
lhs ^= rhs + 0x9e3779b9 + (lhs << 6) + (lhs >> 2);
}

struct override_hash {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: the change to this hash is probably also breaking ABI?

inline size_t operator()(const std::pair<const PyObject *, const char *>& v) const {
inline size_t operator()(const std::pair<const PyObject *, std::string>& v) const {
size_t value = std::hash<const void *>()(v.first);
value ^= std::hash<const void *>()(v.second) + 0x9e3779b9 + (value<<6) + (value>>2);
hash_combine(value, std::hash<std::string>()(v.second));
return value;
}
};
Comment on lines +85 to 97
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, this at least doesn't regress the hash like #2092 does. However this is still not what Boost's hash_combine does. Boost's equivalent would be:

size_t seed = 0;
size_t lhs = std::hash<const void*>()(v.first); // Same as here
size_t rhs = std::hash<std::string>()(v.second); // Same as here
seed ^= lhs + 0x9e3779b9 + (seed << 6) + (seed >> 2); // This line is missing!
seed ^= rhs + 0x9e3779b9 + (seed << 6) + (seed >> 2);

Right now, instead of the first seed ^= lhs... we're just using seed = lhs. With initial seed being 0, boost's hash_combine would really do just lhs += 0x9e3779b9; before the lhs ^= rhs + ... line.

Full disclaimer, I do not claim to be an expert on good hashing functions. I'm just noticing the slight difference. I don't actually know if this should be addressed. I would however trust Boost's implementation on the grounds of "boost::hash_combine was written by someone who cares about hashes more than I do". Also, the boost::hash_combine has had this magic number since the beginning, in 2005.

Expand All @@ -97,7 +103,7 @@ struct internals {
type_map<type_info *> registered_types_cpp; // std::type_index -> pybind11's type information
std::unordered_map<PyTypeObject *, std::vector<type_info *>> registered_types_py; // PyTypeObject* -> base type_info(s)
std::unordered_multimap<const void *, instance*> registered_instances; // void * -> instance*
std::unordered_set<std::pair<const PyObject *, const char *>, override_hash> inactive_override_cache;
std::unordered_set<std::pair<const PyObject *, std::string>, override_hash> inactive_override_cache;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Er... mayhaps this change requires a bump on the internal version.

I can't think of an easy to way to keep const char* around, as I'm unaware of what happens with Python strings 'n such...
Ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely needs to bump that, yes :-/ That does complicate things slightly, release-wise?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ouch, not going to do that until the next release, at least. We should bundle all such changes together and then do a bump, I think. Also, I thought standard library templated classes were not valid in an ABI (@jpivarski)? I take it this is why we have so many issues with the ABI? Could we redesign this to just be simple classes and do our own memory management here?

Copy link
Collaborator Author

@EricCousineau-TRI EricCousineau-TRI Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I think that could be done! I'll check out Boris's suggestion in Betsy's original PR, tho, and see if that gives us const char* 🤞

From Boris:

This should probably use detail::get_fully_qualified_tp_name (from #2520) [...]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dern, looking at #2520, it's std::string. But yeah, I can see about doing some naive memory management.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TLDR: for now, if we can keep the ABI the same, that's best. Eventually we will need a bump. In the future, maybe we can design internals that work cross-compiler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see. Cool, I'll re-open and re-scope #2773 to capture this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I made an "abi break" - very-brighly colored - label ( abi break ). Let's use this to indicate PRs that break the ABI and should be pooled together?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@henryiii Agreed that it would be amazing if we can get rid of these and use something that's basically shared between all compilers (guess that's POD types and extern "C"?). Just curious if ríght now, we have a problem when using STL in the internals, or whether that's at least +- safely worked around.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current workaround is fine, but something we'd like to possibly redesign to enable wider compatibility in the future, perhaps. In the current design, std::string should be fine, I think it's not worse than unordered map or anything else. I know std::vector is not allowed, so guessing pretty much everything else is out. Somewhere I think @bstaletic just listed the options, which I think is what I was thinking; either we do everything ourselves, or we pay a cost. (Though I feel like we can probably do pretty well). But also haven't really looked at this part of the code much at all, mostly pretty general observations ATM.

type_map<std::vector<bool (*)(PyObject *, void *&)>> direct_conversions;
std::unordered_map<const PyObject *, std::vector<PyObject *>> patients;
std::forward_list<void (*) (std::exception_ptr)> registered_exception_translators;
Expand Down Expand Up @@ -150,7 +156,7 @@ struct type_info {
};

/// Tracks the `internals` and `type_info` ABI version independent of the main library version
#define PYBIND11_INTERNALS_VERSION 4
#define PYBIND11_INTERNALS_VERSION 5

/// On MSVC, debug and release builds are not ABI-compatible!
#if defined(_MSC_VER) && defined(_DEBUG)
Expand Down
3 changes: 2 additions & 1 deletion include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2221,7 +2221,8 @@ inline function get_type_override(const void *this_ptr, const type_info *this_ty
if (!self)
return function();
handle type = type::handle_of(self);
auto key = std::make_pair(type.ptr(), name);
std::string full_name = get_fully_qualified_tp_name((PyTypeObject*) type.ptr()) + "." + name;
auto key = std::make_pair(type.ptr(), full_name);

/* Cache functions that aren't overridden in Python to avoid
many costly Python dictionary lookups below */
Expand Down