gh-108724: Add PyMutex and _PyParkingLot APIs #109344
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`PyMutex` is a one byte lock with fast, inlineable lock and unlock functions for the common uncontended case. The design is based on WebKit's `WTF::Lock`. PyMutex is built using the `_PyParkingLot` APIs, which provides a cross-platform futex-like API (based on WebKit's `WTF::ParkingLot`). This internal API will be used for building other synchronization primitives used to implement PEP 703, such as one-time initialization and events.
colesbury
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 3161e17 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
!buildbot ARM Raspbian |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit f963fd8 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
|
!buildbot wasm |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit f963fd8 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
This was referenced Sep 13, 2023
The lock tests were being picked up twice: once in test_lock.py and once in Test_testinternalcapi from test_misc.py. The Test_testinternalcapi was not skipping tests when the platform doesn't have threads. This moves the tests to test_misc.py, doesn't include them in Test_testinternalcapi, and skips them if the platform doesn't support threads.
|
!buildbot wasm |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 779a401 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
ericsnowcurrently
requested changes
Sep 13, 2023
| // the mutex is unlocked. If the current thread holds the GIL, then the GIL | ||
| // will be released while the thread is parked. | ||
| static inline void | ||
| PyMutex_Lock(PyMutex *m) |
There was a problem hiding this comment.
I was going to recommend using "acquire"/"release" instead of "lock"/"unlock", as the former is quite prevalent in this context (and, to my brain, "lock" evokes the object rather than the action). However, there are definitely examples of "lock"/"unlock".
Ultimately, it would be nice if the names were consistent everywhere, to avoid confusing readers. (It would also be nice if the naming were consistent in the industry and academia.) The closer we can get, the better. I'd rather this PR help us converge than diverge, but I haven't taken the time to decide which way the PR goes.😄 (My initial gut reaction was that it diverges.) Feedback on this from a couple of other core devs would be helpful.
There was a problem hiding this comment.
Lock/unlock is much more common in libraries and programming languages:
- pthread_mutex_t (POSIX)
- std::mutex (C++)
- std::sync::mutex (Rust)
- java.util.concurrent.locks.Lock (Java)
- sync.Mutex (Go)
Windows tends to use "wait"/"release" terminology.
There was a problem hiding this comment.
FWIW, I'm fine with lock/unlock (vs. acquire/release) at this point.
Regardless, I don't see this as a blocker for the PR, since it isn't public API (yet--and the name would get more scrutiny during the PEP process).
Release the GIL if `detach` is `1` and there is a current thread state. It's the caller's responsibility to make sure that if there is a valid thread state (and `detach` is 1), then the current thread holds the GIL.
colesbury
commented
Sep 15, 2023
There was a problem hiding this comment.
Thanks for your continued review. Here's a summary of the major changes:
- I removed the thread-local data from
parking_lot.c. It wasn't strictly necessary and didn't actually improve performance. - I switched to statically initializing the
bucketslinked lists inparking_lot.c, which avoids needing to check if it's initialized inenqueue/dequeue. - I moved the
_PySemaphorestruct definition topycore_semaphore.hbecause it now includes platform-specific details and I'd like to avoid exposing those inpycore_parking_lot.h(I expect more files to use the parking lot API than the_PySemaphoreAPI.) - I switched
_PyParkingLot_Unparkto use a function pointer as a callback, like you asked about in the previous review. I think this ends up being a bit nicer, but let me know what you think.
ericsnowcurrently
requested changes
Sep 15, 2023
colesbury
commented
Sep 18, 2023
|
BTW, thanks for your work on this and for addressing all my concerns. |
|
I'm going to wait until tomorrow, to accommodate timezones. 😄 |
|
Thanks again, @colesbury! |
|
|
|
@colesbury ^^^ |
|
|
|
The AMD64 Ubuntu Shared 3.x failure is due to an unrelated flaky test. |
|
|
|
The WASM buildbot failures are fixed by gh-109583. |
csm10495
pushed a commit
to csm10495/cpython
that referenced
this pull request
Sep 28, 2023
PyMutex is a one byte lock with fast, inlineable lock and unlock functions for the common uncontended case. The design is based on WebKit's WTF::Lock. PyMutex is built using the _PyParkingLot APIs, which provides a cross-platform futex-like API (based on WebKit's WTF::ParkingLot). This internal API will be used for building other synchronization primitives used to implement PEP 703, such as one-time initialization and events. This also includes tests and a mini benchmark in Tools/lockbench/lockbench.py to compare with the existing PyThread_type_lock. Uncontended acquisition + release: * Linux (x86-64): PyMutex: 11 ns, PyThread_type_lock: 44 ns * macOS (arm64): PyMutex: 13 ns, PyThread_type_lock: 18 ns * Windows (x86-64): PyMutex: 13 ns, PyThread_type_lock: 38 ns PR Overview: The primary purpose of this PR is to implement PyMutex, but there are a number of support pieces (described below). * PyMutex: A 1-byte lock that doesn't require memory allocation to initialize and is generally faster than the existing PyThread_type_lock. The API is internal only for now. * _PyParking_Lot: A futex-like API based on the API of the same name in WebKit. Used to implement PyMutex. * _PyRawMutex: A word sized lock used to implement _PyParking_Lot. * PyEvent: A one time event. This was used a bunch in the "nogil" fork and is useful for testing the PyMutex implementation, so I've included it as part of the PR. * pycore_llist.h: Defines common operations on doubly-linked list. Not strictly necessary (could do the list operations manually), but they come up frequently in the "nogil" fork. ( Similar to https://man.freebsd.org/cgi/man.cgi?queue) --------- Co-authored-by: Eric Snow <[email protected]>
|
This change introduced new compiler warnings: see issue gh-110014. |
This was referenced Oct 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PyMutexis a one byte lock with fast, inlineable lock and unlock functions for the common uncontended case. The design is based on WebKit'sWTF::Lock.PyMutex is built using the
_PyParkingLotAPIs, which provides a cross-platform futex-like API (based on WebKit'sWTF::ParkingLot). This internal API will be used for building other synchronization primitives used to implement PEP 703, such as one-time initialization and events.This also includes tests and a mini benchmark in
Tools/lockbench/lockbench.pyto compare with the existingPyThread_type_lock.Uncontended acquisition + release:
Linux (x86-64): PyMutex: 11 ns, PyThread_type_lock: 44 ns
macOS (arm64): PyMutex: 13 ns, PyThread_type_lock: 18 ns
Windows (x86-64): PyMutex: 13 ns, PyThread_type_lock: 38 ns
PR Overview
The primary purpose of this PR is to implement
PyMutex, but there are a number of support pieces (described below).PyMutex: A 1-byte lock that doesn't require memory allocation to initialize and is generally faster than the existingPyThread_type_lock. The API is internal only for now._PyParking_Lot: A futex-like API based on the API of the same name in WebKit. Used to implementPyMutex._PyRawMutex: A word sized lock used to implement_PyParking_LotPyEvent: a one time event. This was used a bunch in the "nogil" fork and is useful for testing thePyMuteximplementation, so I've included it as part of the PR.pycore_llist.h: Defines common operations on doubly-linked list. Not strictly necessary (could do the list operations manually), but they come up frequently in the "nogil" fork. (Similar to https://man.freebsd.org/cgi/man.cgi?queue)Linked issue