Repository navigation
jit: lower the empty dict literal, drop unlowered loop-header trace entries, decline frameless traceback nodes - #803
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (5)
WalkthroughThe PR adjusts JIT traceback emission fallback, avoids panics on missing merge entries, changes empty-dictionary and trace-entry lowering, and adds benchmarks for traceback shapes and loop-exit local handling. ChangesJIT execution and regression benchmarks
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 5347220a00
ℹ️ 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".
| // opaque inline hook fabricates a frame from the promoted callee | ||
| // metadata, while a null one answers None and breaks every consumer | ||
| // that follows `tb_frame.f_code` — `traceback.print_exc` among them. | ||
| return false; |
There was a problem hiding this comment.
When portal_frame_reg is unseeded, this decline still violates traceback semantics: the inline fallback in record_inline_traceback_for_recording constructs a new frame with no live locals or original identity, while the top-level fallback in record_top_level_application_traceback also rejects the missing frame and emits no node. Consequently, code that catches and exposes an exception from a frameless compiled walk can observe the wrong tb_frame/locals or a shortened traceback; thread the call's real red frame through the walk rather than accepting either fallback.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 19a0d74). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
`Instruction::BuildMap` with count 0 emitted `abort_permanent` rather than taking the `new_array_clear` + `build_map_from_array` lowering every other count takes. `BuildSet` already lowers count 0 through the same array path. `emit_abort_permanent!` links the block to the returnblock, so the decline also stopped the rest of the function from being lowered. Assisted-by: Claude
`merge_entry_by_green` took every pc `find_loop_header_pcs` found, a bytecode scan independent of what was emitted. For a header behind an `abort_permanent` nothing at or after it emits an op, so `derive_resume_marker` answers `None` and the dense carry-forward's value — the last `-live-` of the emitted prefix — diverges into `carryfwd_resume_pc` and resolves as that header's walk entry. `run_perfn_walk` seeds an entry marker's abstract registers from the live frame by slot. Entering the emitted prefix with the frame parked at the loop header filled the prefix's registers with the header's operand stack, and replaying the prefix stored them into locals; the abort flush then committed that to the frame. A `for` header's live stack of 1 matches a prologue marker's depth of 1, so the existing stack-depth check let it through — the `while` spelling was rejected by that check. Filter the headers on `derive_resume_marker`, and let `compile_and_run_once` interpret a green with no static merge entry instead of asserting one exists. Assisted-by: Claude
…o frame box `record_fresh_application_traceback` substituted `const_ref(0)` for a `TracebackNodeSite` whose frame register was unseeded, emitting a node whose `tb_frame` answers None. `traceback.print_exc`, `format_exception` and `extract_tb` all read `tb.tb_frame.f_code`, so such a node raises AttributeError; the recording pass meanwhile attaches a node with a real frame, because the opaque hook fabricates one from the promoted callee metadata. Return false there, as `record_prepend_application_traceback` already does, so the caller keeps the hook. The two `raise/r` legs now differ only in which recorder runs. Only the missing frame box falls back. The other `traceback_node_site` declines must stay silent: the `w_code` sentinel and non-code cases would have the hook dereference `w_code` through `createframe_obj`, and a hidden_applevel frame contributes no node at all. Also correct the effect-info the doc comment claims for the opaque hooks — `default_effect_info()` is `EffectInfo::MOST_GENERAL`, not `CanRaise`. Assisted-by: Claude
…rame consequences Both application-traceback recorders decline when the walk holds no frame box for the level. Record what that costs at each site: at top level `record_top_level_application_traceback` resolves its frame operand from the same unseeded color and declines as well, so no node is contributed; below it the opaque hook builds a frame from the callee's code and globals, which carries no locals and its own identity. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a0d74b8d
ℹ️ 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".
| if pjc.merge_entry_for(target_pc).is_none() { | ||
| return None; |
There was a problem hiding this comment.
When an abort_permanent precedes a hot loop, this returns after counter_tick_checked has fired and reset the hot counter, but it does not mark the green as DONT_TRACE_HERE or otherwise record a permanent decline. The interpreter therefore counts another threshold's worth of backedges and repeatedly re-enters compile_and_run_once and portal registration even though the missing entry is deterministic; with a low configured threshold this can put every iteration on the cold compilation path. Permanently decline this green when its static merge entry is absent.
Useful? React with 👍 / 👎.
Three defects plus the documentation follow-up from the #790 review.
A local read back as the
forloop's iterator (pre-existing, both backends)Silent wrong answer once the loop crosses the compile threshold;
PYRE_JIT=0is clean. The chain:BuildMapdeclinedcount == 0and the decline emittedabort_permanent.emit_abort_permanent!appends a returnblock link, soblock_closed_by_terminatorholds for every later PC and the rest of the function is never lowered — 127 bytes of jitcode versus 915 for thedict()twin, and nojit_merge_pointat all.trace_entry_pcswas a pure bytecode scan, so the unlowered loop header was still published as a walk entry.derive_resume_markeranswersNonepast the truncation, so the dense carry-forward pointed that header at the prologue's last-live-.run_perfn_walkaccepted the entry (its only sanity check compares operand-stack depth, and aforheader's live stack of 1 matches the prologue marker's), then seeded the prologue's registers from the live frame BY SLOT while the frame was parked at the header. The iterator landed in a register and the replayed prologueSTORE_FASTpublished it into a local.Two commits, because either alone is insufficient:
347c27db23lowers{}through the samenew_array_clear+build_map_from_arraypath as every other count, exactly likeBuildSet.cbddf590e4filterstrace_entry_pcsonderive_resume_marker(..).is_some()and makescompile_and_run_onceinterpret instead ofexpecting a merge entry. Any otherabort_permanentahead of a hotforloop reproduces the identical corruption — confirmed withmatch seq: case [_, _]:.Bench:
pyre/bench/synth/loop_exit_empty_dict_local_clobber.py.A traceback node with a null
tb_frame(#790 review, P2)d9dd30cda5.record_fresh_application_tracebackfilled an unseeded frame color withconst_ref(0).traceback.print_exc,format_exceptionandextract_tball readtb.tb_frame.f_code, so such a node raisesAttributeError; meanwhile the recording pass attached a node with a real frame, so recording and compiled runs diverged. It now returnsbooland declines, matching itsrecord_prepend_application_tracebacksibling, and the caller keeps the opaque hook — which fabricates a frame from the callee's code and globals. Only the missing frame box falls back; thew_codesentinel / non-code declines stay silent (the hook dereferencesw_codethroughcreateframe_obj) and so doeshidden_applevel.The two other findings in that review were verified and rejected: the "walk-catch arm double-records a concrete node" reading is inverted — the base guard
matches!(op.opname, "raise/r" | …)is dead code becauseDecodedOp.opnameholds only the part before the slash, so base ran both calls too, andrecord_fresh_application_tracebackperforms no concrete record at all. Reinstating that guard would delete the recording iteration's only node for a same-frame raise.hidden_applevelis honoured one layer down inrecord_application_tracebackfor both hooks.pyre/bench/synth/exception_raise_caught_same_frame_tb.pylocks the node count.5347220a00records what the decline costs, from the parity review of this branch: at top level the sibling hook resolves its frame operand from the same unseeded color and declines too, so that level contributes no node, and the fabricated frame carries no locals and its own identity.Verification
check.pydynasm 323/323, cranelift 323/323 (run sequentially)cargo test --workspace --features dynasm— 7082 passed, 0 failedcargo fmt --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests