-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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-117657: Fix itertools.count thread safety #119268
Conversation
Can you arrange the In general, we don't want to damage the traditional build if we don't have to. Also, we would like maintainers to be able to easily look at the GIL path to understand what the function does and how it works. After the PR, I personally find it challenging to just read this code that used to be simple and obvious. |
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.
Thanks @wiggin15 for fixing this. The logic looks correct to me.
I've left some suggestions inline in addition to Raymond's comment above.
Just to be clear, are you suggesting to bring back the original code under an ifdef, like so? #ifdnef Py_GIL_DISABLED
// the original 3 lines, unmodified
#else
// free-threading code only
#endif ? |
+1 That would be a simple and reasonable way to preserve readability, maintainability, and performance. It would also make clear exactly what the no-gil code should be doing. |
Thank you @colesbury and @rhettinger for the constructive feedback. I pushed a revised commit. Please share your comments. |
The path for the traditional build seems fine to me. The no-gil path is much more readable than before but I'm not expert enough to sign-off on that path. |
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.
Thanks!
Oops, forgot to backport this to 3.13 |
Sorry, @wiggin15 and @DinoV, I could not cleanly backport this to
|
…19268) Fix itertools.count in free-threading mode (cherry picked from commit 87939bd) Co-authored-by: Arnon Yaari <[email protected]>
GH-120007 is a backport of this pull request to the 3.13 branch. |
) Fix itertools.count in free-threading mode (cherry picked from commit 87939bd) Co-authored-by: Arnon Yaari <[email protected]>
Fix itertools.count in free-threading mode
Thread safety in count_next. count_next has two modes. slow mode (obj->cnt set to PY_SSIZE_T_MAX), which now uses the object mutex (only if GIL is disabled) and fast mode, which is either simple cnt++ if GIL is enabled, or uses atomic_compare_exchange if GIL is disabled.