gh-146073: Add fitness/exit quality mechanism for JIT trace frontend

[cocolato]

Background

See: #146073

Introduced a preliminary fitness/exit quality mechanism for JIT trace frontend has, enabling the tracer to:

• Stop proactively: Stop at the appropriate time without waiting for the buffer to fill up or hitting a dead end
• Stop at good locations: Prioritize stopping at the entry points of existing executors (ENTER_EXECUTOR) to avoid stopping on instructions that can be optimized
• Control frame depth: Apply a penalty that increases with depth for function call inlining

• Issue: Improving trace quality by tracking "fitness" and "exit quality" #146073

[Fidget-Spinner]

Great work. Thanks for doing this!

[Fidget-Spinner]

I also forgot to mention: but we should adjust for the following:

1. Penalize frame depth underflow more so than normal frame push/pop.
2. Stop tracing when we hit MAX_ABSTRACT_FRAME_DEPTH, as we can't optimize it anyways.

[markshannon]

This looks good, and thanks for doing this.

Overall, the approach looks good.

I originally had in a mind that the fitness would reduce geometrically, not arithmetically, but your arithmetic approach looks easier to reason about and at least as good in terms of trace quality.

We need to reduce the number of configurable parameters to just one or two.
Then we can make sure that fitness and penalties are set such that we are guaranteed not to overflow the trace or optimizer buffers.

I'm not sure that rewinding is worth it. As long as "good" exits have a much higher score than "bad" exits, then we should (almost) always end up at a good exit.

[markshannon]

I'd stick with 2000. We really don't need traces of over 1000 uops; it is far more important to find a good stopping point than to have very long traces

[Fidget-Spinner]

@markshannon 1000 uops is only roughly 300 uops after optimization which corresponds to roughly only 60 bytecode instructions. That's too low. A reminder that the trace recording spews out a lot of uops that is only optimized away by the optimizer.

[Fidget-Spinner]

@cocolato I suspect richards perf regression might be due to too low trace length, I experimented with richards before and it's quite sensitive to how well the optimizer performs

[cocolato]

Yes, and I tried reducing the branch/frame penalty, which also improve richards' performance.

[markshannon]

Suggested change

	assert (uop_buffer_remaining_space(trace) >space_needed);

If we choose the fitness and exit values correctly, we can't run out of space.

[cocolato]

I'm not sure if the current parameters allow us to remove the check here, so I think it's better to keep it for now.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[cocolato]

I’ll try to optimize the relevant parameters further, as performance in benchmarks like Richards has declined, and I’ll add some debug logs to make it easier to track fitness-related metrics later on.

[markshannon]

It might be worth thinking about a few scenarios to help choose parameters.
For example:

• A loop should only be unrolled once, but we do want to unroll short loops.
• There shouldn't be at most ~4 unbiased branches in a trace

[cocolato]

The newest result with lower frame penalty:

+----------------------+----------+-----------------------+
| Benchmark | baseline | fitness |
+======================+==========+=======================+
| raytrace | 262 ms | 212 ms: 1.24x faster |
+----------------------+----------+-----------------------+
| pickle_pure_python | 277 us | 254 us: 1.09x faster |
+----------------------+----------+-----------------------+
| go | 83.7 ms | 78.8 ms: 1.06x faster |
+----------------------+----------+-----------------------+
| xml_etree_iterparse | 74.1 ms | 69.9 ms: 1.06x faster |
+----------------------+----------+-----------------------+
| xml_etree_process | 50.4 ms | 48.3 ms: 1.04x faster |
+----------------------+----------+-----------------------+
| xml_etree_generate | 77.3 ms | 74.1 ms: 1.04x faster |
+----------------------+----------+-----------------------+
| xml_etree_parse | 122 ms | 119 ms: 1.03x faster |
+----------------------+----------+-----------------------+
| regex_compile | 103 ms | 99.7 ms: 1.03x faster |
+----------------------+----------+-----------------------+
| deltablue | 2.19 ms | 2.14 ms: 1.03x faster |
+----------------------+----------+-----------------------+
| unpickle_pure_python | 171 us | 167 us: 1.02x faster |
+----------------------+----------+-----------------------+
| regex_effbot | 2.16 ms | 2.12 ms: 1.02x faster |
+----------------------+----------+-----------------------+
| fannkuch | 253 ms | 249 ms: 1.02x faster |
+----------------------+----------+-----------------------+
| json_loads | 19.3 us | 19.1 us: 1.01x faster |
+----------------------+----------+-----------------------+
| json_dumps | 7.60 ms | 7.66 ms: 1.01x slower |
+----------------------+----------+-----------------------+
| pidigits | 136 ms | 138 ms: 1.01x slower |
+----------------------+----------+-----------------------+
| pyflate | 267 ms | 273 ms: 1.02x slower |
+----------------------+----------+-----------------------+
| float | 45.2 ms | 46.2 ms: 1.02x slower |
+----------------------+----------+-----------------------+
| richards | 16.5 ms | 17.3 ms: 1.05x slower |
+----------------------+----------+-----------------------+
| Geometric mean | (ref) | 1.03x faster |
+----------------------+----------+-----------------------+

[bedevere-app]

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

[cocolato]

Thank you all for taking the time to review this! Do the current parameters meet our expectations?

Changes have been addressed. Any minor tweaks can be done as follow-up PRs. Thanks Mark!

[Fidget-Spinner]

I'll take responsibility for this PR and fix/revert anything if it breaks stuff. Thanks again Hai Zhu and Mark for the reviews.

Any minor tweaks can be follow-up PRs.

[Fidget-Spinner]

I see a slight (~8%) slowdown in Richards on my system. However, I think that's acceptable considering most other things sped up.

[markshannon]

I don't think this should have been merged, for a couple of reasons:

• The choice of parameters is driven by a few benchmarks. There seem to be as many parameters as benchmarks that are sensitive to those benchmarks, so over-fitting is inevitable. In general, we should be using the benchmarks to verify design, not drive it.
• This PR should simplify the code, not make it more complicated. There are several special cases in the tracer than using fitness and exit quality should eliminate, and this PR eliminates none of them.

[markshannon]

I see a slight (~8%) slowdown in Richards on my system. However, I think that's acceptable considering most other things sped up.

How is an 8% slowdown "slight"?
It may be acceptable, given the other speedups, but we should know why it happened before merging.

[Fidget-Spinner]

Ok, I'll revert and re-open the PR. Sorry for the trouble @cocolato

[Fidget-Spinner]

@cocolato and @markshannon I used sys._dump_tracelets("hello.gvz") to see the output of richards.
Before this PR:

After this PR:

Tweaking FITNESS_INITIAL to a higher number (FITNESS_PER_INSTRUCTION * UOP_MAX_TRACE_LENGTH) seems to indeed restore some of the original trace's characteristics. So I think I was right to say that the current initial value is stoo low.

[markshannon]

Yes, that does look like the new traces are too short.
The trace before this PR is only ~70 uops, so less than 200 uops before optimization. Which suggests that it isn't necessary a low initial fitness that is causing the trace to be too short. Something clearly is, though.

I also note that neither manages to close any loops, either within a trace or across traces, which surprises me a bit.

[cocolato]

Sorry about the revert. I’ll I do more detailed analysis and optimization later.

[markshannon]

Sorry about the revert. I’ll I do more detailed analysis and optimization later.

Not a problem, and thanks for working on this.