8000 gh-135871: Fix needless spinning in _PyMutex_LockTimed (timeout==0) by jtibbertsma · Pull Request #135872 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-135871: Fix needless spinning in _PyMutex 8000 _LockTimed (timeout==0) #135872

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 25, 2025
Merged

gh-135871: Fix needless spinning in _PyMutex_LockTimed (timeout==0) #135872

merged 5 commits into from
Jun 25, 2025
+3 −1

Conversation

jtibbertsma
Copy link
Contributor
@jtibbertsma jtibbertsma commented Jun 24, 2025 •
edited
Loading
  • Move the timeout == 0 guard outside the else so a non-blocking call returns immediately after a failed CAS instead of entering the spin loop.
  • Reload v on every spin iteration, allowing timed/blocking callers to notice an unlock promptly.

No-GIL builds now honor the semantics of non-blocking attempts and avoid wasted CPU; GIL builds are unaffected (MAX_SPIN_COUNT == 0).

@python-cla-bot
Copy link
8000
python-cla-bot bot commented Jun 24, 2025 •
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app

This comment was marked as resolved.

Sign in to view
@bedevere-app bedevere-app bot mentioned this pull request Jun 24, 2025
Open
@bedevere-app

This comment was marked as duplicate.

Sign in to view
@bedevere-app

This comment was marked as duplicate.

Sign in to view
@jtibbertsma
pythongh-135871: Fix needless spinning in _PyMutex_LockTimed (timeout…
7296a7d 
…==0)

* Move the timeout == 0 guard outside the else so a non-blocking call
  returns immediately after a failed CAS instead of entering the spin loop.
* Reload v on every spin iteration, allowing timed/blocking callers to
  notice an unlock promptly.

No-GIL builds now honor the semantics of non-blocking attempts and avoid
wasted CPU; GIL builds are unaffected (MAX_SPIN_COUNT == 0).
ZeroIntensity
ZeroIntensity reviewed Jun 24, 2025
View reviewed changes
Copy link
Member
@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Please don't force-push, we squash at the end.

@ZeroIntensity ZeroIntensity removed the request for review from pitrou June 24, 2025 01:58
@ZeroIntensity
Copy link
Member

(Sorry Antoine, hit the wrong button!)

colesbury
colesbury reviewed Jun 24, 2025
View reviewed changes
Python/lock.c Outdated
@@ -89,6 +89,7 @@ _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout, _PyLockFlags flags)
// Spin for a bit.
_Py_yield();
spin_count++;
v = _Py_atomic_load_uint8_relaxed(&m->_bits);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it fixes a logic bug but it also hurts performance

See Tools/lockbench/lockbench.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the review! I'm convinced, this line reduces throughput on my machine by something like 40% for 8 threads. I think changing else if to if still makes sense though. I updated the PR.

vstinner
vstinner approved these changes Jun 25, 2025
View reviewed changes
Copy link
Member
@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@vstinner vstinner added the needs backport to 3.13 bugs and security fixes label Jun 25, 2025
colesbury
colesbury approved these changes Jun 25, 2025
View reviewed changes
Copy link
Contributor
@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM

Let's investigate (and fix) the missing load separately

@colesbury colesbury enabled auto-merge (squash) June 25, 2025 16:15
@colesbury colesbury merged commit cbfaf41 into python:main Jun 25, 2025
40 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 25, 2025
@jtibbertsma @miss-islington
Fix needless spinning in _PyMutex_LockTimed with zero timeout (pyth…
535b8d6 
…ongh-135872)

The free threading build could spin unnecessarily on `_Py_yield()` if the initial
compare and swap failed.
(cherry picked from commit cbfaf41)

Co-authored-by: Joseph Tibbertsma <josephtibbertsma@gmail.com>
@miss-islington-app
Copy link

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

cherry_picker cbfaf41caf135b8598a560854cd59e992a2ccfed 3.13

@bedevere-app
Copy link
bedevere-app bot commented Jun 25, 2025

GH-135946 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Jun 25, 2025
@bedevere-app
Copy link
bedevere-app bot commented Jun 25, 2025

GH-135946 is a backport of this pull request to the 3.14 branch.

colesbury pushed a commit to colesbury/cpython that referenced this pull request Jun 25, 2025
@jtibbertsma @colesbury
[3.13] Fix needless spinning in _PyMutex_LockTimed with zero timeout (
697bdf5 
pythongh-135872)

The free threading build could spin unnecessarily on `_Py_yield()` if the initial
compare and swap failed.
(cherry picked from commit cbfaf41)

Co-authored-by: Joseph Tibbertsma <josephtibbertsma@gmail.com>
@bedevere-app
Copy link
bedevere-app bot commented Jun 25, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 25, 2025
@bedevere-app
Copy link
bedevere-app bot commented Jun 25, 2025

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

colesbury pushed a commit that referenced this pull request Jun 25, 2025
@miss-islington @jtibbertsma
[3.14] gh-135871: Fix needless spinning in _PyMutex_LockTimed with …
80fc62f 
…zero timeout (gh-135872) (gh-135946)

The free threading build could spin unnecessarily on `_Py_yield()` if the initial
compare and swap failed.
(cherry picked from commit cbfaf41)

Co-authored-by: Joseph Tibbertsma <josephtibbertsma@gmail.com>
colesbury added a commit that referenced this pull request Jun 25, 2025
@colesbury @jtibbertsma
[3.13] gh-135871: Fix needless spinning in _PyMutex_LockTimed with …
c64df2a 
…zero timeout (gh-135872) (gh-135947)

The free threading build could spin unnecessarily on `_Py_yield()` if the initial
compare and swap failed.
(cherry picked from commit cbfaf41)

Co-authored-by: Joseph Tibbertsma <josephtibbertsma@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments

@vstinner vstinner vstinner approved these changes

@colesbury colesbury colesbury approved these changes

Assignees

@colesbury colesbury

Labels
topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jtibbertsma @ZeroIntensity @vstinner @colesbury
0