Skip to content
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

fix: use PyMutex instead of std::mutex in free-threaded build #5219

Merged
merged 3 commits into from
Jul 2, 2024
Merged
Changes from 2 commits
Commits
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
24 changes: 18 additions & 6 deletions include/pybind11/detail/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,20 +148,32 @@ struct override_hash {

using instance_map = std::unordered_multimap<const void *, instance *>;

#ifdef Py_GIL_DISABLED
// Wrapper around PyMutex to provide BasicLockable semantics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I missed this when I looked before:

Could you please remove the

#include <mutex>

near the top? (I double-checked that std::mutex is not referenced anymore in this file.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need it for std::unique_lock

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, ok, thanks.

class pymutex {
PyMutex mutex;

public:
pymutex() : mutex({}) {}
void lock() { PyMutex_Lock(&mutex); }
void unlock() { PyMutex_Unlock(&mutex); }
};

// Instance map shards are used to reduce mutex contention in free-threaded Python.
struct instance_map_shard {
std::mutex mutex;
instance_map registered_instances;
pymutex mutex;
// alignas(64) would be better, but causes compile errors in macOS before 10.14 (see #5200)
char padding[64 - (sizeof(std::mutex) + sizeof(instance_map)) % 64];
char padding[64 - (sizeof(instance_map) + sizeof(std::mutex)) % 64];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oversight? — Was this meant to be sizeof(pymutex) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks.

};
#endif

/// Internal data structure used to track registered instances and types.
/// Whenever binary incompatible changes are made to this structure,
/// `PYBIND11_INTERNALS_VERSION` must be incremented.
struct internals {
#ifdef Py_GIL_DISABLED
std::mutex mutex;
pymutex mutex;
#endif
// std::type_index -> pybind11's type information
type_map<type_info *> registered_types_cpp;
Expand Down Expand Up @@ -614,7 +626,7 @@ inline local_internals &get_local_internals() {
}

#ifdef Py_GIL_DISABLED
# define PYBIND11_LOCK_INTERNALS(internals) std::unique_lock<std::mutex> lock((internals).mutex)
# define PYBIND11_LOCK_INTERNALS(internals) std::unique_lock<pymutex> lock((internals).mutex)
#else
# define PYBIND11_LOCK_INTERNALS(internals)
#endif
Expand Down Expand Up @@ -651,7 +663,7 @@ inline auto with_instance_map(const void *ptr,
auto idx = static_cast<size_t>(hash & internals.instance_shards_mask);

auto &shard = internals.instance_shards[idx];
std::unique_lock<std::mutex> lock(shard.mutex);
std::unique_lock<pymutex> lock(shard.mutex);
return cb(shard.registered_instances);
#else
(void) ptr;
Expand All @@ -667,7 +679,7 @@ inline size_t num_registered_instances() {
size_t count = 0;
for (size_t i = 0; i <= internals.instance_shards_mask; ++i) {
auto &shard = internals.instance_shards[i];
std::unique_lock<std::mutex> lock(shard.mutex);
std::unique_lock<pymutex> lock(shard.mutex);
count += shard.registered_instances.size();
}
return count;
Expand Down