jit: apply every elapsed counter-decay interval, and drain it before every table access - #1207
Conversation
…apply every elapsed interval counter.py:104-121 calls `decay_all_counters()` inside the 32nd minor collection. pyre defers it, and the deferral diverged in two ways. `tick` applied one decay however many 32-collection intervals had elapsed; it now applies one per interval, stepping the generation forward one at a time so each step rounds back to f32 the way `decay_all_counters` does. `change_current_fraction`, `reset`, `reset_all` and `set_decay` did not drain at all, so a pending decay landed on values written after the collection that scheduled it: `_trace_next_iteration` (warmstate.py:617-619) writes 0.98 and the next `tick` turned it into 0.98 * 0.96. All four now drain first, and `set_decay` drains before it replaces the multiplier. `would_tick_fire` takes `&self` and answers from a decay-adjusted read, stepping the same number of generations so it agrees with the `tick` that would drain them. Assisted-by: Claude
…ct two stale notes
The decay drain lands one more decay interval per counter than before, so
two synthetic fixtures settle on different counters:
inline_chain_depth_typeflip dynasm+cranelift bridges_compiled 19 -> 18,
guard_failures 3820 -> 3681
exception_traceback_loop_forms wasm guard_failures 810 -> 811
Re-recorded from a full local gate at 4a391053e79 (dynasm 423/425,
cranelift 424/425, wasm 417/418; the remaining dynasm failure is the
intermittent minor_remembered_set GC panic, which fires on a different
bench each full run).
check.py's baseline-resolution note claimed a fresh local build reports
the ubuntu str_fstring pair. It reports the macos-latest pair: two full
local gates pass against the plain `.darwin` overlays holding dynasm 659
and cranelift 658. The darwin GitHub-runner overlay for cranelift held
the same value as the platform-wide one, so it could never be reached
and is removed.
counter.rs described the deferred decay as avoiding a borrow the GC does
not hold. The accessor for the JIT_DRIVER cell mints a
`&'static mut JitDriverPair`, and a minor collection can be triggered by
an allocation the metainterp makes while already holding one, so
decaying from inside the collector would alias it.
Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 521efc3950
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while self.last_decay_generation != generation { | ||
| self.last_decay_generation = self.last_decay_generation.wrapping_add(1); | ||
| self.decay_all_counters(); |
There was a problem hiding this comment.
When one JIT thread remains idle while another thread performs many minor collections, the process-global generation can advance arbitrarily far beyond this counter's per-thread generation. Its next table access then performs one full 2,048-entry scan per elapsed interval in a single foreground operation; with decay=0, every scan is a no-op, and after values have underflowed to zero further scans are likewise unnecessary. Long-running allocation-heavy processes can therefore incur an unbounded pause when an idle thread resumes. Preserve the per-interval rounding while skipping intervals once they cannot change the table (and directly consume the backlog for multiplier 1.0 or an all-zero table).
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 521efc3). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
1 participant
Follow-up to the #1188 review. Two review findings on
JitCounter's deferred decay turned out to be the same defect, so they are fixed together.The defect
counter.py:104-121decays inside the minor collection, so a value written after a collection is never decayed by it. pyre defers the decay to the next table access, and the deferral was visible in two ways:boolsentinel recorded "a decay is pending", so 32 collections and 320 collections cost the same singledecay_all_counters(). Upstream applies one per 32-collection interval.change_current_fractionwritesentry.times[0] = 0.98—trace_next_iteration(warmstate.py:617-619) uses it to schedule tracing on the next iteration. It did not drain the pending decay first, so the nexttickdecayed the fresh 0.98 down to 0.9408. Upstream cannot do this: the decay already happened at collection time.The fix
DECAY_PENDING: AtomicBoolbecomesDECAY_GENERATION: AtomicUsize, and eachJitCountertracks the last generation it applied.apply_pending_decaysteps the generation forward one interval at a time, decaying once per step, and runs first intick,change_current_fraction,reset,reset_allandset_decay— the last before the multiplier is replaced, so the pending intervals use the multiplier that was in force.The generation is per-counter rather than global because
JIT_DRIVERineval.rsis thread-local, so each mutator thread has its ownJitCounter; one thread'stickmust not consume another's decay.Stepping one interval at a time is deliberate:
decay_all_countersrounds back tof32after every step, somult^Napplied once differs in the last bits from N applications, and upstream performs N.would_tick_fireis&selfand cannot drain, so it decay-adjusts its read the same way — the predicate must not disagree with thetickthat drains the same generations.The deferral itself stays.
driver_pair()mints a&'static mut JitDriverPairfrom theJIT_DRIVERcell, and a minor collection can be triggered by an allocation the metainterp makes while already holding one, so decaying from inside the collector would alias it. Thecounter.rscomment that described this as a borrow the GC does not hold is corrected.Baselines
Counters now cool at the upstream rate, so three fixtures settle differently. Re-recorded from a full local gate:
inline_chain_depth_typeflipbridges_compiled19 → 18,guard_failures3820 → 3681exception_traceback_loop_formsguard_failures810 → 811loops_compiledis unchanged at 6, and both native backends move to the same pair — the compile shape is the same, the counter schedule is not.check.py's baseline-resolution note
It claimed a fresh local build reports the ubuntu
str_fstringpair. It reports the macos-latest pair: two full local gates pass against the plain.darwinoverlays holding dynasm 659 and cranelift 658. The darwin GitHub-runner overlay for cranelift held the same value as the platform-wide one, so it could never be reached and is removed.Verification
Full local darwin gate at the fix's base: dynasm 423/425, cranelift 424/425, wasm 417/418, with the three rows above accounting for the jit-stats failures. The remaining dynasm failure is the intermittent
minor_remembered_setGC panic, which fires on a different bench each full run and is unrelated.cargo test -p majit-trace64 passed.The gate ran before the two comment corrections, which are comment-only, and the branch has since been rebased onto current main.
— authored by Claude