Skip to content
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

Get rid of conditional inputs and outputs for instructions in bytecodes.c #128914

Closed
markshannon opened this issue Jan 16, 2025 · 4 comments
Closed

Get rid of conditional inputs and outputs for instructions in bytecodes.c #128914

markshannon opened this issue Jan 16, 2025 · 4 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement

Comments

@markshannon
Copy link
Member

markshannon commented Jan 16, 2025 •
edited by bedevere-app bot
Loading

We should remove the conditional stack effects in instruction definitions in bytecodes.c

Conditional stack effects already complicate code generation and that is only going to get worse with top-of-stack caching and other interpreter/JIT optimizations.

There were two reasons for having conditional stack effects:

  1. Ease of porting the old ceval code to bytecodes.c
  2. Performance

Reason 1 no longer applies. Instructions are much more regular now and it isn't that much work to remove the remaining conditional stack effects.

That leaves performance. I experimentally removed the conditional stack effects for LOAD_GLOBAL and LOAD_ATTR which is the worse possible case for performance as it makes no attempt to mitigate the extra dispatch costs and possibly worse specialization.
The results are here
Overall we see a 0.8% slowdown. It seems that specialization is not significantly worse, but there is a large increase in PUSH_NULL following LOAD_GLOBAL that appears to responsible for the slowdown. An extra specialization should fix that.

Prior discussion

Linked PRs
@bedevere-app bedevere-app bot mentioned this issue Jan 16, 2025
Merged
@picnixz picnixz added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Jan 17, 2025
@picnixz
Copy link
Member

picnixz commented Jan 17, 2025

(Tagging it as a performance change even though we would end up with a slowdown. However, this is still something that is performance-related)

markshannon added a commit that referenced this issue Jan 20, 2025
@markshannon
GH-128914: Remove conditional stack effects from bytecodes.c and th…
ab61d3f 
…e code generators (GH-128918)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this issue Jan 21, 2025
@markshannon @srinivasreddy
pythonGH-128914: Remove conditional stack effects from bytecodes.c …
b0a2da6 
…and the code generators (pythonGH-128918)
colesbury added a commit to colesbury/cpython that referenced this issue Jan 22, 2025
@colesbury
Revert "pythonGH-128914: Remove conditional stack effects from `bytec…
68ce740 
…odes.c` and the code generators (pythonGH-128918)"

The commit introduced a large performance regression in the free threading build.

This reverts commit ab61d3f.
colesbury added a commit to colesbury/cpython that referenced this issue Jan 22, 2025
@colesbury
Revert "pythonGH-128914: Remove conditional stack effects from `bytec…
f60435e 
…odes.c` and the code generators (pythonGH-128918)"

The commit introduced a ~2.5-3% regression in the free threading build.

This reverts commit ab61d3f.
@bedevere-app bedevere-app bot mentioned this issue Jan 22, 2025
Merged
markshannon pushed a commit that referenced this issue Jan 23, 2025
@colesbury
Revert "GH-128914: Remove conditional stack effects from `bytecodes.c…
a10f993 
…` and the code generators (GH-128918)" (GH-129202)

The commit introduced a ~2.5-3% regression in the free threading build.

This reverts commit ab61d3f.
@markshannon
Copy link
Member Author

markshannon commented Jan 23, 2025 •
edited
Loading

It seems that #128918 caused a 2-3% slowdown on the free-threading build.

@markshannon markshannon reopened this Jan 23, 2025
@bedevere-app bedevere-app bot mentioned this issue Jan 23, 2025
Merged
@mdboom
Copy link
Contributor

mdboom commented Jan 23, 2025

It seems that #128918 caused a 2-3% slowdown on the free-threading build.

FWIW, I couldn't reproduce this on our infrastructure. I'm not claiming one is better or more reliable, only that the effect being noticed here might be more complicated...

markshannon added a commit that referenced this issue Jan 27, 2025
@markshannon
GH-128914: Remove all but one conditional stack effects (GH-129226)
75b4962 
* Remove all 'if (0)' and 'if (1)' conditional stack effects

* Use array instead of conditional for BUILD_SLICE args

* Refactor LOAD_GLOBAL to use a common conditional uop

* Remove conditional stack effects from LOAD_ATTR specializations

* Replace conditional stack effects in LOAD_ATTR with a 0 or 1 sized array.

* Remove conditional stack effects from CALL_FUNCTION_EX
@markshannon
Copy link
Member Author

Latest implementation has no adverse impact on performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mdboom @markshannon @picnixz