FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-117657: Fix itertools.count thread safety by wiggin15 · Pull Request #119268 · python/cpython · GitHub

/ cpython Public

gh-117657: Fix itertools.count thread safety - #119268

Merged
DinoV merged 1 commit into
python:mainfrom
wiggin15:main
May 21, 2024
Merged

gh-117657: Fix itertools.count thread safety#119268
DinoV merged 1 commit into
python:mainfrom
wiggin15:main

Conversation

wiggin15 commented May 20, 2024
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

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.

wiggin15 requested a review from rhettinger as a code owner May 20, 2024 20:49

ghost commented May 20, 2024
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

Copy link
Copy Markdown
Contributor

Can you arrange the #ifdef logic so that the traditional GIL build has EXACTLY the same code that it does now. That shouldn't be difficult since count_next() is only three lines of code.

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.

DinoV requested a review from colesbury May 20, 2024 21:08

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @wiggin15 for fixing this. The logic looks correct to me.

I've left some suggestions inline in addition to Raymond's comment above.

Comment thread Lib/test/test_itertools.py Outdated
Comment thread Lib/test/test_itertools.py Outdated
Comment thread Lib/test/test_itertools.py Outdated
Comment thread Modules/itertoolsmodule.c Outdated

wiggin15 commented May 20, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

Can you arrange the #ifdef logic so that the traditional GIL build has EXACTLY the same code that it does now. That shouldn't be difficult since count_next() is only three lines of code.

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

?

Copy link
Copy Markdown
Contributor

Just to be clear, are you suggesting to bring back the original code under an ifdef, like so?

+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.

Copy link
Copy Markdown
Contributor Author

Thank you @colesbury and @rhettinger for the constructive feedback. I pushed a revised commit. Please share your comments.

wiggin15 requested a review from colesbury May 21, 2024 00:48
Comment thread Modules/itertoolsmodule.c Outdated

Copy link
Copy Markdown
Contributor

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.

colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks!

DinoV merged commit 87939bd into python:main May 21, 2024

Copy link
Copy Markdown
Contributor

Oops, forgot to backport this to 3.13

colesbury added the needs backport to 3.13 bugs and security fixes label Jun 3, 2024

Copy link
Copy Markdown

Thanks @wiggin15 for the PR, and @DinoV for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

Copy link
Copy Markdown

Sorry, @wiggin15 and @DinoV, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 87939bd5790accea77c5a81093f16f28d3f0b429 3.13

colesbury self-assigned this Jun 3, 2024
colesbury pushed a commit to colesbury/cpython that referenced this pull request Jun 3, 2024
…19268)

Fix itertools.count in free-threading mode
(cherry picked from commit 87939bd)

Co-authored-by: Arnon Yaari <wiggin15@yahoo.com>

bedevere-app Bot commented Jun 3, 2024

Copy link
Copy Markdown

GH-120007 is a backport of this pull request to the 3.13 branch.

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 3, 2024
colesbury added a commit that referenced this pull request Jun 3, 2024
)

Fix itertools.count in free-threading mode
(cherry picked from commit 87939bd)

Co-authored-by: Arnon Yaari <wiggin15@yahoo.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
colesbury removed their assignment Jul 19, 2024
This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL