gh-111786: Turn off optimize for speed for _PyEval_EvalFrameDefault on MSVC

[mdboom]

• Issue: PGO build broken on Windows #111786

[gvanrossum]

LGTM. @markshannon or @brandtbucher can merge if they agree.

[markshannon]

3-10% faster and fixes the build. Sounds good to me 🙂
I'd like @zooba's approval though, in case we missed something.

[zooba]

My only question would be whether we know that turning t off implicitly turns s (size) on, or if it leaves us with no optimisations at all.

The results seem to be okay for PGO builds, but regular builds may suffer a regression if optimisation is just turned off. (For debug builds we have it off deliberately, and would want to avoid turning it on.)

[mdboom]

Converting to draft -- I'm seeing some non-determinacy to this fix, so we should hold off on merging until I get to the bottom of it.

[brandtbucher]

I see this has been re-opened. Did you figure out the issue?

If so, it looks like it's ready to merge (assuming we aren't seeing a regression on non-PGO builds).

[mdboom]

My only question would be whether we know that turning t off implicitly turns s (size) on, or if it leaves us with no optimisations at all.

I couldn't find a way to directly introspect this. However, I decided to try to forcibly turn "s" on after turning "t" off and compare the results to this PR, e.g.:

# pragma optimize("t", off) # pragma optimize("s", on) 

Using SizeBench, I was able to confirm that both that and this PR generate the same size code for _PyEval_EvalFrameDefault (it's in two blocks: 37,951 bytes of hot code and 28,104 bytes of cold code).

So that (might be) indirect evidence at least that we still get size optimization this way.

[mdboom]

I see this has been re-opened. Did you figure out the issue?

Yes, it was entirely me not seeing the difference between optimize("", on) and optimize("", "on"). (I blame insufficient coffee).

If so, it looks like it's ready to merge (assuming we aren't seeing a regression on non-PGO builds).

I just fired off some benchmarking runs to test the effect of this on non-PGO builds. It will take a few hours to get the results back.

[brandtbucher]

Yes, it was entirely me not seeing the difference between optimize("", on) and optimize("", "on"). (I blame insufficient coffee).

I blame "that's a rough API design".

[zooba]

So that (might be) indirect evidence at least that we still get size optimization this way.

Unoptimised code will be huge by comparison - 2x or more. Sounds like we're still getting the optimization.

[mdboom]

Unoptimised code will be huge by comparison - 2x or more. Sounds like we're still getting the optimization.

And some level of PGO is still happening, as evidenced by the split into hot and cold paths.

If so, it looks like it's ready to merge (assuming we aren't seeing a regression on non-PGO builds).

Unfortunately, it seems this causes an 11% regression on non-PGO builds. (This is comparing non-PGO or this PR vs. a non-PGO build of its base 9f33ede).

If we care about this (I'm not sure we do -- that's a question for someone with more knowledge of the history of Windows builds). We can probably disable this #pragma when doing a non-PGO build, but I don't see an obvious way to do it. I don't think the compiler gives us a preprocessor variable with the info (at least I can't find one), so we'd probably have to inject a variable from the build system. Anyway, that's a problem for tomorrow for me.

[zooba]

FYI, pyproject.props is where you'll want to add it. You'll see examples of conditional PreprocessorDefinition for Py_NOGIL, and the condition should be Condition="$(SupportPGO) and ($(Configuration) == 'PGInstrument' or $(Configuration) == 'PGUpdate')".

I'd suggest _Py_USING_PGO as a name, unless there's something already being used via configure, in which case copy that.

[mdboom]

I have updated this to only disable the optimization when doing a PGO build.

@zooba's suggestion of only doing this during the PGInstrument and PGUpdate phases doesn't work. It's actually the final Release phase that fails, and having them behave differently reintroduces the original compiler error. So in this case _Py_USING_PGO is true for all of the phases of a PGO build.

EDIT: I take that back -- with a clean build this works, and is simpler.

[gvanrossum]

LGTM. Let me know when you're ready for this to be merged.

[mdboom]

@gvanrossum: As far as I know this is ready, but maybe @zooba should have one more quick look.

[gvanrossum]

I'm just going to merge it, it looks great.