gh-111569: Implement Python critical section API #111571
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
Critical sections are helpers to replace the global interpreter lock with finer grained locking. They provide similar guarantees to the GIL and avoid the deadlock risk that plain locking involves. Critical sections are implicitly ended whenever the GIL would be released. They are resumed when the GIL would be acquired. Nested critical sections behave as if the sections were interleaved.
|
@ericsnowcurrently I'd appreciate your review if you have time to look at this. This builds off of the |
zooba
reviewed
Oct 31, 2023
| }; | ||
| assert(test_data.obj1 != NULL); | ||
| assert(test_data.obj2 != NULL); | ||
| assert(test_data.obj3 != NULL); |
There was a problem hiding this comment.
Just a drive-by comment, I would much prefer to have conditional returns from native test functions than asserts. When these fail, they can block test progress (on Windows it pops up a modal dialog by default), or may trigger error reporting or a debugger. It also shows up as a test crash, rather than a failure.
Perhaps a new macro is needed to print location information to the console and then fail?
There was a problem hiding this comment.
I agree that the failure case is unpleasant, but I don't think trying to route proper error returns in the C API tests is worthwhile. To the extent that there are bugs in the tested code, they are likely to show up as crashes, deadlocks, or other unpleasant behavior even if we make the asserts more nicely behaved (because this is testing C code, not Python). Additionally, it's challenging to route error messages from other threads in C since they don't always have a GIL context and might not be able to create Python error objects.
One consequence of this is that I think we should basically have a zero tolerance for flaky C API tests. If we are going to be bothered by an unpleasant failure, it better signify something important.
If crashes in the C API tests become an actual problem, then I think it would be better to invest in running these tests in a subprocess like we do with test_support.script_helper.assert_python_failure.
There was a problem hiding this comment.
Yeah, that's reasonable. I'm mostly annoyed about it because some versions of MSVC have a compiler bug on ARM64 that causes some of the interlocked tests to fail, which means the entire test run is basically useless until all the machines get updated (as far as I can tell, the code is only ever tested and never actually used, so it doesn't need fixing for our releases - there's nothing we can do about it on our side anyway).
ericsnowcurrently
requested changes
Nov 3, 2023
There was a problem hiding this comment.
At its conceptual core this is a relatively straight-forward addition (inasmuch as anything thread-related can be 😄). I've left comments for a few typos, some minor readability issues, and in places where I'd like some clarification.
In particular, how is reentrancy handled?
I'm also especially curious about critical sections involving many objects, not just one or two. See my comment in pycore_critical_section.h, line 45.
FWIW, it's also nice to see how a lot of the code paths you are adding gets exercised on regular builds (even if the fundamental logic is skipped there). That certainly gives me more confidence about maintainability. 😄
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
colesbury
commented
Nov 6, 2023
There was a problem hiding this comment.
Thanks for reviewing this Eric.
In particular, how is reentrancy handled?
Reentrancy is handled implicitly. Basically, if a thread tries to Py_BEGIN_CRITICAL_SECTION the same object, it would block, calling _PyThreadState_Detach(), which suspends the outer critical section and unlocks the object, allowing the most recent Py_BEGIN_CRITICAL_SECTION to continue. We may want to handle this case more explicitly in the future for better efficiency.
I'm also especially curious about critical sections involving many objects, not just one or two.
This doesn't provide an API for locking more than two objects at at once. That hasn't been necessary in the nogil forks. (We could implement it, but my feeling is that if we have a design that wants to lock N objects simultaneously, we probably want to rethink that design.)
|
I have made the requested changes; please review again |
ericsnowcurrently
requested changes
Nov 8, 2023
|
Thanks @ericsnowcurrently. I've updated the comments. |
|
|
|
Looks like this PR is causing the WASI buildbots to time out on the new |
|
Oops. I'll put up a PR to fix WASI tomorrow morning. |
aisk
pushed a commit
to aisk/cpython
that referenced
this pull request
Feb 11, 2024
Critical sections are helpers to replace the global interpreter lock with finer grained locking. They provide similar guarantees to the GIL and avoid the deadlock risk that plain locking involves. Critical sections are implicitly ended whenever the GIL would be released. They are resumed when the GIL would be acquired. Nested critical sections behave as if the sections were interleaved.
Glyphack
pushed a commit
to Glyphack/cpython
that referenced
this pull request
Sep 2, 2024
Critical sections are helpers to replace the global interpreter lock with finer grained locking. They provide similar guarantees to the GIL and avoid the deadlock risk that plain locking involves. Critical sections are implicitly ended whenever the GIL would be released. They are resumed when the GIL would be acquired. Nested critical sections behave as if the sections were interleaved.
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.
Critical sections are helpers to replace the global interpreter lock with finer grained locking. They provide similar guarantees to the GIL and avoid the deadlock risk that plain locking involves. Critical sections are implicitly ended whenever the GIL would be released. They are resumed when the GIL would be acquired. Nested critical sections behave as if the sections were interleaved.