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

gh-121263: Macro-ify most stackref functions for MSVC #121270

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jul 2, 2024

@Fidget-Spinner
Copy link
Member Author

cc @mdboom

@mdboom
Copy link
Contributor

mdboom commented Jul 2, 2024

Kicking off a benchmarking run now...

@mdboom
Copy link
Contributor

mdboom commented Jul 2, 2024

The raw benchmaring results are here.

This PR makes things 5% faster on 64-bit Windows, 7% faster on 32-bit Windows, and no measurable change on x86_64 Linux.

The important comparison, however, is whether this change counteracts all of the negative effects of #118450. Comparing directly to commit d611c4c (the commit immediately before #118450 landed), shows the same amount of speedup 5% faster on 64-bit Windows, 7% faster on 32-bit Windows, and no measurable change on x86_64 Linux.

So this PR is effective and seems to make up for the regression.

However... if we could hold off on merging it until we confirm one other thing. @zooba reminded me that we still have a pragma "hack" to disable optimization on MSVC for the main eval loop. This was introduced to work around a compiler crash when the main eval loop function got extremely large (when the Tier 2 and Tier 2 interpreters were merged together). Of course, this hack is no longer needed for non-JIT builds, since they no longer include the Tier 2 interpreter. I am doing another benchmarking run just removing the hack to see if it also fixes the performance issue -- if we can do that and use inline functions rather than macros, that seems preferable.

EDIT: Added link to change that removed Tier 2 interpreter from default builds.

@Fidget-Spinner
Copy link
Member Author

Fidget-Spinner commented Jul 3, 2024

Thanks Mike for the attempt! Sadly, removing the pragma doesn't seem to speed up anything:
https://github.com/faster-cpython/benchmarking-public/tree/main/results/bm-20240702-3.14.0a0-a03affd

As the reported benchmark results are good, and this PR seems to make up for all the perf loss, I will merge this PR, to restore performance of Windows builds.

@Fidget-Spinner Fidget-Spinner merged commit 722229e into python:main Jul 3, 2024
36 checks passed
@Fidget-Spinner Fidget-Spinner deleted the macro-ify branch July 3, 2024 09:49
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
mdboom added a commit to mdboom/cpython that referenced this pull request Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants