gh-146073: Add fitness/exit quality mechanism for JIT trace frontend#148089
gh-146073: Add fitness/exit quality mechanism for JIT trace frontend#148089cocolato wants to merge 23 commits intopython:mainfrom
Conversation
cocolato
requested review from
FFY00,
Fidget-Spinner,
ZeroIntensity,
ericsnowcurrently and
markshannon
as code owners
This comment was marked as outdated.
This comment was marked as outdated.
|
It appears that the current parameters do not yet guarantee runtime safety; I will continue to work on fixes and optimizations. |
|
I've commented on the issue #146073 (comment) |
This comment was marked as outdated.
This comment was marked as outdated.
|
@markshannon @Fidget-Spinner gentle ping, I’d like to hear your suggestions about the current parameters |
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
I've a few comments, mostly broad ideas and suggestions, rather than anything that needs fixing.
I think we should merge this soon. We can tweak the parameters later as we refine our ideas.
|
|
||
| /* Exit quality thresholds: trace stops when fitness < exit_quality. | ||
| * Higher = trace is more willing to stop here. */ | ||
| #define EXIT_QUALITY_CLOSE_LOOP (FITNESS_INITIAL / 2) |
There was a problem hiding this comment.
I think we want this higher, close to FITNESS_INITIAL*0.9. It is only super-short loops that we want to unroll.
| * Higher = trace is more willing to stop here. */ | ||
| #define EXIT_QUALITY_CLOSE_LOOP (FITNESS_INITIAL / 2) | ||
| #define EXIT_QUALITY_ENTER_EXECUTOR (FITNESS_INITIAL * 3 / 8) | ||
| #define EXIT_QUALITY_DEFAULT (FITNESS_INITIAL / 8) |
There was a problem hiding this comment.
I'd increase this to make sure that the fitness cannot drop from above EXIT_QUALITY_DEFAULT to below EXIT_QUALITY_SPECIALIZABLE in a single uop.
| * N_BACKWARD_SLACK more bytecodes before reaching EXIT_QUALITY_CLOSE_LOOP, | ||
| * based on AVG_SLOTS_PER_INSTRUCTION. */ | ||
| #define N_BACKWARD_SLACK 50 | ||
| #define FITNESS_BACKWARD_EDGE (FITNESS_INITIAL - EXIT_QUALITY_CLOSE_LOOP \ |
There was a problem hiding this comment.
A backwards edge isn't necessarily bad, although too many suggests a poor trace.
Maybe don't penalize the first backwards edge much, but penalize subsequent ones a lot.
What we really want is to avoid unrolling a loop that doesn't include the trace start.
It would be easier to reason about, if we used a simpler calculation for the value.
|
|
||
| /* Backward edge penalty for JUMP_BACKWARD_NO_INTERRUPT (coroutines/yield-from). | ||
| * Smaller than FITNESS_BACKWARD_EDGE since these loops are very short. */ | ||
| #define FITNESS_BACKWARD_EDGE_COROUTINE (FITNESS_BACKWARD_EDGE / 4) |
There was a problem hiding this comment.
It is not the length of the loop that matter. We want to include yields in generators in the traces of the loop that calls them, whether that is a yield from loop or a for loop doesn't much matter.
| tracer->prev_state.instr_oparg = oparg; | ||
| tracer->prev_state.instr_stacklevel = PyStackRef_IsNone(frame->f_executable) ? 2 : STACK_LEVEL(); | ||
| if (_PyOpcode_Caches[_PyOpcode_Deopt[opcode]]) { | ||
| if (_PyOpcode_Caches[_PyOpcode_Deopt[opcode]] |
There was a problem hiding this comment.
Can we use a lookup table to avoid the long chain of tests?
| assert(curr_instr->op.code == JUMP_BACKWARD_JIT || curr_instr->op.code == RESUME_CHECK_JIT || (exit != NULL)); | ||
| tracer->initial_state.jump_backward_instr = curr_instr; | ||
|
|
||
| // Reduce side-trace fitness as chain depth grows, but clamp the reduction |
There was a problem hiding this comment.
This is probably fine for now, but reducing the fitness can skew some of our assumptions about back edges and such.
One other thing to note for the future, is that the fitness at an exit might help us pick a better warmup value for that exit.
2 participants
Reopen: #147966