Carry jf_savedata through guard resume; drop the vable identity override - #1983
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
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 cbc1129). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
- resume.rs consume_vable_info reads the identity with next_ref() and asserts get_total_size(virtualizable) == vable_size - 1. The identity_override argument, the NULLREF substitution in next_ref_for_resume_slot, the JitState::blackhole_virtualizable_identity hook (trait default and state-field macro impl), and MAJIT_LEAF3_PROV are removed. - patch_new_loop_to_load_virtualizable_fields no longer returns early when inputargs are not longer than the red prefix. - GUARD_NOT_FORCED_2: cranelift records the force spill locations on the descr (set_rd_locs), keeps slot 0 for FINISH's result, and adds the guard's Ref spills to FINISH's gcmap; dynasm routes it through consider_guard_not_forced_2_j2. The dynasm raw exit returns jf_savedata instead of None. - handle_fail, resume_in_blackhole_from_exit_layout, wasm_ca_resume_deopt and handle_async_forcing root the copied jf_savedata word with DeadFrameRefRoots across their allocations and read it back; handle_fail returns the reloaded value. - Trace::get_iter_for_optimizer seeds ByteTraceIter from plain InputArg copies that carry the InputArgRc types and values. spec_folds! 87 -> 87, try_walker_specialize_ 77 -> 77. Assisted-by: Grok 4.7 Assisted-by: Claude Opus 5.5
The raw malloc fallback stays only for an untyped array descr (type_id == 0). Assisted-by: Grok 4.7 Assisted-by: Claude Opus 5.5
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
youknowone
force-pushed
the
fib_recursive
branch
from
September 26, 2026 16:42
49560bc to
cbc1129
Compare
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
1 participant
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.
Summary
Two commits on
main.Carry
jf_savedatathrough guard resume; drop the vable identity overrideresume.rsconsume_vable_infoisresume.py's again. It reads the identity withnext_ref()and assertsget_total_size(virtualizable) == vable_size - 1.identity_overrideargument, theNULLREFsubstitution innext_ref_for_resume_slot, theJitState::blackhole_virtualizable_identityhook, andMAJIT_LEAF3_PROV.patch_new_loop_to_load_virtualizable_fields: it no longer returns early wheninputargsis no longer than the red prefix.compile.pyhas no such return.GUARD_NOT_FORCED_2:assembler.pystore_info_on_descr), keeps slot 0 for FINISH's result, and adds the guard's Ref spills to FINISH's gcmap.consider_guard_not_forced_2_j2.jf_savedata(llmodel.pyget_savedata_ref) instead ofNone.jf_savedatarooting:handle_fail,resume_in_blackhole_from_exit_layout,wasm_ca_resume_deoptandhandle_async_forcingroot the copied word withDeadFrameRefRootsacross their allocations and read it back.Trace::get_iter_for_optimizer: seedsByteTraceIterfromInputArgcopies that carry theInputArgRctypes and values.Abort
BC_NEW_ARRAYwhen a typed allocation returns nullA typed array descr (
type_id != 0) now aborts the trace whenalloc_oldgen_typedreturns 0. The raw malloc fallback stays only fortype_id == 0. This is the #1944 CodeRabbit finding ondispatch.rs.History
The first commit replaces an earlier "Port leftover=[] GETFIELD and AllVirtuals" that never landed on main.
(baked, path)abort tuples in place ofassert i == len(inputargs), pyre→majit listiter/str predicate callbacks, an EC-top scan frame in TLS, a savedata TLS hold, a skip-if-unchangedgen_store_back_in_vable, and soft misses in place of hard errors.pyre/check.pyfailed 58 / 58 / 57 rows (dynasm / cranelift / wasm), including asynth/generator_body_loopcrash from the partial store-back.majit-gccommits that cancelled each other were dropped.Fold census:
spec_folds!87 → 87,try_walker_specialize_77 → 77.Not addressed: the #1944 Codex finding on borrowed
[u8; N]layout. The same Rust type has two layouts: a ctor local is a length-prefixed GcArray, and an inline field is headerless. Declining the graph when a borrow's layout disagrees would decline 1669 graphs and make hot loops time out. It needs a slice representation change instead.Test plan
Run locally on the same two commits based on
17c8884acc7:python3 scripts/extract-llbc.pycargo test --all --no-default-features --features dynasmcargo test -p majit-backend-wasmpyre/extra_tests/parity_tests/run.py, boundary check,cargo fmt --checkpython3 pyre/check.py(dynasm, cranelift, wasm). The rows that remain red here are also red on main:synth/gc_pypy_frontend(guard_failures 331 -> 37x) andsynth/pickle_terminal_raise_resume(loops_aborted 3 -> 4) fail the same way in main's own CI run 36212964394 at17c8884acc7. Rebuilding the base sources locally gives the same numbers. These baselines were not re-recorded.synth/raise_catchtimes out at 5 s only under host load (load average 124–208). Run alone, it uses 0.93 s (dynasm) and 1.22 s (cranelift) of CPU, against 0.86 s for pypy.After rebasing onto
b18ddc84291, the four new main commits touch none of this PR's files.cargo check --testspasses for majit-metainterp and the backends, with dynasm and with cranelift.🤖 Generated with Claude Code