Repository navigation
jit: mapdict instance-attr folds + two resume-journal correctness fixes - #592
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 33 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 Run ID: 📒 Files selected for processing (13)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 644971e). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
The full-body-walker LOAD_ATTR fold previously bailed on unboxed int/float slots (load_attr_fast_path returns None when p.unboxed.is_some()), so every `obj.x` on an unboxed-int attribute compiled to the forcing getattr_fn residual: ForceToken + SetfieldGc(vable) + CallMayForceR + GuardNotForced + GuardNoException plus an MRO walk. Add an unboxed fold path mirroring the boxed one: - load_attr_unboxed_fast_path (mapdict.rs): same LOAD_ATTR resolution gates as load_attr_fast_path but accepts only an UnboxedPlainAttribute, returning the (storageindex, listindex, UnboxType) coordinates. - read_unboxed_storage_raw (mapdict.rs): _prim_direct_read's raw longlong read before boxing (mapdict.py:600-601). - jit_mapdict_unboxed_read_raw (helpers.rs): non-forcing residual mirroring jit_mapdict_read; returns the raw i64. - try_walker_specialize_load_attr (jitcode_dispatch.rs): after the boxed fast path declines, emit guard_class + guard_value(map) + the non-forcing int residual, then wrapint. Float slots still decline (no Int->Float bitcast yet). The boxed wrapint result carries its heap-ptr concrete stamp via box_int_concrete so a downstream eager void residual (e.g. the still- residual STORE_ATTR) can resolve its value arg. The read is non-forcing and the box virtualizes when an integer consumer immediately unboxes it, so the vable spill and force/exception guards disappear. synth dynasm 176/176. Assisted-by: Claude
An `obj.x = obj.x + 1` unboxed-int hot loop still compiled STORE_ATTR to the forcing setattr residual: ForceToken + SetfieldGc(vable) + NewWithVtable (re-box the value) + SetfieldGc(box) + CallMayForceN(setattr_fn) + GuardNotForced + GuardNoException. setattr forces because it may run __setattr__ / a data descriptor and raise; for a plain instance whose map and version_tag are guarded, with the attribute already present as an unboxed int slot and the incoming value the same type, the write is a pure in-place list[listindex] = raw that cannot force or raise. Fold that case to a non-forcing raw write: - PyreHelperKind::StoreAttr (effectinfo.rs) + tag the setattr residual with it (flatten.rs lower_setattr). - store_attr_unboxed_fast_path + write_unboxed_storage_raw (mapdict.rs): the STORE_ATTR_caching resolution gates (custom __setattr__ / __getattribute__, version_tag, classify, find_map_attr) restricted to an existing unboxed slot, and the same-type in-place longlong write (mapdict.py:615-619). - jit_mapdict_unboxed_write_raw (helpers.rs): non-forcing void residual. - try_walker_specialize_store_attr (jitcode_dispatch.rs): after resolving an unboxed-int slot and proving the value is a plain int (exact type, bool rejected), guard_class + guard_value(map), unbox the value to raw i64, and return a substitute descr (CannotRaise) + arglist. The caller swaps only the helper/effect/args and continues through the SAME generic residual recorder/executor, so concrete execution, body-effect tracking, and rollback are identical to the generic setattr path. No W_IntObject is re-boxed. - Hoist the shared guard_class + guard_value(map) block into walker_guard_mapdict_instance_shape (used by both the load and store folds). - PYRE_FBW_STOREATTR_FOLD kill switch (default ON), independent of the read fold since the write commits a concrete heap mutation. The store collapses from ~7 ops to a single non-forcing CallN carrying the raw value; the value re-box virtualizes away. attr_min (o.x=o.x+1, 2e6 iters) 0.23s -> 0.02s. synth dynasm 176/176, cranelift 176/176, GC-soak clean. Assisted-by: Claude
run_synthetic_bench passed max-pypy-ratio to every enabled backend. run_bench
takes no wasm_vs_* parameter and hardcodes ("wasm", None, None), so the
synthetic suite was the only caller gating wasm on a ratio.
Pass None for wasm and keep its per-bench timeout as the hang guard.
Assisted-by: Claude
The map guard_value the attr folds rely on was dead code until the fix on main (walker_guard_mapdict_instance_shape tested box_value, which falls back to the walker's always-present concrete shadow, so the guard was never emitted). The synthetic suite stayed green at 176/176 throughout because it had no fixture that reaches the shapes an unguarded map breaks; polymorphic_slot_retype.py covers local retyping and the valuestack, not map identity. mapdict_polymorphic_map_attr: one LOAD_ATTR site over two plain instances whose attribute insertion order assigns `q` different storage slots. Without the map guard this read 1201010 instead of 2200000, since GuardClass(&INSTANCE_TYPE) passes for both maps. mapdict_unboxed_type_change_attr: a hot loop over an unboxed int slot that takes a float mid-loop, and an unboxed float slot that takes an int. Either write runs convert_to_boxed, which allocates a new map through node_copy; without the guard the trace kept calling jit_mapdict_unboxed_read_raw, which unerase_unboxed'es a now-real PyObjectRef as *mut Vec<i64> (int slot returned a heap address as an int; float slot segfaulted). Both compile their loop, so they pin the guard rather than the interpreter. Assisted-by: Claude
LOAD_ATTR declined an unboxed float slot and STORE_ATTR declined anything but an int one, so a float attribute kept the forcing getattr/setattr residuals: o.f = o.f + 1.0 ran 8.16s against pypy's 0.25s at 60M iterations, while the same loop without the attribute was already at parity. The decline comment claimed a Float-bank bitcast was needed first. It is not: unboxed storage already holds the value's bit pattern (mapdict.py:577,584 float2longlong / longlong2float), so a float-typed helper converts in Rust and the trace only needs the bank it already has. mapdict is unchanged — both fast paths already return the UnboxType and the raw accessors are type-agnostic. Add jit_mapdict_unboxed_read_f / jit_mapdict_unboxed_write_f beside their raw twins and emit_trace_call_float_typed beside emit_trace_call_int_typed, then give both folds a Float arm: read through CallF plus wrapfloat, write through walker_unbox_float into a [Ref, Int, Int, Float] void residual. A value whose type is not exactly float still declines, since that write converts the slot to boxed storage. Route the concrete void call through call_void_function_typed: args reach the executor packed into machine words, so passing an f64 through the all-i64 transmute would leave the callee reading an untouched floating-point register. The host-trampoline path reflects the callee's real signature and is unchanged. attr_float at 60M: 8.16s -> 0.24s against pypy 0.23s. Assisted-by: Claude
Replace the generic setattr residual's helper and arguments with a direct mapdict slot write when the receiver's guarded shape pins an existing boxed slot. The residual's original may-force effect is kept, so the force token, the virtualizable spill, and the trailing force/exception guards stay as the generic setattr emitted them; only the opaque setattr_fn MRO walk is replaced. Adds store_attr_boxed_fast_path + write_boxed_storage (matching plain_direct_write's boxed arm) and the jit_mapdict_boxed_write residual. PYRE_FBW_STOREATTR_FOLD=0 declines the fold. attr_boxed at N=60M: 3.77s without the fold, 0.5s with it. Dropping the residual's may-force effect on top measures 0.47s, so the win is the removed MRO walk rather than the force protocol, and the effect is left alone. Assisted-by: Claude
`drive_bridge_carrier_walk`'s `PYRE_P2_COMPILE` path returns into `full_body_walk_trace` after its sub-walk has concrete-executed the reconstructed callee and journaled the callee's eager stores. That walk's entry `fbw_store_journal_reset()` dropped those entries rather than settling them, so a root walk that did not commit rolled back only its own: the callee's stores stood while the blackhole replayed the callee from the guard, applying them a second time. The drain's own abort path already rolls back. Take a `WalkJournals` argument. The fresh-walk caller keeps the reset; the drain continuation keeps the journals so the root walk's epilogue settles them -- commit keeps the stores, non-commit rolls them back. The unjournaled-effect flag carries over for the same reason: a sub-walk that only recorded a residual must still block the root's commit. `pyre/bench/synth/inline_multiframe_drain_journaled_store.py` under `PYRE_P2_COMPILE=1`: hits[0] 120299 -> 120000 (N=120000), 3x deterministic. loops_compiled, bridges_compiled and loops_aborted are unchanged at 2 / 0 / 299, and `P2Drain::CompileRoot` still fires 598 times, so no trace decision moved -- the 299 aborts are the 299 doubles. The default drain-abort path is untouched. Assisted-by: Claude
The JIT LOAD_ATTR/STORE_ATTR folds resolve attributes through the `*_fast_path` ports of LOAD_ATTR_caching/STORE_ATTR_caching but skipped two steps those paths perform. STORE_ATTR_caching sets `attr.ever_mutated` before `_direct_write` (mapdict.py:1635-1637). `store_attr_boxed_fast_path` and `store_attr_unboxed_fast_path` now mark it too; a folded write previously left the attribute marked unmutated. Nothing in pyre reads `ever_mutated` yet, so no observable behavior changes. LOAD_ATTR_caching resolves to `attr._direct_read` (mapdict.py:1539), which for an unboxed attribute migrates the instance off unboxed storage once its class froze unboxing (mapdict.py:594-596). The folded read performs `_prim_direct_read` alone, so `load_attr_unboxed_fast_path` now declines a frozen class and leaves the read to the residual, whose `direct_read` runs the migration. Both forms return the same value — `_direct_read` reads before it converts — so this changes trace shape, not results. On a probe that freezes unboxing and then hot-loops the first read of 40000 pre-created instances, both print 799980000: before: bridges_compiled=1 guard_failures=202 after: bridges_compiled=0 guard_failures=1 Skipping the migration left unboxed and boxed instances mixed under one promoted-map guard; the residual migrates each on its first read, after which the boxed fold applies. mapdict_frozen_unboxing_fold.py covers that shape, which no bench reached. It passes either way — no output distinguishes the two — so it is coverage, not a regression guard. The effectinfo docs for both helper tags described the earlier folds: the LoadAttr tag still listed an unboxed slot as falling through to the residual, and StoreAttr claimed integer-only. Both reported by the Codex parity review on PR #592. Assisted-by: Claude
The WalkJournals enum was inserted between full_body_walk_trace's rustdoc
and the fn, so rustdoc attached the whole 16-line block ("Issue #73
production full-body tracer") to the enum and the fn lost its docs. Move
the enum above the block.
Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nine commits: the mapdict instance-attribute folds, the two parity gaps the Codex
review found inside them, a
check.pygate fix, and a correctness fix in the"committed effect re-applied on resume" family found while validating the folds.
1. mapdict instance-attribute folds
LOAD_ATTR/STORE_ATTRon a fixed-layout instance whose guarded shape pins anexisting storage slot lower to a direct slot access instead of the opaque
getattr_fn/setattr_fnMRO walk. Unboxed int, unboxed float, and boxedslots.
PYRE_FBW_LOADATTR_FOLD=0/PYRE_FBW_STOREATTR_FOLD=0decline.All four arms take the receiver type's live
version_tagguard (main added itmid-development; it is threaded through the shared
walker_guard_mapdict_instance_shapehelper so no arm can miss it): the instancemap pins the storage layout, but class mutation can change lookup precedence
without changing that map.
attr_boxedat N=60M: 3.77s → 0.5s. Dropping the residual's may-force effecton top measures 0.47s, so the win is the removed MRO walk rather than the force
protocol — the boxed arm therefore keeps the residual's original may-force
effect (force token, virtualizable spill, trailing force/exception guards) and
replaces only the helper and its arguments. The unboxed arms write no GC
reference and do drop it.
Two synthetic fixtures cover the map guard (polymorphic map, unboxed type
change).
2.
mapdict: fold parity for ever_mutated and allow_unboxingBoth reported by the Codex parity review on this PR; both verified against the
upstream source before fixing.
The folds resolve attributes through the
*_fast_pathports ofLOAD_ATTR_caching/STORE_ATTR_cachingbut skipped two steps those paths take.ever_mutated—STORE_ATTR_cachingsets it before_direct_write(mapdict.py:1635-1637); the two store fast paths did not, so a folded write left
the attribute marked unmutated. Severity is honest: latent. Nothing in pyre
reads
ever_mutated— every.get()is the "already set?" check before setting,and
node_readdocuments that both read variants share one body — so noobservable behavior changes.
allow_unboxing—LOAD_ATTR_cachingresolves toattr._direct_read(mapdict.py:1539), which for an unboxed attribute migrates the instance off
unboxed storage once its class froze unboxing (mapdict.py:594-596). The folded
read performs
_prim_direct_readalone.load_attr_unboxed_fast_pathnowdeclines a frozen class and leaves the read to the residual, whose
direct_readruns the migration.
This is not a wrong answer either way —
_direct_readreads before itconverts — but the gate does fire and changes trace shape. Probe: freeze
unboxing, then hot-loop the first read of 40000 pre-created instances. Both print
799980000:
Skipping the migration left unboxed and boxed instances mixed under one
promoted-map guard. The residual migrates each on its first read, after which the
boxed fold applies; post-freeze new instances are already boxed
(mapdict.py:197), so the window is pre-freeze instances not yet read.
mapdict_frozen_unboxing_fold.pycovers that shape, which no bench reached. Itpasses without the fix — no output distinguishes the two, and
check.pygatesoutput, not counters — so it is coverage, not a regression guard.
3.
check: exempt wasm from the synthetic pypy ratio gaterun_synthetic_benchappliedmax-pypy-ratioto every enabled backend.run_benchtakes no
wasm_vs_*parameter and hardcodes("wasm", None, None), andcheck.py:81states "The wasm backend has no perf-ratio gate" — the syntheticsuite was the only caller gating wasm on a ratio, i.e. it broke the invariant the
file documents. Pass
Nonefor wasm; its per-bench timeout stays the hang guard.This is a live failure on main today (
FAIL wasm synth/residual_raise_except_resume … x10.0), not a hypothetical: that bench measured ~11.9x on wasm and passedbefore #576 added the header, so only the gate appeared — the number barely moved.
4.
jit: settle the drain sub-walk's journals in the root walkdrive_bridge_carrier_walk'sPYRE_P2_COMPILEpath returns intofull_body_walk_traceafter its sub-walk has concrete-executed the reconstructedcallee and journaled its eager stores. That walk's entry
fbw_store_journal_reset()dropped those entries rather than settling them —its comment assumes a "prior aborted walk", but the drain's sub-walk already ran.
So a root walk that did not commit rolled back only its own entries: the callee's
stores stood while the blackhole replayed the callee from the guard. The drain's
own abort path already rolls back; this second exit did not.
full_body_walk_tracetakes aWalkJournalsargument (only two callers exist).The fresh-walk caller keeps the reset; the drain continuation keeps the journals
so the root walk's epilogue settles them — commit keeps the stores, non-commit
rolls them back.
Measured on the in-repo regression guard
pyre/bench/synth/inline_multiframe_drain_journaled_store.py("hits[0] != N meansthe drain doubled"), under
PYRE_P2_COMPILE=1, both binaries verified distinct:Every counter identical and
P2Drain::CompileRootstill fires 598 times, so notrace decision moved — the 299 aborts are the 299 doubles. 3x deterministic both
ways.
PYRE_P2_COMPILEis opt-in/default-OFF, so this was latent, not shipping;the default drain-abort path is untouched.
Known gap this PR does not close
The
list.appendjournal hazard the dropped commit aimed at is real and stays asit is on main: the generic dispatcher concrete-executes a declined
list.appendwithout
fbw_append_journal_push, so a non-committing walk cannot rewind it. Theabort was the wrong instrument.
try_execute_residual_call_via_executoralreadycarries the right one —
inplace_list_journalcaptures(list, len_before)beforethe call and journals it from the success arm — and an append is rewindable by
length under every strategy, so routing the declined append through that path keeps
both the trace and the journal. Left for its own change, with its own repro.
Verification
python3 pyre/check.pyon baseccdc1a52be(LLBC re-extracted after the rebase; therun built all four binaries itself): dynasm 194/194, cranelift 194/194, wasm 193/193,
3/3 backends, 0 FAIL.
🤖 Generated with Claude Code