Skip to content

gh-140795: Put the thread state in PyCriticalSection structs#146066

Closed
encukou wants to merge 1 commit intopython:mainfrom
encukou:critical-state
Closed

gh-140795: Put the thread state in PyCriticalSection structs#146066
encukou wants to merge 1 commit intopython:mainfrom
encukou:critical-state

Conversation

@encukou
Copy link
Member

@encukou encukou commented Mar 17, 2026

GH-141406 improved performance by only fetching thread state once and storing it in a variable on the stack.

This instead puts the thread state in the PyCriticalState struct (also a temp variable on the stack), bringing the public and private implementations closer together.

pythonGH-141406 improved performance by only fetching thread state once
and storing it in a variable on the stack.

This instead puts the thread state in the PyCriticalState struct
(also a temp variable on the stack), bringing the public and private
implementations closer together.
@colesbury
Copy link
Contributor

I used Claude to benchmark the PR on Linux in a few cases (main executable, Py_BUILD_CORE extension, non-stdlib extension). tl;dr it seems a 2-3% slower in microbenchmarks of Py_BEGIN/END_CRITICAL_SECTION and essentially no difference in larger benchmarks.

https://github.com/colesbury/gh-140795-benchmarks/blob/main/results.txt

@kumaraditya303
Copy link
Contributor

improved performance by only fetching thread state once and storing it in a variable on the stack.

The important part that this PR misses is that in my PR the thread state is passed in a register whereas here it will need to be loaded from stack, I don't see value in doing this unless the performance is atleast equal.

@encukou
Copy link
Member Author

encukou commented Mar 18, 2026

I see. In my (much more limited) testing this looked better.
Sorry for the noise!

@encukou encukou closed this Mar 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants