Closed gvanrossum closed 1 year ago
brandtbucher
commented
1 year ago I've wondered the same thing. We could benchmark this on Linux pretty easily by disabling computed gotos and timing the results with and without PREDICT turned on. I imagine that would be very close to whatever the Windows speedup is (but without requiring a Windows benchmarking setup).
brandtbucher
commented
1 year ago Based on Linux builds on our benchmarking machine, disabling USE_COMPUTED_GOTOS makes things ~1% slower (I thought it would be more, but it could be that our new superinstructions do most of the heavy lifting when reducing this part of the dispatch overhead):
When USE_COMPUTED_GOTOS is turned off, disabling PREDICT doesn't appear to have any effect:
So this could be worth exploring as a cleanup opportunity (probably after observing similar MSVC results first).
gvanrossum
commented
1 year ago Agreed, this looks somewhat promising. Let's wait until we have a reliable way to benchmark this on Windows. (Didn't I read somewhere that we need 2.5% perf difference to be sure a benchmark result isn't noise?)
ericsnowcurrently
commented
1 year ago (Didn't I read somewhere that we need 2.5% perf difference to be sure a benchmark result isn't noise?)
gvanrossum
commented
1 year ago Another way to evaluate the effectiveness of PREDICT() without benchmarking: look at opcode counts and successors for opcodes that have a PREDICT() in them. We should be able to tell how often the PREDICT() is effective from that.
Separately, PREDICT() in opcodes that may be specialized aren't very effective unless the specialized versions also use PREDICT(). So we should either add PREDICT() to the specialized versions, or remove it from the base opcode.
gvanrossum
commented
1 year ago Conclusion: no need to worry about specializations here; half of these opcodes don't show up in the stats (because they're not used in any of the benchmarks); four of the others are clearly effective; MAP_ADD seems questionable. Perhaps we can just drop the PREDICT() calls from the 5 that don't show up (being very specialized opcodes) and from MAP_ADD?
earonesty
commented
1 year ago windows is far from esoteric tho! seems like it's still worth it on windows, which is a critical o/s
markshannon
commented
1 year ago One thing we could do, is strip the PREDICT() macros from bytecodes.c and automatically insert the prediction logic using data from the stats.
The current, manually inserted predictions aren't very good, so it should be easy to do better.
E.g. according to the stats, BEFORE_WITH is followed by POP_TOP about 80% of the time, STORE_FAST__LOAD_FAST the remaining 20%, and everything else some negligible fraction.
We could generate this code at the end of BEFORE_WITH:
if (opcode == POP_TOP) goto target_POP_TOP;
if (opcode == STORE_FAST__LOAD_FAST) goto target_STORE_FAST__LOAD_FAST;
GOTO_DISPATCH();(The if ... goto ... will need to wrapped in a macro, for portability)
gvanrossum
commented
1 year ago Since PREDICT() only matters on the MS compiler, maybe we could just manually insert more appropriate PREDICT() calls based on the stats? Where are the most recent stats?
markshannon
commented
1 year ago I think we should automate this. https://github.com/faster-cpython/ideas/tree/main/stats look for the one from the most recent Sunday.
markshannon
commented
1 year ago The PREDICT macros are gone.
The
PREDICT()macro is already a no-op on compilers with computed GOTO. So it's only used on Windows (and a few esoteric platforms). Is it worth the bother? We should ideally benchmark this. In the past it's been claimed to have substantial benefits.