GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117657: Fix itertools.count thread safety by wiggin15 · Pull Request #119268 · python/cpython

Conversation

@wiggin15

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

Uh oh!

There was an error while loading. Please reload this page.

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.

  • Issue: Make TSAN tests pass with the GIL disabled in free-threaded builds #117657

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@wiggin15
wiggin15 requested a review from rhettinger as a code owner May 20, 2024 20:49
@ghost

ghost commented May 20, 2024 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added the awaiting review label May 20, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 20, 2024
Make TSAN tests pass with the GIL disabled in free-threaded builds #117657
Closed
@DinoV
DinoV self-requested a review May 20, 2024 20:50
@DinoV DinoV added skip news topic-free-threading labels May 20, 2024
@rhettinger

rhettinger commented May 20, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DinoV
DinoV requested a review from colesbury May 20, 2024 21:08
colesbury
colesbury reviewed May 20, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_itertools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_itertools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_itertools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_itertools.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/itertoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@wiggin15

wiggin15 commented May 20, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger

rhettinger commented May 20, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@wiggin15
wiggin15 force-pushed the main branch from f2234ae to d609aac Compare May 20, 2024 23:10
@wiggin15

wiggin15 commented May 20, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@wiggin15
wiggin15 requested a review from colesbury May 21, 2024 00:48
rhettinger
rhettinger reviewed May 21, 2024
View reviewed changes
Comment thread Modules/itertoolsmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger

rhettinger commented May 21, 2024

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@wiggin15
Fix itertools.count in free-threading mode
2cebdb3
@wiggin15
wiggin15 force-pushed the main branch from d609aac to 2cebdb3 Compare May 21, 2024 14:35
colesbury
colesbury approved these changes May 21, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 21, 2024
@DinoV
DinoV merged commit 87939bd into python:main May 21, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 21, 2024
@colesbury

colesbury commented Jun 3, 2024

Copy link
Copy Markdown
Contributor

Oops, forgot to backport this to 3.13

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@colesbury colesbury added the needs backport to 3.13 only security fixes label Jun 3, 2024
@miss-islington-app

miss-islington-app Bot commented 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.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

miss-islington-app Bot commented Jun 3, 2024

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned DinoV Jun 3, 2024
@colesbury colesbury self-assigned this Jun 3, 2024
@colesbury colesbury unassigned DinoV Jun 3, 2024
colesbury pushed a commit to colesbury/cpython that referenced this pull request Jun 3, 2024
@wiggin15 @colesbury
[3.13] pythongh-117657: Fix itertools.count thread safety (pythonGH-1…
5576e64
…19268)

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

Co-authored-by: Arnon Yaari <[email protected]>
@bedevere-app

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jun 3, 2024
colesbury added a commit that referenced this pull request Jun 3, 2024
@colesbury @wiggin15
[3.13] gh-117657: Fix itertools.count thread safety (GH-119268) (#120007
79fae3b
)

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

Co-authored-by: Arnon Yaari <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@wiggin15 @estyxx
pythongh-117657: Fix itertools.count thread safety (python#119268)
e6d705d
Fix itertools.count in free-threading mode
@colesbury colesbury removed their assignment Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger rhettinger left review comments
@colesbury colesbury colesbury approved these changes
@DinoV DinoV Awaiting requested review from DinoV

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@wiggin15 @rhettinger @colesbury @DinoV
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.