diff --git a/majit/majit-metainterp/src/trace_ctx.rs b/majit/majit-metainterp/src/trace_ctx.rs index 926d663375c..f52cf11077f 100644 --- a/majit/majit-metainterp/src/trace_ctx.rs +++ b/majit/majit-metainterp/src/trace_ctx.rs @@ -3414,6 +3414,38 @@ impl TraceCtx { self.box_value(elem) } + /// Resolve the array-base `OpRef` (`frame.locals_cells_stack_w`) for a + /// NONSTANDARD virtualizable the way `opimpl_getfield_gc_r` does + /// (`_opimpl_getfield_gc_any_pureornot`, pyjitpl.py): forward the cached + /// field box on a hit, otherwise record the `GetfieldGcR` once and publish + /// it with `getfield_now_known`. + /// + /// Every `getarrayitem_vable` / `setarrayitem_vable` on such a frame has to + /// come through here, because the per-array element cache is keyed by this + /// base `OpRef`. Recording a fresh base per access instead keys each store + /// under an `OpRef` no read ever looks up, so a store neither updates nor + /// invalidates the entry the reads share — a local written after the read + /// that seeded the cache then keeps reading its pre-store value for as long + /// as the base stays cached. + fn nonstandard_vable_array_base(&mut self, vable_opref: OpRef, fdescr: &DescrRef) -> OpRef { + let record_descr = self.vable_array_record_descr(fdescr); + let field_index = record_descr.index(); + if let Some(cached) = self.heapcache_getfield_cached(vable_opref, field_index) { + self.profiler().count_ops( + OpCode::GetfieldGcR, + crate::pyjitpl::counters::HEAPCACHED_OPS, + ); + return cached; + } + self.profiler() + .count_ops(OpCode::GetfieldGcR, crate::counters::OPS); + self.profiler() + .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS); + let op = self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr); + self.heapcache_getfield_now_known(vable_opref, field_index, op); + op + } + /// pyjitpl.py:1167-1172 `opimpl_getfield_vable_i(box, fielddescr, pc)`. /// /// ```text @@ -4025,13 +4057,7 @@ impl TraceCtx { if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, concrete) { // arraybox = self.opimpl_getfield_gc_r(box, fdescr) // return self.opimpl_getarrayitem_gc_i(arraybox, indexbox, adescr) - let record_descr = self.vable_array_record_descr(&fdescr); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::OPS); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS); - let array_opref = - self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr); + let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr); return ( self.vable_getarrayitem_int_descr(array_opref, index, adescr), None, @@ -4103,13 +4129,7 @@ impl TraceCtx { index, adescr.index(), ); - let record_descr = self.vable_array_record_descr(&fdescr); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::OPS); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS); - let array_opref = - self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr); + let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr); let item = self.vable_getarrayitem_ref_descr(array_opref, index, adescr); if let Some(v) = fwd { self.set_opref_concrete(item, v); @@ -4174,13 +4194,7 @@ impl TraceCtx { ) -> (OpRef, Option) { let concrete = self.concrete_of_opref(vable_opref); if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, concrete) { - let record_descr = self.vable_array_record_descr(&fdescr); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::OPS); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS); - let array_opref = - self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr); + let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr); return ( self.vable_getarrayitem_float_descr(array_opref, index, adescr), None, @@ -4244,18 +4258,12 @@ impl TraceCtx { ) -> bool { let vable_concrete = self.concrete_of_opref(vable_opref); if self.is_nonstandard_virtualizable(pc, vable_opref, &fdescr, vable_concrete) { - let record_descr = self.vable_array_record_descr(&fdescr); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::OPS); - self.profiler() - .count_ops(OpCode::GetfieldGcR, crate::counters::RECORDED_OPS); - let array_opref = - self.record_op_with_descr(OpCode::GetfieldGcR, &[vable_opref], record_descr); + let array_opref = self.nonstandard_vable_array_base(vable_opref, &fdescr); self.profiler() .count_ops(OpCode::SetarrayitemGc, crate::counters::OPS); self.profiler() .count_ops(OpCode::SetarrayitemGc, crate::counters::RECORDED_OPS); - self.vable_setarrayitem_descr(array_opref, index, value, adescr); + self.execute_setarrayitem_gc(array_opref, index, value, adescr); return true; } // index = self._get_arrayitem_vable_index(pc, fdescr, indexbox) @@ -4488,6 +4496,29 @@ impl TraceCtx { ) { self.record_op_with_descr(OpCode::SetarrayitemGc, &[array_opref, index, value], descr); } + + /// `execute_setarrayitem_gc(arraydescr, arraybox, indexbox, itembox)` + /// (pyjitpl.py): record the `SETARRAYITEM_GC`, then publish the stored item + /// to the heapcache. `gen_store_back_in_vable` deliberately does NOT come + /// through here — it records the op directly — so the raw recording form + /// stays available as `vable_setarrayitem_descr`. + /// + /// The heapcache write is what keeps a later read of the same slot from + /// forwarding the value the cache was seeded with: the element cache is the + /// only thing a nonstandard virtualizable's reads consult for the stored + /// box, so a store that skips it leaves every subsequent read answering the + /// pre-store value. + fn execute_setarrayitem_gc( + &mut self, + array_opref: OpRef, + index: OpRef, + value: OpRef, + descr: DescrRef, + ) { + let descr_index = descr.index(); + self.vable_setarrayitem_descr(array_opref, index, value, descr); + self.heapcache_setarrayitem(array_opref, index, descr_index, value); + } } #[cfg(test)] diff --git a/pyre/bench/synth/inline_subwalk_mutating_residual.py b/pyre/bench/synth/inline_subwalk_mutating_residual.py index e4feed6b6a8..024e5cc9226 100644 --- a/pyre/bench/synth/inline_subwalk_mutating_residual.py +++ b/pyre/bench/synth/inline_subwalk_mutating_residual.py @@ -1,4 +1,4 @@ -# pyre-check: max-pypy-ratio=200 +# pyre-check: max-pypy-ratio=40 # gh#495 guard: an inlined subwalk whose callee makes an unjournaled mutation # through a nested residual CALL, in the two miss-handling shapes. # diff --git a/pyre/bench/synth/inline_subwalk_property_mutates.py b/pyre/bench/synth/inline_subwalk_property_mutates.py index c417d74d33e..f6c81805acc 100644 --- a/pyre/bench/synth/inline_subwalk_property_mutates.py +++ b/pyre/bench/synth/inline_subwalk_property_mutates.py @@ -1,4 +1,4 @@ -# pyre-check: max-pypy-ratio=80 +# pyre-check: max-pypy-ratio=50 # gh#495 guard: inlined property residual mutates before branch and caught miss. # @property value-returning mutating + try/except-inside-callee raising branch N = 60000 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 610594466b5..00177a20733 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs @@ -413,6 +413,13 @@ pub(crate) fn callee_body_contains_raise(body_code: &[u8]) -> bool { false } +/// Whether a method-form callee body is free of `LoadAttr` residuals. +/// +/// Consulted only by the entries that pass `allow_method_load_attr = false`. +/// A `self.attr` read in the body is what makes it answer `false`, which is the +/// common shape (`def at(self, i): return self.v + i`), so an entry that opts +/// out of the check trades a narrower inline surface for the ability to inline +/// ordinary accessor methods. pub(crate) fn method_form_callee_body_supported( body_code: &[u8], callee_descr_refs: &[DescrRef], @@ -1742,7 +1749,7 @@ pub(crate) fn try_walker_inline_user_call( has_closure, None, None, - false, + true, false, None, ) @@ -2689,6 +2696,12 @@ pub(crate) fn try_walker_inline_resolved_user_call( if !bridge_rec_root_selfrec && fbw_hazardous_inline_denied(callee_code_key) { return Ok(None); } + // True when only the widened method-form surface reaches this callee: an + // unbound `self.attr` accessor body, which the narrow surface declines. + let widened_method_form = method_form + && bound_method.is_none() + && allow_method_load_attr + && !method_form_callee_body_supported(body.code, callee_descr_refs); // A legacy, unseeded inline sub-walk inside a FOR_ITER body resumes a guard // at the caller's CALL boundary, so deopt re-executes the whole callee. // Replaying a live-heap mutation would double it, so a Dirty body stays on @@ -2725,8 +2738,16 @@ pub(crate) fn try_walker_inline_resolved_user_call( // iteration's contribution is dropped, silently. A `Clean` // body is still admitted from there — it has nothing that can // abort. - foriter_deferred_admit = - arg_class_guard.is_none() && !fbw_foriter_deferred_call_denied(callee_code_key); + // + // The widened method-form surface stays out: its deferred call + // is the `self.attr` read's own dispatch, which the lever + // resolves to a builtin rather than a body it can inline, so the + // admission spends the abort and then denies the callee anyway. + // Declining here reaches the same residual call without retiring + // the enclosing loop. + foriter_deferred_admit = arg_class_guard.is_none() + && !widened_method_form + && !fbw_foriter_deferred_call_denied(callee_code_key); foriter_deferred_admit } CalleeReplaySafety::Dirty => { @@ -2758,12 +2779,30 @@ pub(crate) fn try_walker_inline_resolved_user_call( return Ok(None); } } - if method_form - && bound_method.is_none() - && !allow_method_load_attr - && !method_form_callee_body_supported(body.code, callee_descr_refs) - { - return Ok(None); + if method_form && bound_method.is_none() { + // Two surfaces for an unbound method-form callee. The narrow one + // declines any `self.attr` read in the body. The wide one admits it, + // and pays for the reach with a body that raises: the sub-walk records + // into the handler region, and a guard whose resume coordinate lands on + // the `Reraise` needs ref registers the recorded path never wrote + // (`collect_callee_active_boxes`). That decline arrives mid-recording + // on a non-effect-free opcode, so it has no mid-body carrier and the + // whole enclosing loop is discarded. Decline such a body here instead, + // where the call simply stays residual and the loop still compiles. + let declined = if allow_method_load_attr { + callee_body_contains_raise(body.code) + } else { + !method_form_callee_body_supported(body.code, callee_descr_refs) + }; + if declined { + if std::env::var_os("PYRE_FBW_INLINE_DIAG").is_some() { + eprintln!( + "[inline-method-form] decline pc={} allow_load_attr={allow_method_load_attr}", + op.pc + ); + } + return Ok(None); + } } if std::env::var("PYRE_FBW_INLINE_DIAG").is_ok() { let mut pc = 0usize;