Repository navigation
jit, gc, _pickle: memoize callee inline verdicts; fix the immortal-pickler child loss and the unguarded probe in major marking - #994
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 Plus Run ID: 📒 Files selected for processing (14)
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
https://github.com/youknowone/pyre/blob/4d60a8164ddd6ea51c41661fd93f99a15e77d853/pyre-jit-trace/src/jitcode_dispatch/diag.rs#L55-L57
When PYRE_FBW_INLINE_DIAG is normally unset, eligible user-call inline attempts still execute the direct std::env::var check at inline_call.rs:2841, so this hot path continues taking the environment lock and walking the environment table despite the new cache. Additional direct checks remain at inline_call.rs:4201,4395,4535 and vable_ops.rs:360; route these through fbw_inline_diag_enabled() as well to complete the intended optimization.
ℹ️ 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".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit fff3d0b). 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)None. 4. Structural adaptations
|
…yload Six inline-recipe predicates decode the whole callee body, and the recipes called them per call site on every trace that reached the call: `callee_body_contains_raise`, `method_form_callee_body_supported`, `exception_string_override_straight_line`, `..._sample_safe`, `..._has_nested_call`, and the `abort_permanent` scan. Each is a pure function of the callee's assembled bytecode and its descr pool, which `sub_jitcode_body_for_code` and `sub_jitcode_descr_pool_for_code` resolve from the same code object. `PyJitCodePayload` now carries them as an `InlineBodyFacts` `OnceCell` beside `sub_descr_pool`, and `replace_with` drops them with the body they describe. This retires `CALLEE_ABORT_PERMANENT_SEEN`, a thread-local `BTreeMap` keyed by the raw `w_code` address. Its own comment recorded the hazard: switching `w_code_new` to `try_gc_alloc_stable` would let a reclaimed address answer for another code object, where a stale `false` admits an inline whose body carries `abort_permanent`. Every substituted site already declined when the descr pool was absent, so resolving the verdicts before that check leaves the decisions unchanged. Assisted-by: Claude
`std::env::var_os` takes a global lock and walks the env table on every call. `PYRE_FBW_STRICT_DIAG` was read from inside `callee_fast_path_inlinable`'s per-opcode decode loop, and `PYRE_FBW_INLINE_DIAG` on every inline decline. Adds `fbw_strict_diag_enabled`, `fbw_inline_diag_enabled`, `fbw_mf_diag_enabled` and `p2_diag_enabled` to `jitcode_dispatch::diag`, matching the `OnceLock<bool>` shape of the `pcmap_*_audit_enabled` helpers already there, and routes all 22 call sites in the crate through them. Assisted-by: Claude
…ld descr `vable_setfield_descr` is a raw pass-through to `record_op_with_descr`; it does not apply the `vable_static_record_descr` resolution that `vable_setfield` does. The pre-residual-call `last_instr` publish passed the vinfo's own `static_field_descrs[idx]`, which numbers the field by the vinfo `[token, statics, arrays]` order rather than PyFrame's struct declaration order, so the store carried a descr identity of its own. `emit_traceback_node` writes the same field on the same frame at the same offset through `pyframe_next_instr_descr()`, i.e. the parent-struct descr. Under two identities neither store supersedes the other and which one reaches the frame is decided by deferred-flush order rather than record order. `virtualizable.py:71` builds the vinfo descrs with `cpu.fielddescrof(VTYPE, name)`, so upstream has one object per field and the question cannot arise. `fbw_state.rs` already made this correction for the exit publish; this is the same correction for the residual-call publish. Also delete `gen_writeback_inline_frame_to_heap`, which carried the same vinfo-descr shape on two more stores. It has had no callers since the trait meta-interpreter was retired and the FBW walker became the sole tracer; its live successor, the CALL_ASSEMBLER inline-callee `last_instr` pin, already uses `pyframe_next_instr_descr()`. `trace_ctx.rs gen_store_back_in_vable` records under the vinfo descr too, but it is a faithful port of `pyjitpl.py:3489-3521`, writes all six statics plus the array at once, and is shared with the synthetic-VTYPE unit tests; it is left alone. Verified: pyre/bench/frame_lineno_mid_replay_regression.py PASS (it pins `tb_lineno` against `tb_frame.f_lineno` on a frame that catches, which is the two-writers-one-offset case). Assisted-by: Claude
…eir field stores An immortal object allocated with `alloc_with_gc_header` is traced exactly once: `grey_child`'s registered-header path sets VISITED and nothing clears it, and minors never trace immortal children at all. The only thing that recurses into an immortal's children is `walk_raw_immortal_roots`, which runs from root slots only. So an immortal with mutable GC slots keeps its children alive only while it sits on a frame/shadow root slot at collection time — holding one in an instance attribute, a module global or a list element loses them. `W_Pickler` / `W_Unpickler` / their memo proxies / `W_BufferWrapper` are exactly that shape: `w_memo` persists across `dump` calls. Allocate all five with `allocate_stable`, i.e. non-moving managed old-gen with TRACK_YOUNG_PTRS — an ordinary marked and swept object, which is the state `W_Pickler(W_Root)` has upstream. All five together: an immortal proxy over a managed pickler would recreate the latch on the proxy. The proxies' twin methods proved the second half was already missing: `UnpicklerMemoProxy::clear` carries `unpickler_write_barrier` on its `w_memo` store while `PicklerMemoProxy::clear` carries nothing. Add `pickler_write_barrier` and call it on every pickler GC-pointer store — `clear_memo`, `set_memo`, `set_dispatch_table`, `set_persistent_id`, the proxy `clear` — per `framework.py:1423`, which requires one barrier per non-constant GC setfield regardless of the child's generation. The deleters store `PY_NULL`, a constant, and are left alone. The two post-allocate proxy wirings get one as well: the initializing-store exemption assumes a fresh young object, and `malloc_typed_stable` has already consumed the proxy's TRACK_YOUNG_PTRS by firing the managed barrier during construction. `clear_memo` additionally re-reads `self` from a pinned slot after `w_list_new`, which can collect; it was storing through the pre-call `&mut`. Sweeping a `W_Unpickler` now leaks its `encoding`/`errors` `String` buffers, since the registration installs no destructor — strictly better than today, where the whole object leaks. Verified with a repro holding the pickler in an instance attribute, a module global and a list element, dumping across two major collections: 5/10 runs died with `GC BUG: invalid type_id ... site=remember_young_pointer_insert` before the fix, 0/20 after. test_descr, test_pickletools and test_picklebuffer report no regressions, and pickle_ctor_args jitstats are unchanged (loops_compiled=5, bridges_compiled=0, loops_aborted=0, guard_failures=1). `W_MemberDescr` is the censused sixth type and is deliberately excluded: its setter uses `mark_prebuilt_roots_dirty`, an immortal-only mechanism, and member descriptors are minted for every `__slots__` name including those on builtin types anchored in immortal type dicts. Assisted-by: Claude
…ing path `seed_major_root` probed an arbitrary address before any range check: `registered_pyobject_header` filters only on `addr >= GcHeader::SIZE` and 8-byte alignment, then performs two loads — the vtable word at `addr` and the header at `addr - 8`. The major root banks are untyped `i64` slices (`walk_bh_regs` over a blackhole `registers_r` sized to the jitcode's `num_regs` with only the live subset filled, and `walk_resume_ref_roots` over the in-flight resume image), whose documented contract is that slots holding non-pointers pass through unchanged. So the probe could dereference an arbitrary word. The minor path was never changed and still range-checks first in `drag_out_root`, which is why this was major-only. `incminimark.py:2739-2753 _collect_obj` performs no such probe: the type system guarantees every `Ptr(GcStruct)` root is a real GC object, so the only tests are non-null and `is_in_nursery`. pyre's roots are untyped, and the arena range check is what stands in for that guarantee, so it has to precede every dereference. Restore that order in `seed_major_root` and drop the same probe from `grey_child`'s `header_bearing` arm. The `grey_child` arm had a second effect: an address it admitted got VISITED set with nothing to clear it. The only reset passes are the oldgen sweeps and `finish_incremental_cycle`, which iterates only `prebuilt_root_objects` — a list appended to solely under NO_HEAP_PTRS. A non-arena object admitted there was therefore traced in the first major after it appeared and never again. `incminimark.py:2782` states the invariant: such an object "should be in 'prebuilt_root_objects', and the GCFLAG_VISITED will be reset at the end of the collection". The arm also returned ahead of the `GC BUG: invalid major child` diagnostic, swallowing the class of bug the probe was added to chase. `registered_pyobject_header` keeps its remaining caller in `do_write_barrier`, where the bootstrap-prebuilt family needs it; its doc is updated to say so. The probe was reaching non-arena picklers because those objects were immortal. The preceding commit allocates them stable, i.e. managed, so `is_managed_heap_object` now answers for them directly and the probe has nothing left to find there. Validated in that combination: pickle_ctor_args, the fixture the probe was introduced for, passes 10/10 with jitstats unchanged from baseline (loops_compiled=5, bridges_compiled=0, loops_aborted=0, guard_failures=1). Also 0/40 on mutate_then_raise_caught under MAJIT_STRICT, 0/20 on the pickler-in-instance-attribute repro, `cargo test -p majit-gc` 228 passed, and check_synthetic.py ALL PASSED. Also refresh a stale comment in pyre-jit: PyCode is allocated with `malloc_typed`, not `Box::into_raw`, so the reason its registered TypeInfo trace is inert is its empty gc_ptr_offsets. Assisted-by: Claude
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fff3d0b473
ℹ️ 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".
| pub(crate) fn fbw_inline_diag_enabled() -> bool { | ||
| static ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *ENABLED.get_or_init(|| std::env::var_os("PYRE_FBW_INLINE_DIAG").is_some()) |
There was a problem hiding this comment.
When PYRE_FBW_INLINE_DIAG is unset (the normal configuration), several hot-path consumers still bypass this helper and call std::env::var on every inline attempt, notably inline_call.rs:2841, inline_call.rs:4395, inline_call.rs:4535, and vable_ops.rs:360. Consequently, successful user-call and type-call tracing still takes the global environment lock that this change is intended to remove; use fbw_inline_diag_enabled() at the remaining consumers as well.
Useful? React with 👍 / 👎.
Five commits: two on the walker's inline path, three closing documented GC/descr defects.
pyre-jit-trace: decide callee body-shape verdicts once per jitcode payloadSix
try_walker_inline_*decline predicates are pure functions of the callee's code object but re-decoded the callee bytecode at every call-site evaluation (callee_body_contains_raise,method_form_callee_body_supported, the threeexception_string_override_*, and theabort_permanentscan). They now compute once intoInlineBodyFacts, aOnceCellonPyJitCodePayloadbesidesub_descr_pool— the home the file already documented for this kind of verdict, and onereplace_withdrops together with the body it describes.That retires
CALLEE_ABORT_PERMANENT_SEEN, a thread-localBTreeMapkeyed on the raww_codeaddress, whose surrounding comment already named the hazard: a freed-and-recycled code object answers for the wrong callee.Upstream makes this decision once per graph at codewriter time (
policy.py look_inside_graph) and bakes it into the jitcode;_opimpl_inline_call1/2/3have no runtime decline at all. pyre decides at trace time, so the verdicts at least become per-code-object rather than per-call-site.pyre-jit-trace: cache the fourPYRE_FBW_*/PYRE_P2diag flags22
std::env::var_ossites now route throughOnceLock<bool>helpers matching thepcmap_*_audit_enabledshape already in that module.PYRE_FBW_STRICT_DIAGwas being read insidecallee_fast_path_inlinable's per-opcode decode loop.jit: publish the residual-calllast_instrunder the parent-struct field descrvable_setfield_descris a raw pass-through torecord_op_with_descrand does not apply thevable_static_record_descrresolution thatvable_setfielddoes. The pre-residual-call publish passed the vinfo's ownstatic_field_descrs[idx], which numbers the field by the vinfo[token, statics, arrays]order rather than PyFrame's struct declaration order.emit_traceback_nodewrites the same field on the same frame at the same offset through the parent-struct descr. Under two identities neither store supersedes the other and flush order decides which reaches the frame.virtualizable.py:71builds the vinfo descrs withcpu.fielddescrof(VTYPE, name), so upstream has one object per field.fbw_state.rsalready made this correction for the exit publish.Also deletes
gen_writeback_inline_frame_to_heap, which carried the same shape on two more stores and has had no callers since the FBW walker became the sole tracer.Verified by
pyre/bench/frame_lineno_mid_replay_regression.py, which pinstb_linenoagainsttb_frame.f_linenoon a frame that catches — the two-writers-one-offset case._pickle,memoryview: allocate the pickler types stable and barrier their field storesAn immortal object is traced exactly once —
grey_child's registered-header path sets VISITED and nothing clears it, and minors never trace immortal children. The only thing that recurses into an immortal's children iswalk_raw_immortal_roots, which runs from root slots only. So an immortal with mutable GC slots keeps its children alive only while it sits on a root slot at collection time; holding one in an instance attribute, a module global or a list element loses them.W_Pickler/W_Unpickler/ their memo proxies /W_BufferWrapperare exactly that shape (w_memopersists acrossdumpcalls). All five now useallocate_stable— non-moving managed old-gen, an ordinary marked and swept object, which is the stateW_Pickler(W_Root)has upstream.The proxies proved the second half was missing:
UnpicklerMemoProxy::clearcarries a write barrier on itsw_memostore whilePicklerMemoProxy::clearcarries none. Addedpickler_write_barrieron every pickler GC-pointer store perframework.py:1423.clear_memoadditionally re-readsselffrom a pinned slot afterw_list_new, which can collect.A repro holding the pickler in an instance attribute, a module global and a list element across two majors died 5/10 runs with
GC BUG: invalid type_id ... site=remember_young_pointer_insert; 0/20 after.gc: range-check the address before dereferencing it in the major marking pathseed_major_rootprobed an arbitrary address before any range check —registered_pyobject_headerfilters only on alignment and a minimum address, then loads the vtable word ataddrand the header ataddr - 8. The major root banks are untypedi64slices (walk_bh_regsover a blackholeregisters_rsized tonum_regswith only the live subset filled;walk_resume_ref_rootsover the in-flight resume image) whose contract is that non-pointer slots pass through unchanged. The minor path still range-checks first indrag_out_root, so this was major-only.incminimark.py _collect_objperforms no such probe: the type system guarantees everyPtr(GcStruct)root is a real GC object. pyre's roots are untyped and the arena range check stands in for that guarantee, so it must precede every dereference. The same probe is dropped fromgrey_child'sheader_bearingarm, which had a second effect: an address it admitted got VISITED set with no pass to clear it (the only reset walksprebuilt_root_objects), so such an object was traced in the first major after it appeared and never again. That arm also returned ahead of theGC BUG: invalid major childdiagnostic.The probe was reaching non-arena picklers because those objects were immortal; the preceding commit makes them managed, so
is_managed_heap_objectanswers for them directly.pickle_ctor_args— the fixture the probe was introduced for — passes 10/10 without it, jitstats unchanged.Verification
Local, on the current base:
pickle_ctor_args10/10, jitstats identical to baseline (loops_compiled=5 bridges_compiled=0 loops_aborted=0 guard_failures=1)mutate_then_raise_caught0 crashes / 40 underMAJIT_STRICT=1frame_lineno_mid_replay_regression.pyPASScargo test -p majit-gc228 passed;cargo test -p pyre-jit-trace --features dynasm337 passedcheck_synthetic.pyALL PASSEDtest_descr,test_pickletools,test_picklebuffer— no regressionsNot run locally: full
check.py(cranelift/wasm backends). Notecheck.pyis red onmainitself at every recent commit, and the two jit-stats fixtures that fail here —exception_args_virtualandlist_length_hint_validate— are named in #970's commit message as deliberately left alone ("re-recording them would disarm the floor"). Thesandbox e2ejob is flaky onmain(pass/fail/pass across three consecutive main commits).— authored by Claude