-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
gh-121464: Make concurrent iteration over enumerate safe under free-threading #125734
Open
eendebakpt
wants to merge
14
commits into
python:main
Choose a base branch
from
eendebakpt:enumerate_ft_v4
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+79
−20
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
eendebakpt
changed the title
Draft: gh-120608: Make concurrent iteration over enumerate safe under free-threading
Draft: gh-121464: Make concurrent iteration over enumerate safe under free-threading
Oct 19, 2024
eendebakpt
changed the title
Draft: gh-121464: Make concurrent iteration over enumerate safe under free-threading
gh-121464: Make concurrent iteration over enumerate safe under free-threading
Oct 19, 2024
My expertise is with the Python-based |
rruuaanng
reviewed
Oct 21, 2024
|
||
|
||
class EnumerateThreading(unittest.TestCase): | ||
@staticmethod |
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.
This @staticmethod
seems to be removable.
eendebakpt
commented
Jan 3, 2025
cc @colesbury |
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.
We make concurrent iteration with
enumerate
thread-safe (e.g. not crash, correct results are not guaranteed, see #124397) byUsing
_PyObject_IsUniquelyReferenced
for the re-use of the result tupleIn
enum_next
usingFT_ATOMIC_LOAD_SSIZE_RELAXED
/FT_ATOMIC_STORE_SSIZE_RELAXED
for loading and saving of the mutableen->en_index
In
enum_next_long
(andenum_reduce
) we have to deal withen->en_longindex
which is mutable. Since the objects inen_longindex
need to be properly refcounted this is a bit hard to handle. Two options:i) Use a lock. This is a simple approach and the performance cost might be acceptable since use cases of
enumerate
with numbers that do not fit in a long are rareii) The
en->en_longindex
can be atomically swapped withstepped_up
using_Py_atomic_exchange_ptr
. With this approachen->en_longindex
is either zero, or has a refcount of 1. The thread performing the swap ofen->en_longindex
can decrement the olden->en_longindex
. A branch with this approach is enumerate_ft_v3.In this PR we pick option i) because it is simpler and more robust.