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

Remove #pragma that disables optimization on _PyEval_EvalFrameDefault on MSVC #129244

Open
mdboom opened this issue Jan 23, 2025 · 8 comments
Open
Assignees
Labels
deferred-blocker interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-windows performance Performance or resource usage type-feature A feature request or enhancement

Comments

@mdboom
Copy link
Contributor

mdboom commented Jan 23, 2025

Feature or enhancement

Proposal:

The bug that caused a compiler crash has now been fixed upstream. We should remove our workaround to disable optimization on PGO builds on MSVC (and presumably get a modest speedup).

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

Linked PRs

@mdboom mdboom added the type-feature A feature request or enhancement label Jan 23, 2025
@mdboom mdboom self-assigned this Jan 23, 2025
@picnixz picnixz added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jan 24, 2025
@encukou encukou added the performance Performance or resource usage label Jan 24, 2025
mdboom added a commit that referenced this issue Jan 25, 2025
* Remove compiler workaround

* Remote _Py_USING_PGO
@itamaro
Copy link
Contributor

itamaro commented Jan 25, 2025

Looks like this is done, closing.

@ambv
Copy link
Contributor

ambv commented Feb 11, 2025

This will have to be reverted for now as the 3.14.0a5 build is failing.

@ambv
Copy link
Contributor

ambv commented Feb 11, 2025

See #130004.

@ambv
Copy link
Contributor

ambv commented Feb 11, 2025

As we are using the Azure Pipelines VMs to build, we are not controlling the version of the compiler and so far the version available on the image we're using (2025020.1.0 with Visual Studio 2022 17.12.12+1cce77968) there does not have the fix.

@mdboom do you know which VS version would be sufficient to reintroduce the change from this ticket?

@mdboom
Copy link
Contributor Author

mdboom commented Feb 11, 2025

The minimum version of cl.exe required is 19.43.34618, which is currently in prerelease. I was under the (wrong) impression that this would be backported to the maintenance branch, which is why I submitted the PR. We can add a version check now so that when this does eventually make it into a primary release, we will be ready.

@Fidget-Spinner
Copy link
Member

Marking as deferred blocker so we don't forget to fix this by 3.14 final.

@chris-eibl
Copy link
Contributor

Upgrading to Visual Studio 2022 17.13.0 Preview 5.0, which ships with cl.exe 19.43.34808, works for me.
I had those issues on 17.12.4 and 17.12.3, too.

@encukou
Copy link
Member

encukou commented Feb 17, 2025

With #130011 merged, can this be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deferred-blocker interpreter-core (Objects, Python, Grammar, and Parser dirs) OS-windows performance Performance or resource usage type-feature A feature request or enhancement
Projects
Development

No branches or pull requests

7 participants