Skip to content

MSVC pragma to turn compiler optimization off around the _PyEval_EvalFrameDefault is no longer necessary #121338

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

Closed
mdboom opened this issue Jul 3, 2024 · 2 comments
Labels
OS-windows performance Performance or resource usage type-bug An unexpected behavior, bug, or error

Comments

@mdboom
Copy link
Contributor

mdboom commented Jul 3, 2024

Bug report

Bug description:

When the Tier 2 interpreter loop was first merged with the Tier 1 interpreter loop, the MSVC compiler would crash when building for PGO, presumably due to the very large size of the function. #111786 introduced a #pragma optimize("off") to turn off optimization around _PyEval_EvalFrameDefault which seems to have side-stepped the crash.

This crash was reported to the MSVC compiler team, but there hasn't been resolution around that.

In the intervening time, the interpreter loop has changed significantly again, and it seems that this #pragma hack is no longer necessary, either for default builds (which no longer include a Tier 2 interpreter) and --enable-experimental-jit builds (which do). I propose we remove it for the sake of removing what should have been a temporary hack and to give the compiler the fairest shot at doing its best work. Benchmarking shows no significant change for either a default build or a JIT build.

I'm not sure whether this should also be backported to 3.13. It's not really a bugfix, but I would certainly feel better shipping 3.13.0 without a weird compiler workaround (which would be harder to remove after the rc).

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 3, 2024
mdboom added a commit to mdboom/cpython that referenced this issue Jul 3, 2024
@zooba
Copy link
Member

zooba commented Jul 3, 2024

I'd support backporting.

It's not clear that it has any effect on PGO'd code (which usually ignores optimisation preferences like /Ot and /Os), but it's still a really bad look to have contributors complaining about MSVC perf when we also explicitly disable optimisations. Let's pretend we never turned it off :)

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 8, 2024
(cherry picked from commit d69529d)

Co-authored-by: Michael Droettboom <[email protected]>
mdboom added a commit that referenced this issue Jul 8, 2024
gh-121338: Remove GH-pragma optimize (GH-121340)
(cherry picked from commit d69529d)

Co-authored-by: Michael Droettboom <[email protected]>
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
@hugovk
Copy link
Member

hugovk commented Aug 8, 2024

Triage: closing because the linked PRs are merged, please re-open if still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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