Skip to content

gh-150902: Optimize PyCriticalSection2 to skip locking the same locks as the ones held by the current CS2#151085

Merged
colesbury merged 3 commits into
python:mainfrom
dpdani:gh/150902-pycs2-skip-recursive
Jun 16, 2026
Merged

gh-150902: Optimize PyCriticalSection2 to skip locking the same locks as the ones held by the current CS2#151085
colesbury merged 3 commits into
python:mainfrom
dpdani:gh/150902-pycs2-skip-recursive

Conversation

@dpdani

@dpdani dpdani commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This mimics an optimization already present for the single-mutex critical section.

…me locks as the ones held by the current CS2

@colesbury 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.

Looks good, a few comments from Claude:

  1. Stale comment in _PyCriticalSection2_End (pycore_critical_section.h:199-202). This comment is now inaccurate and should be updated as part of this PR:
  // if mutex1 is NULL, we used the fast path in
  // _PyCriticalSection_BeginSlow for mutexes that are already held,
  // which should only happen when mutex1 and mutex2 were the same mutex,
  After this change, _cs_mutex == NULL also occurs when m1 != m2 but both were already held by the current CS2. The "should only happen when mutex1 and mutex2 were the same mutex" claim
  no longer holds.

  2. Pointer comparison in the asserts. assert(m1 < m2) and assert(prev2->_cs_base._cs_mutex < prev2->_cs_mutex2) compare PyMutex * directly. Relational comparison of unrelated pointers
  is technically UB, and the header deliberately casts to (uintptr_t) when sorting (line 163). For consistency, cast here too:
  assert((uintptr_t)m1 < (uintptr_t)m2);

  4. Minor: ordering differs from the single-mutex version. Here the world-stopped check comes before the recursive-skip; in _PyCriticalSection_BeginSlow it's the reverse. Both produce
  identical results (NULL mutexes, no push), so this is harmless — just a slight inconsistency.

@dpdani

dpdani commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

I'll address them, thanks Claude

@dpdani

dpdani commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

... and it still cannot count 😁

@colesbury

Copy link
Copy Markdown
Contributor

... and it still cannot count 😁

I deleted 3. because I didn't think it was relevant

@colesbury colesbury added needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Jun 16, 2026
@colesbury colesbury enabled auto-merge (squash) June 16, 2026 16:43
@colesbury colesbury merged commit c2ca772 into python:main Jun 16, 2026
58 checks passed
@miss-islington-app

Copy link
Copy Markdown

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

@dpdani

dpdani commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Ah ok, sorry Claude, was too quick to judge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs backport to 3.14 bugs and security fixes needs backport to 3.15 pre-release feature fixes, bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants