Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Fidget-Spinner merged 1 commit intopython:mainfromFidget-Spinner:macro-ify
Jul 3, 2024

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-SpinnerFidget-Spinner commentedJul 2, 2024
edited by bedevere-appbot
Loading

@Fidget-Spinner
Copy link
MemberAuthor

cc@mdboom

@mdboom
Copy link
Contributor

Kicking off a benchmarking run now...

@mdboom
Copy link
Contributor

mdboom commentedJul 2, 2024
edited
Loading

Theraw 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 apragma "hack" to disable optimization on MSVC for the main eval loop. This wasintroduced 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 theyno 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
MemberAuthor

Fidget-Spinner commentedJul 3, 2024
edited
Loading

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-SpinnerFidget-Spinner merged commit722229e intopython:mainJul 3, 2024
@Fidget-SpinnerFidget-Spinner deleted the macro-ify branchJuly 3, 2024 09:49
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull requestJul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull requestJul 17, 2024
mdboom added a commit to mdboom/cpython that referenced this pull requestJul 19, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Fidget-Spinner@mdboom

[8]ページ先頭

©2009-2026 Movatter.jp