GH-128914: Remove all but one conditional stack effects by markshannon · Pull Request #129226 · python/cpython · GitHub
Skip to content

GH-128914: Remove all but one conditional stack effects - #129226

Merged
markshannon merged 9 commits into
python:mainfrom
faster-cpython:remove-most-conditional-stack-effects
Jan 27, 2025
Merged

GH-128914: Remove all but one conditional stack effects#129226
markshannon merged 9 commits into
python:mainfrom
faster-cpython:remove-most-conditional-stack-effects

Conversation

@markshannon

@markshannon markshannon commented Jan 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Conditional stack effects fall into three categories:

  • Pushes NULL if oparg & 1
  • LOAD_ATTR which calls _PyObject_GetMethod if oparg & 1
  • Not necessary: could be handled with an array or in the compiler.

This PR:

  • Adds a new uop _PUSH_NULL_CONDIITONAL that pushes NULL if oparg & 1 and uses it in all instructions that pushed NULL conditionally, so that those ops no longer have conditional stack effects.
  • Converts LOAD_ATTR to use self_of_null[oparg & 1] instead of self_or_null if (oparg & 1)`. This has no meaningful change on the generated code, so no impact on the interpreter. It will force the JIT to spill any registers, but we expect all jitted code to be specialized anyway.
  • Changes BUILD_SLICE to use an array of args. Give CALL_FUNCTION_EX has a fixed stack effect by and pushing NULL in the compiler when **kwargs is not defined.

A future PR will remove support for conditional stack effects and the split annotation.

@bedevere-app bedevere-app Bot mentioned this pull request Jan 23, 2025
Closed
@markshannon

markshannon commented Jan 23, 2025

Copy link
Copy Markdown
Member Author

@markshannon

markshannon commented Jan 23, 2025

Copy link
Copy Markdown
Member Author

Performance for free-threading is about 1% slower, mostly attributable to the Richards benchmarks. Which suggests to me that the change to LOAD_ATTR needs a rethink to restore the non-specialized performance.

@markshannon

markshannon commented Jan 24, 2025

Copy link
Copy Markdown
Member Author

I needed to effectively revert the change to LOAD_ATTR for performance reasons, so sys.monitoring behavior is unchanged.

@markshannon

markshannon commented Jan 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Performance is neutral on the default build.

Free-threading shows a 1% slowdown which makes no sense given the stats and code changes.
The changes to the generated code are minimal and there are no changes that would introduce contention.
The only change to instructions executed is about 100M extra PUSH_NULLs out of over 200B instructions (1 in 2000) and PUSH_NULL is a very cheap instruction. From that I would expect < 0.05% slowdown.

@markshannon
markshannon merged commit 75b4962 into python:main Jan 27, 2025
@bedevere-bot

bedevere-bot commented Jan 27, 2025

Copy link
Copy Markdown

@markshannon
markshannon deleted the remove-most-conditional-stack-effects branch January 31, 2025 16:57
@MatthieuDartiailh MatthieuDartiailh mentioned this pull request Aug 24, 2025
Open
@emmettbutler emmettbutler mentioned this pull request Sep 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@markshannon @bedevere-bot @iritkatriel