Uh oh!
There was an error while loading. Please reload this page.
gh-131798: Support generator frames in the JIT optimizer - #143340
Conversation
markshannon
left a comment
There was a problem hiding this comment.
The changes to optimizer_bytecodes.c look good.
This PR also does loop peeling.
We should probably do that, but in another PR where we can assess its merits and when to peel, and when not.
Uh oh!
There was an error while loading. Please reload this page.
When you're done making the requested changes, leave the comment: |
Fidget-Spinner
commented
Jan 5, 2026
No this doesn't peel. The problem is that generators have a JUMP_BACKWARD in them. If we don't do this, the generators are not traced through and just end early. In fact, none of the generator tests form a loop or pass if I don't add this change. So to test generators, I need this in this PR. |
Fidget-Spinner
commented
Jan 5, 2026
Note that we still close a loop when we see it in one iteration, not a peeled iteration. This only affects tracing inner loops, such as when tracing through a generator, or a double nested for loop. In those cases, the inner loop is now traced one more iteration, which is what we want for generators. |
markshannon
commented
Jan 5, 2026
Going through a backward jump is equivalent to loop peeling. Even if the loops are strangely shaped. I don't suppose you have stats for this? |
Fidget-Spinner
commented
Jan 5, 2026
No I do not. |
How can you tell the impact of allowing the extra backward edge in the trace? |
Fidget-Spinner
commented
Jan 5, 2026
For this example: Before this PR dumps: After this PR: The trace is significantly improved. |
Fidget-Spinner
commented
Jan 5, 2026
In the above example, the trace is wrongly cut short because of the JUMP_BACKWARD caused by the inner generator. |
I patched main with just the changes to optimizer_bytecodes.c, but not the change to JUMP_BACKWARD handling, and I get this pair of traces (running for a bit longer to warm the side exit): Which covers the same code as the single trace in your example. So, we don't need the changes to |
Fidget-Spinner
commented
Jan 5, 2026
@markshannon removed the changes and limited it only to the optimizer. |
markshannon
commented
Jan 6, 2026
You'll need to update the test, which is a bit awkward as there are two executors to check if you're looking for the We really need a proper test framework for the optimizer. |
Fidget-Spinner
commented
Jan 6, 2026
@markshannon test fixed and verified on my machine. |
Android build failure looks unrelated |
Uh oh!
There was an error while loading. Please reload this page.
This PR covers the iteration of generators in the JIT optimizer.
Benchmark results: 1.5% faster on AArch64 macOS https://github.com/facebookexperimental/free-threading-benchmarking/blob/main/results/bm-20260102-3.15.0a3%2B-8f7b4f4-JIT/bm-20260102-macm4pro-arm64-Fidget%252dSpinner-cover_more_frames-3.15.0a3%2B-8f7b4f4-vs-base.md
0% faster on x86-64 Linux. https://github.com/facebookexperimental/free-threading-benchmarking/blob/main/results/bm-20260102-3.15.0a3%2B-8f7b4f4-JIT/bm-20260102-vultr-x86_64-Fidget%252dSpinner-cover_more_frames-3.15.0a3%2B-8f7b4f4-vs-base.md
I think the macOS results are a little suspicious, maybe the baseline was when the machine was too hot or something? No clue.