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.

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

Stackrefs (#118450) introduced a large performance regression on Windows #121263

Closed
mdboom opened this issue Jul 2, 2024 · 2 comments
Closed
Labels
3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-windows performance Performance or resource usage type-bug An unexpected behavior, bug, or error

Comments

@mdboom
Copy link
Contributor

mdboom commented Jul 2, 2024

Bug description:

#118450 "Convert the evaluation stack to stack refs" (by @Fidget-Spinner) unfortunately seems to have introduced a 7% - 11% performance regression overall (though as much as 25% for some benchmarks) on Windows only. Linux (both x86_64 and aarch64) and macOS (arm) seem to have a change beneath the noise threshold. To be clear, @Fidget-Spinner's work is great and the lesson here is that for these sorts of sweeping low-level changes, we should have benchmarked on all of the Tier 1 platforms to be certain.

I don't know what the cause is -- it is probably MSVC not being able to "optimize through" something and introducing unnecessary overhead, but that's just a theory.

Cc: @brandtbucher, @markshannon

CPython versions tested on:

CPython main branch

Operating systems tested on:

Windows

Linked PRs

@mdboom mdboom added type-bug An unexpected behavior, bug, or error performance Performance or resource usage OS-windows labels Jul 2, 2024
@Fidget-Spinner
Copy link
Member

@mdboom Thanks for bringing this up! In the past this was usually due to MSVC not being able to inline in the eval loop because its too big of a function. I can change all the static inlines to macros and try benchmarking again?

@mdboom
Copy link
Contributor Author

mdboom commented Jul 2, 2024

I can change all the static inlines to macros and try benchmarking again?

Sounds like a good first thing to try. Thanks!

@Eclips4 Eclips4 added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.14 new features, bugs and security fixes labels Jul 2, 2024
Fidget-Spinner added a commit that referenced this issue Jul 3, 2024
Macro-ify most stackref functions for MSVC
@mdboom mdboom closed this as completed Jul 3, 2024
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
mdboom added a commit to mdboom/cpython that referenced this issue Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-windows performance Performance or resource usage type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants