diff --git a/pyre/bench/synth/list_append_write_barrier_gc.cranelift.jitstats b/pyre/bench/synth/list_append_write_barrier_gc.cranelift.jitstats index 144b9884a47..07ce39597da 100644 --- a/pyre/bench/synth/list_append_write_barrier_gc.cranelift.jitstats +++ b/pyre/bench/synth/list_append_write_barrier_gc.cranelift.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=5 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1345 +guard_failures=938 internal_compile_panics=0 -loops_aborted=1 -loops_compiled=12 +loops_aborted=2 +loops_compiled=11 diff --git a/pyre/bench/synth/list_append_write_barrier_gc.dynasm.jitstats b/pyre/bench/synth/list_append_write_barrier_gc.dynasm.jitstats index 144b9884a47..07ce39597da 100644 --- a/pyre/bench/synth/list_append_write_barrier_gc.dynasm.jitstats +++ b/pyre/bench/synth/list_append_write_barrier_gc.dynasm.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=5 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1345 +guard_failures=938 internal_compile_panics=0 -loops_aborted=1 -loops_compiled=12 +loops_aborted=2 +loops_compiled=11 diff --git a/pyre/bench/synth/list_append_write_barrier_gc.wasm.jitstats b/pyre/bench/synth/list_append_write_barrier_gc.wasm.jitstats index 144b9884a47..07ce39597da 100644 --- a/pyre/bench/synth/list_append_write_barrier_gc.wasm.jitstats +++ b/pyre/bench/synth/list_append_write_barrier_gc.wasm.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=5 +bridges_compiled=3 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1345 +guard_failures=938 internal_compile_panics=0 -loops_aborted=1 -loops_compiled=12 +loops_aborted=2 +loops_compiled=11 diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index 46854eb2f0b..f7c98faf884 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -961,19 +961,19 @@ pub fn jit_trace_fnaddrs() -> Vec<(&'static str, i64)> { "pyre_object::try_gc_alloc_stable_raw", pyre_object::gc_hook::try_gc_alloc_stable_raw as *const (), ); - // `w_int_gc_alloc` is the collector-heap arm of `w_int_new`, reached from + // `w_int_box_slow` is the allocating tail of `w_int_new`, reached from // inside a descended body whenever a fold boxes an int. Bind the // macro-emitted trampoline rather than the raw fn, for the reason // `prepare_list_ref_store` documents: the raw `(i64) -> *mut PyObject` is // `(i64) -> i32` on wasm32, while the wasm backend types the residual's // `call_indirect` `(i64) -> i64` from the descr alone. - let w_int_gc_alloc: extern "C" fn(i64) -> i64 = - pyre_object::intobject::__majit_call_target_w_int_gc_alloc; + let w_int_box_slow: extern "C" fn(i64) -> i64 = + pyre_object::intobject::__majit_call_target_w_int_box_slow; push_alias_pair( &mut entries, - "pyre_object::intobject::w_int_gc_alloc", - "pyre_object::w_int_gc_alloc", - w_int_gc_alloc as *const (), + "pyre_object::intobject::w_int_box_slow", + "pyre_object::w_int_box_slow", + w_int_box_slow as *const (), ); // `w_type_set_abstract` stores the runtime-mutable `flag_abstract` atomic — a // side effect on per-type state, not a build-time constant, so it carries diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/branch.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/branch.rs index e66ab7c9ae9..c3058299b8f 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/branch.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/branch.rs @@ -332,6 +332,39 @@ pub(crate) fn kept_stack_has_boxed_int_hazard( } } +/// Whether the walk mirror holds any kept operand-stack slot at `target` as the +/// NULL `ConstPtr`. +/// +/// That encoding is ambiguous by construction: it is what a genuine null operand +/// (`PUSH_NULL` ahead of a `CALL`) and an unset vable shadow slot both decode to, +/// which is why `opref_is_null_const_ptr` makes every reader treat it as no value +/// at all. A kept stack carrying one therefore has a slot the snapshot cannot +/// source, and it is a strictly narrower state than an uncovered slot in general +/// — a mirror shorter than the resume depth, or a `NONE` hole, both still resume +/// through the shadow. +pub(crate) fn kept_stack_has_null_const_slot( + frame: &ActiveResumeFrame, + target: usize, + vstack_boxes: &[OpRef], +) -> bool { + let pjc = &frame.0; + if pjc.code_ptr.is_null() { + return false; + } + // SAFETY: the depth twin is a read-only payload table kept alive by the + // frame's `Arc`. + let depth = unsafe { pjc.depth_trivia_for_jitcode_pc(target) }; + let Some(depth) = depth.map(|d| d as usize) else { + return false; + }; + (0..depth).any(|s| { + vstack_boxes + .get(s) + .copied() + .is_some_and(opref_is_null_const_ptr) + }) +} + /// The resume snapshot's live Ref register colors at a kept-stack branch /// guard's not-taken arm, plus the jitcode `num_regs_r` (the const-window /// boundary `n()`). These are exactly the registers the blackhole restores diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs index d59cb290847..b5d4b544bde 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -5842,13 +5842,18 @@ pub(crate) fn try_walker_specialize_seqiter_getitem_next( // `DeferredCall` is admitted alongside `Clean`, and the terminating `raise` // is what makes that necessary: `RaiseVarargs` classifies as a deferred // residual, so a cursor body that ends on one would otherwise never be - // served. The deferred promise holds here — a residual the lever cannot - // inline aborts before executing and denies the callee. This route's own - // entry is a FOR_ITER, which is not a CALL, and it still passes - // `entry_is_call_boundary: true`: `opcode_for_iter` peeks its single - // iterator operand where `opcode_binary_op` pops both of its own, so the - // rewind re-executes this entry from the stack it already had and the - // boundary is nameable. + // served — which is why this route passes `entry_is_call_boundary: true` + // below. The deferred promise holds here: a residual the lever cannot + // inline aborts before executing and denies the callee. + // + // What the shared FOR_ITER gate withholds admission from is an entry + // reached from an operator opcode, because those opcodes POP their + // operands: a rewind that re-executes `BINARY_OP` or `COMPARE_OP` needs + // operands the flush cannot re-materialize and resumes one short. + // `FOR_ITER` only PEEKS, and its single operand is the iterator the walk + // already holds, so re-executing it needs nothing the stack lost. The + // cursor bump below runs only after the callee returns, so the re-executed + // step reads the same index. if !matches!( replay_safety, CalleeReplaySafety::Clean | CalleeReplaySafety::DeferredCall diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index 96f15241254..f14c492d8bb 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -9283,6 +9283,38 @@ fn guarded_branch_core( && gate_frame.as_ref().is_some_and(|f| { kept_stack_has_boxed_int_hazard(f, other_target, ctx.concrete_registers_r) }); + // Hazard (4): a VALID mirror holds a kept operand-stack slot as + // the NULL `ConstPtr`, and the not-taken edge decodes no + // `ref_copy` trampoline. Hazards (1)-(3) admit an uncovered + // slot on the premise that it is an edge-materialized merge temp + // whose value `resolved_recovered` supplies; with no moves + // decoded there is no such source, and a NULL `ConstPtr` is the + // one uncovered state with no fallback either — the encoding is + // both a genuine null operand and what an unset vable shadow + // slot decodes to, so nothing downstream can tell the snapshot + // which it is. The resume then rebuilds a kept slot NULL, and + // when the arm's pending CALL binds that null `match_signature` + // renders it as a missing positional parameter: + // `re/_parser.py` `_parse_sub` evaluates + // `not nested and not items` inside `_parse`'s argument list, so + // both calls' `PUSH_NULL` slots are kept across the + // short-circuit guard and `nested + 1` arrives NULL. This is + // strictly narrower than "the mirror does not cover": a mirror + // shorter than the resume depth, and a `NONE` hole, both still + // resume through the shadow and must keep compiling — declining + // for those instead loses every bridge in + // `bench/synth/attr_cache_invalidation` and turns its 1002 guard + // failures into 4 million. `bhimpl_goto_if_not` has no + // analogue: `consume_boxes` (resume.py) restores every register + // bank by color, which is why suppressing bridge recording + // (`MAJIT_NO_BRIDGE`) makes the same guard correct. + let kept_null_const_slot = ctx.vstack_valid + && gate_frame.as_ref().is_some_and(|f| { + kept_stack_has_null_const_slot(f, other_target, &ctx.vstack_boxes) + }) + && resolved_recovered + .as_deref() + .is_none_or(|moves| moves.is_empty()); // A not-taken arm resuming at an exception-handler-protected // PC carries the kept exception operand (`PUSH_EXC_INFO`'s // Ref) on its operand stack; the handler-entry mirror reseed @@ -9290,7 +9322,7 @@ fn guarded_branch_core( // whole block out), and where the mirror still does not cover, // that kept Ref always also trips Hazard (1)/(2)/(3) — so the // exc-region case needs no decline of its own. - if reads_null_ref || uses_edge_recovery || kept_boxed_int { + if reads_null_ref || uses_edge_recovery || kept_boxed_int || kept_null_const_slot { // Attribute the kept-stack decline to the hazard that // fired and the mirror state behind it, so a corpus run // (`PYRE_FBW_DEBUG_ABORT`) can separate the distinct @@ -9304,7 +9336,8 @@ fn guarded_branch_core( "[decline-why] PERMANENT pc={} other_target={} vstack_valid={} \ subwalk={} mirror_covers_kept={} depth_gt_1={} kept_stack={} \ kept_stack_any_leg={} reads_null_ref={} uses_edge_recovery={} \ - kept_boxed_int={} kept_recovered_nonempty={}", + kept_boxed_int={} kept_null_const_slot={} \ + kept_recovered_nonempty={}", op.pc, other_target, ctx.vstack_valid, @@ -9316,6 +9349,7 @@ fn guarded_branch_core( reads_null_ref, uses_edge_recovery, kept_boxed_int, + kept_null_const_slot, kept_recovered.as_deref().is_some_and(|m| !m.is_empty()), ); } diff --git a/pyre/pyre-object/src/intobject.rs b/pyre/pyre-object/src/intobject.rs index 907a4aee7de..c475427970d 100644 --- a/pyre/pyre-object/src/intobject.rs +++ b/pyre/pyre-object/src/intobject.rs @@ -103,10 +103,52 @@ pub fn w_int_new(value: i64) -> PyObjectRef { let idx = (value - PREBUILTINTFROM) as usize; return (&SMALL_INTS[idx] as *const W_IntObject).cast_mut() as PyObjectRef; } + w_int_box_slow(value) +} + +/// The allocating tail of [`w_int_new`], behind a residualisation boundary +/// (`rlib/jit.py:139 @dont_look_inside`). The collector arm is the +/// `gct_fv_gc_malloc` bracket (`rpython/memory/gctransform/framework.py`) in +/// its alloc-then-init form — take the block, then write the header and +/// payload into it; it falls through to `malloc_typed` when no collector owns +/// the heap. +/// +/// Both arms have a shape no trace can carry, which is why the boundary sits +/// around the pair rather than around either one. A stack-built `W_IntObject` +/// lowers to a `SyntheticTransparentCtor` for its `PyObject` header, whose +/// funcptr constant degrades to a `symbolic_fnaddr` hash; a descending +/// sub-jitcode walk cannot record such a call, so it declines the entire +/// descent — that is what takes `list.pop()`'s fold off the compiled loop. +/// Writing the fields individually instead lands the header's own `ob_type` +/// slot at offset 0, which the wasm backend does not lower faithfully. +/// +/// The boundary is a deviation from `wrapint` +/// (`objspace/std/intobject.py`), which keeps its allocation inline — its own +/// comment there notes the function is inlined into every caller. The +/// orthodox lowering is `new_with_vtable`, which stays in the trace and can be +/// optimised away where the box does not escape. `fuse_boxing_alloc` +/// (`majit-translate` `model.rs`) rewrites exactly this ctor-plus-`FieldWrite` +/// shape into `NewWithVtable`, but instrumented over this tree it fires +/// nowhere: all 134 candidate sites report the vtable unresolved, because +/// `resolve_vtable_addr` reads `HostStaticAddrs.pytypes` and that table is +/// empty in the build-script pipeline the pass runs in. Drop the boundary +/// once the fusion resolves a vtable there. +/// +/// Spelled `*mut PyObject` rather than `PyObjectRef` so the attribute emits +/// its `extern "C"` call trampoline: the macro recognises raw pointers +/// syntactically and declines to emit one for an aliased return type. +#[majit_macros::dont_look_inside] +pub fn w_int_box_slow(value: i64) -> *mut PyObject { if crate::gc_interp::enabled() { - let boxed = w_int_gc_alloc(value); - if !boxed.is_null() { - return boxed; + let raw = crate::gc_hook::try_gc_alloc_stable_raw(W_INT_GC_TYPE_ID, W_INT_OBJECT_SIZE); + if !raw.is_null() { + unsafe { + let p = raw as *mut W_IntObject; + (*p).ob_header.ob_type = &INT_TYPE as *const PyType; + (*p).ob_header.w_class = get_instantiate(&INT_TYPE); + (*p).intval = value; + } + return raw as PyObjectRef; } } crate::lltype::malloc_typed(W_IntObject { @@ -118,40 +160,6 @@ pub fn w_int_new(value: i64) -> PyObjectRef { }) as PyObjectRef } -/// The collector-heap arm of [`w_int_new`]: the `gct_fv_gc_malloc` bracket -/// (`rpython/memory/gctransform/framework.py`) in its alloc-then-init form — -/// take the block, then write the header and payload into it, rather than -/// copying a stack-built struct over it. Returns null when no collector owns -/// the heap, which is the caller's signal to take the `malloc_typed` arm. -/// -/// Residualised (`rlib/jit.py:139 @dont_look_inside`) rather than traced. A -/// stack-built struct would lower to a `SyntheticTransparentCtor` for the -/// `PyObject` header whose funcptr constant degrades to a `symbolic_fnaddr` -/// hash, which a descending sub-jitcode walk cannot record and declines on; -/// writing the fields individually instead lands the header's own `ob_type` -/// slot at offset 0, which the wasm backend does not lower faithfully. The -/// boundary keeps both shapes out of the trace, and costs the arm nothing -/// where it is unreachable: `gc_interp::enabled()` is false on the native -/// backends, whose `malloc_typed` arm is fused into a `NewWithVtable` by the -/// boxing-constructor pass exactly as before. -/// Spelled `*mut PyObject` rather than `PyObjectRef` so the attribute emits -/// its `extern "C"` call trampoline: the macro recognises raw pointers -/// syntactically and declines to emit one for an aliased return type. -#[majit_macros::dont_look_inside] -pub fn w_int_gc_alloc(value: i64) -> *mut PyObject { - let raw = crate::gc_hook::try_gc_alloc_stable_raw(W_INT_GC_TYPE_ID, W_INT_OBJECT_SIZE); - if raw.is_null() { - return crate::PY_NULL; - } - unsafe { - let p = raw as *mut W_IntObject; - (*p).ob_header.ob_type = &INT_TYPE as *const PyType; - (*p).ob_header.w_class = get_instantiate(&INT_TYPE); - (*p).intval = value; - } - raw as PyObjectRef -} - /// Create a W_IntObject bypassing the small-int cache. /// /// Used for int subclass instances that need unique object identity