jit: collecting bigint residuals, nursery call frames, vable store-back - #1712
jit: collecting bigint residuals, nursery call frames, vable store-back#1712youknowone wants to merge 10 commits into
Conversation
|
Warning Review limit reachedNext included review available in 29 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: Team Run ID: 📒 Files selected for processing (50)
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 Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 971a5dc). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
![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: 66c29a9e74
ℹ️ 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".
| exit_layout, | ||
| guard_exc, | ||
| forced_guard_cache_owner(descr_arc, frame_root.frame()), | ||
| descr_arc.is_guard_forced().then_some(savedata).flatten(), |
There was a problem hiding this comment.
When a GUARD_NOT_FORCED reaches this arm, savedata is only a copied GcRef; the backend deadframe that owned jf_savedata has already been dropped. handle_fail then registers the exception and fail-value roots through GC hooks before resume_in_blackhole_from_exit_layout finally roots this value, so a concurrent collection can move AllVirtuals and leave this copy stale. Root the savedata slot before calling handle_fail (in this and the two repeated entry paths), or retain the deadframe owner until resume.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
| &exit_layout, | ||
| guard_exc, | ||
| forced_cache_owner, | ||
| None, |
There was a problem hiding this comment.
For an in-guest wasm CALL_ASSEMBLER exit through GUARD_NOT_FORCED, dead_frame_from_ran_frame copies jf_savedata into WasmFrameData, but this path never reads it and unconditionally passes None. The resume reader consequently uses resume_after_guard_not_forced == 0, rematerializes the already-forced virtuals, and replays pending heap writes instead of reusing the saved cache; extract and root backend.get_savedata_ref(&frame) through the bridge attempt, then pass it for forced descriptors.
AGENTS.md reference: AGENTS.md:L187-L191
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7eea12312
ℹ️ 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".
| let savedata = crate::allvirtuals::allocate(ptrs, ints); | ||
| self.backend.set_savedata_ref(&mut deadframe, savedata); |
There was a problem hiding this comment.
When async forcing materializes virtuals, AllVirtuals::allocate returns a managed object that is held only in this copied GcRef until set_savedata_ref completes. Each managed backend starts that method with a write-barrier/GC hook, so on the free-threaded target a collection in this publication window can move or reclaim the object and leave jf_savedata holding the stale address. Publish savedata in a root slot before this call and reload the forwarded value for the store.
AGENTS.md reference: AGENTS.md:L159-L161
Useful? React with 👍 / 👎.
| if box_unchanged(field_index, value) { | ||
| continue; |
There was a problem hiding this comment.
When a standard virtualizable slot still has its entry OpRef, this branch now suppresses its SETFIELD_GC/SETARRAYITEM_GC. In upstream MetaInterp.gen_store_back_in_vable, “only write back the fields really modified” is an unimplemented TODO: the following loops still emit every store unconditionally. Implementing that TODO here changes the forcing operation stream rather than porting the upstream mechanism, so restore the unconditional loops.
AGENTS.md reference: AGENTS.md:L223-L228
Useful? React with 👍 / 👎.
d7a2699 to
35ffaa3
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/35ffaa34e24c293189cfa8f45e038a2cff18a280/pyre-interpreter/src/pyframe.rs#L1284
When a call frame has cell variables, this new nursery allocation is followed in finish_for_call_with_globals_obj by pin_root(locals_cells_stack_w) and then collecting/synchronizing calls such as w_cell_new and remember_frame_locals_array, but the returned root slot is ignored and the original raw pointer is retained in arr and later stored into the PyFrame. If another thread requests a collection at one of those GC operations, the root slot is forwarded while the copied pointer remains stale, causing writes into from-space and leaving the frame with a dangling locals array. Preserve the root index and reload the array from it after each possible safepoint before dereferencing or publishing it.
AGENTS.md reference: AGENTS.md:L159-L161
ℹ️ 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".
35ffaa3 to
f70be61
Compare
`exec_or_eval` pinned the locals argument only after `ensure_exec_builtins` / `ensure_eval_builtins`, which dispatch a dict subclass's `setdefault` and can collect. The mapping survives that collection -- the caller's frame holds it -- but it moves: the frame's slot is forwarded and this raw local is not, so the pin below and the `setdictscope` it feeds took the address the object had vacated. `exec_separate_locals` in `bench/synth/exec_namespace_code_object_rooting_regression.py` is the shape that reaches it, and it is the only one of that fixture's five whose locals mapping is an object distinct from its globals. The argument is now published into `ns_roots` before the plant and read back where it is consumed. `globals_supplied` and `locals_supplied` are decided before the plant as well, which retires two `is_none_or_null` reads of pointers taken beforehand, and the `locals_arg is globals_arg` test reads both sides out of their slots. The comment above `inject_closure` still said the closure needed no rooting across these allocations; 3dba87b published it. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Port compile.py AllVirtuals as the jf_savedata GC object. Add alloc_fast_nursery_collecting_typed_roots for a live-root span. After a wasm CALL_ASSEMBLER reload, reapply the frame write barrier and keep the typed result-homing. Also carry the rest of the parked backend, optimizer, and jitcode_dispatch working tree. Assisted-by: Claude
Assisted-by: Claude
emit_memory_error_check gained write-barrier and gc-map arguments; the CheckMemoryError arm still called it with seven. The other two call sites already passed both. Assisted-by: Claude
Measured on tip: loops_compiled=1 bridges_compiled=3 guard_failures=879, matching PyPy's loop + 3-bridge shape. Assisted-by: Claude
build_wasm_module's third return is now WasmModuleData. The consecutive-allocation test still asserted the tuple element against 2, so cargo test failed to compile. Assisted-by: Claude
AllVirtuals moved export_state onto Optimizer and skipped export when a peeled body already had imported state. The peeled loop then re-emitted GetfieldGcR for a declared immutable ref that main hoists, failing jit_interp_immutable_field_folds on macOS and Windows CI. Assisted-by: Claude
Keep OptContext::can_share from inheriting a recorded snapshot, restore the tagged GuardTrue/False orgpc encode, and treat a one-frame sizes vector as the default layout so leftover snapshot boxes are not dropped. Record the collecting typed-roots nursery helper as unmatched. Assisted-by: Claude
f70be61 to
971a5dc
Compare
1 participant
Summary
Three pieces on top of
main, plus a parked working-tree commit.f0608e0— collecting bigint residuals + nursery call-frame locals.jit_bigint_mul/and/or/xorallocate digit arrays through the same collecting nursery path as add (malloc_fast+collect_and_reserve, liveRBigInts rooted)._flat_pycall/_flat_pycall_defaultslocals useNurseryGc, matchingPyFrame.__init__'s[None] * size.17920f5— skip unmodified virtualizable store-back.gen_store_back_in_vableno longer writes boxes that still hold the entry snapshot (pyjitpl.py“xxx only write back the fields really modified”). Stores that do fire use the parentSizeDescrfield descrs so OptHeap can pair them with interpreterSETFIELD_GC.834856f— exec/eval locals mapping.Residual comment/flag tweak after rebase;
mainalready has the fuller plant-before-rooting form.66c29a9— parked tree: AllVirtuals, multi-root nursery alloc, wasm CA barrier, optimizer/dispatch.This is the previous dirty working tree committed as one unit so the branch could rebase. It is not claimed complete: no
check.pyafter rebase, and it should be split before landing if review wants a smaller fib-only PR.rbigint::(50) and thegen_store_back_*/hint_force_virtualizable*unit tests passed before rebase.python3 pyre/check.pyhas not been run on this tip.Remaining on the fib_recursive gap (not in this PR's “done” set): compiled recursive fib still has more bridges than PyPy (7 vs 3), and last_instr/flags writeback on a changed box is still emitted.
Self-review
Assisted-byto commit messages to the commits AI wrote.Auto-review is not done in the session that wrote the code. The large
66c29a9commit should be treated as WIP until split or reviewed on its own.— commented by Claude