diff --git a/majit/majit-backend/src/resume_guard_descr.rs b/majit/majit-backend/src/resume_guard_descr.rs index 0f2fe49bebf..ad967e3dfcb 100644 --- a/majit/majit-backend/src/resume_guard_descr.rs +++ b/majit/majit-backend/src/resume_guard_descr.rs @@ -28,9 +28,7 @@ //! `bridge_dispatch_load()` necessarily holds such an `Arc` for the //! borrow lifetime, so drop cannot interleave with the load → retain //! window. -//! - The only background thread spawned by the driver -//! (`jitdriver.rs:762 invalidation_thread`) touches a -//! `Mutex` and never reaches into `ResumeGuardDescr`. +//! - The driver spawns no background thread at all. //! //! These three facts together close the race CodeRabbit and Codex //! flagged on PR #68 (Critical #6/#10/#13). Any future change that diff --git a/majit/majit-metainterp/src/jitdriver.rs b/majit/majit-metainterp/src/jitdriver.rs index 732d1460242..7b0d6cb26db 100644 --- a/majit/majit-metainterp/src/jitdriver.rs +++ b/majit/majit-metainterp/src/jitdriver.rs @@ -1221,14 +1221,6 @@ pub struct JitDriver { /// PyPy JitDriver(is_recursive=True): enables max_unroll_recursion /// for recursive portal calls (pyjitpl.py _opimpl_recursive_call). is_recursive: bool, - /// Shared quasi-immutable notifier for periodic loop invalidation. - /// RPython compile.py:205: loop.quasi_immutable_deps registration. - /// All compiled loops register their invalidation flag here. - /// A background thread periodically calls invalidate() to force - /// GUARD_NOT_INVALIDATED exits in compiled code. - epoch_qmut: std::sync::Arc>, - /// Handle for the background invalidation thread. - _invalidation_thread: Option>, /// Driver-shared `Assembler`: holds the /// `all_liveness` payload (`assembler.py:30`) populated incrementally /// by `__JitMeta::install_canonical_liveness` (canonical entry) and @@ -1358,45 +1350,10 @@ fn install_state_field_fvc(data: &StateFieldFvcData) { impl JitDriver { /// Create a new JitDriver with the given hot-counting threshold. pub fn new(threshold: u32) -> Self { - Self::with_options(threshold, true) - } - - /// Create a new JitDriver, optionally skipping the background timer that - /// periodically invalidates all compiled loops. - /// - /// The periodic invalidation is a portable stand-in for RPython's - /// GC/signal-triggered invalidation, used by quasi-immutable-bearing - /// consumers (a Python JIT). A consumer with no quasi-immutable state — e.g. - /// a fixed-bytecode interpreter over plain integer reds — has nothing to - /// invalidate; for it the timer only forces pointless re-tracing (and - /// exercises the GUARD_NOT_INVALIDATED resume path needlessly), so it should - /// pass `periodic_invalidation = false`. - pub fn with_options(threshold: u32, periodic_invalidation: bool) -> Self { let mut meta = MetaInterp::new(threshold); if let Some(info) = S::__build_virtualizable_info() { meta.set_virtualizable_info(info); } - let epoch_qmut = - std::sync::Arc::new(std::sync::Mutex::new(crate::quasiimmut::QuasiImmut::new())); - // Background thread: periodically invalidate all registered loops. - // RPython uses GC/signal-triggered invalidation; we use a timer as - // a portable equivalent. Period matches PyPy's checkinterval (~10ms). - #[cfg(not(target_arch = "wasm32"))] - let invalidation_thread = if periodic_invalidation { - let qmut = epoch_qmut.clone(); - Some(std::thread::spawn(move || { - loop { - std::thread::sleep(std::time::Duration::from_millis(50)); - if let Ok(mut qmut) = qmut.lock() { - if qmut.has_watchers() { - qmut.invalidate(); - } - } - } - })) - } else { - None - }; JitDriver { meta, sym: None, @@ -1409,11 +1366,6 @@ impl JitDriver { bridge_body_start_op_count: None, entry_points: Vec::new(), is_recursive: false, - epoch_qmut, - #[cfg(not(target_arch = "wasm32"))] - _invalidation_thread: invalidation_thread, - #[cfg(target_arch = "wasm32")] - _invalidation_thread: None, blackhole_allocator: None, portal_runner: None, portal_jd_index: None, @@ -6332,17 +6284,6 @@ impl JitDriver { } pre_run(); - // RPython compile.py:205-207: register loop token with - // quasi-immutable deps so the background invalidation thread - // can force GUARD_NOT_INVALIDATED exits periodically. - if let Some(token) = self.meta.get_loop_token(key_hash) { - if let Ok(mut qmut) = self.epoch_qmut.lock() { - for flag in token.all_invalidation_flags() { - qmut.register(&flag); - } - } - } - loop { let ( is_finish, diff --git a/majit/majit-metainterp/src/lib.rs b/majit/majit-metainterp/src/lib.rs index 8c763cbfef5..5ee5ac50cbe 100644 --- a/majit/majit-metainterp/src/lib.rs +++ b/majit/majit-metainterp/src/lib.rs @@ -89,7 +89,6 @@ pub mod optimize; pub mod optimizeopt; pub(crate) mod parity; mod pyjitpl; -pub mod quasiimmut; pub mod recorder; pub mod resoperation; pub mod resume; @@ -160,7 +159,6 @@ pub use pyjitpl::{ set_record_inline_application_traceback_hook, struct_fields_write_effect_info, trace_jitcode, trace_jitcode_from_merge_point, trace_jitcode_with_args, trace_jitcode_with_args_and_runtime, }; -pub use quasiimmut::QuasiImmut; pub use resume_box_reader::{ BridgeVirtualCache, decode_fieldnum, default_bridge_array_descr, emit_pending_field_op, materialize_bridge_virtual, rebuilt_value_to_opref, replay_pending_fields, diff --git a/majit/majit-metainterp/src/optimizeopt/heap.rs b/majit/majit-metainterp/src/optimizeopt/heap.rs index 948e5d78fc5..a12342e09ea 100644 --- a/majit/majit-metainterp/src/optimizeopt/heap.rs +++ b/majit/majit-metainterp/src/optimizeopt/heap.rs @@ -889,6 +889,68 @@ impl OptHeap { descr_identity(descr) } + /// `quasiimmut.py:147-159 QuasiImmutDescr.is_still_valid_for`, the check + /// `heap.py:802-804` turns into `InvalidLoop('quasi immutable field changed + /// during tracing')`. + /// + /// ```text + /// def is_still_valid_for(self, structconst): + /// assert self.struct + /// if self.struct != structconst.getref_base(): + /// return False + /// qmut = get_current_qmut_instance(cpu, self.struct, + /// self.mutatefielddescr) + /// if qmut is not self.qmut: + /// return False + /// else: + /// currentbox = self.get_current_constant_fieldvalue() + /// assert self.constantfieldbox.same_constant(currentbox) + /// return True + /// ``` + /// + /// Upstream detects the change through the `qmut` object's identity — + /// invalidation nulls the hidden `mutate_*` field, so + /// `get_current_qmut_instance` hands back a fresh instance and the `is not` + /// test fires; the field-value comparison is the assert that backs it up. + /// Pyre has no per-read `QuasiImmutDescr` to hang that identity on, so the + /// value comparison is the test itself: the tracer captured the field on + /// `arg(1)` (`state::current_quasiimmut_field_value`) and a live re-read + /// through `get_runtime_field` is `get_current_constant_fieldvalue`. + /// + /// Returns `true` — keep the loop — whenever the comparison cannot be made: + /// a struct that did not fold to a constant is the case heap.py:794-796 + /// ignores outright, and an op without the captured value is the namespace + /// twin, which carries a slot index there instead. + fn quasiimmut_field_still_valid( + op: &Op, + obj: OpRef, + descr: &DescrRef, + ctx: &mut OptContext, + ) -> bool { + if op.num_args() < 2 { + return true; + } + let Some(constantfieldbox) = op.arg(1).const_value() else { + return true; + }; + // heap.py:794-796 `if not structvalue.is_constant(): return`. + if ctx + .get_box_replacement_operand_opt(obj) + .and_then(|b| ctx.get_constant_ptr_box(&b)) + .is_none() + { + return true; + } + let Some(currentbox) = ctx + .get_runtime_field(obj, descr) + .and_then(|r| r.inline_const_to_value()) + else { + return true; + }; + // history.py:204 `Const.same_constant`. + currentbox == constantfieldbox + } + /// Compute the `PtrInfo._fields` slot for a field descriptor. /// /// RPython uses `descr.get_index()` only for `info._fields[index]` @@ -3239,23 +3301,29 @@ impl OptHeap { // already emitted one via generate_guard (pyjitpl.py:1087). // Records quasi_immutable_deps for invalidation tracking. let obj = op.arg(0).to_opref(); - // RPython optimize_QUASIIMMUT_FIELD: collect quasi-immutable - // dependencies. Add (obj_ptr, field_idx) to quasi_immutable_deps - // for per-slot watcher registration after compilation. - // field_idx comes from descr (GC object fields) or arg(1) - // (namespace slot index). - let (dep_field_idx, cache_field_key) = if let Some(descr) = op.getdescr() { - ( - Some(Self::field_effect_index(&descr)), + // heap.py:798-804 — the field can have changed between the + // tracer reading it and this pass running. Abandon the loop + // when it has; the traced value is baked in as a constant and + // nothing downstream will re-prove it. + if let Some(descr) = op.getdescr() { + if !Self::quasiimmut_field_still_valid(op, obj, &descr, ctx) { + return OptimizationResult::InvalidLoop( + "quasi immutable field changed during tracing", + ); + } + } + // heap.py:807-809 `self.optimizer.quasi_immutable_deps[ + // qmutdescr.qmut] = None`. Upstream keys the set on the + // `QuasiImmut` instance the descr already resolved; pyre records + // the pair that identifies it — the owning object and which of + // its quasi-immutable fields — and `register_quasi_immutable_deps` + // resolves the instance after compilation. + let (dep_field_idx, cache_field_key) = match op.getdescr() { + Some(descr) => ( + Some(descr.index()), Some(Self::field_cache_identity(&descr)), - ) - } else if op.num_args() > 1 { - let idx = ctx - .get_constant_int_box(&op.arg(1).get_box_replacement(false)) - .map(|v| v as u32); - (idx, idx.map(|v| v as usize)) - } else { - (None, None) + ), + None => (None, None), }; if let Some(idx) = dep_field_idx { // The quasi-immutable dependency object (namespace dict / diff --git a/majit/majit-metainterp/src/quasiimmut.rs b/majit/majit-metainterp/src/quasiimmut.rs deleted file mode 100644 index 596eb8e8cbd..00000000000 --- a/majit/majit-metainterp/src/quasiimmut.rs +++ /dev/null @@ -1,437 +0,0 @@ -use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::{Arc, Mutex, Weak}; - -/// quasiimmut.py: get_mutate_field_name(fieldname). -pub fn get_mutate_field_name(fieldname: &str) -> String { - if let Some(rest) = fieldname.strip_prefix("inst_") { - format!("mutate_{rest}") - } else { - panic!("{fieldname}") - } -} - -/// quasiimmut.py: get_current_qmut_instance(cpu, gcref, mutatefielddescr). -/// -/// PyPy stores the `QuasiImmut` object in the object's mutate field via CPU -/// descriptor reads/writes. Pyre's current runtime-facing shape represents -/// that mutate field as a Rust cell; this helper preserves the same -/// get-or-create semantics without inventing a side table. -pub fn get_current_qmut_instance( - mutate_field: &Mutex>>>, -) -> Arc> { - let mut field = mutate_field - .lock() - .expect("quasi-immutable mutate field mutex poisoned"); - if let Some(qmut) = field.as_ref() { - return qmut.clone(); - } - let qmut = Arc::new(Mutex::new(QuasiImmut::new())); - *field = Some(qmut.clone()); - qmut -} - -/// quasiimmut.py: make_invalidation_function(STRUCT, mutatefieldname). -/// -/// The returned closure mirrors PyPy's invalidation function: if the mutate -/// field currently holds a `QuasiImmut`, clear the field and invalidate it. -pub fn make_invalidation_function( - mutate_field: Arc>>>>, -) -> impl Fn() + Send + Sync + 'static { - move || { - let qmut = mutate_field - .lock() - .expect("quasi-immutable mutate field mutex poisoned") - .take(); - if let Some(qmut) = qmut { - qmut.lock() - .expect("quasi-immutable instance mutex poisoned") - .invalidate(); - } - } -} - -/// Notifier for quasi-immutable fields. -/// -/// When a quasi-immutable field changes, call `invalidate()` to mark -/// all compiled loops that depend on this field's value as invalid. -/// The next time a `GuardNotInvalidated` check runs in those loops, -/// it will fail and execution will fall back to the interpreter. -pub struct QuasiImmut { - /// Weak references to JitCellToken invalidation flags. - watchers: Vec>, - /// quasiimmut.py: compress_limit — threshold for compressing dead refs. - compress_limit: usize, - /// Statistics: total number of invalidations performed. - pub invalidation_count: u64, -} - -impl std::fmt::Debug for QuasiImmut { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("QuasiImmut") - .field("num_watchers", &self.num_watchers()) - .finish() - } -} - -impl QuasiImmut { - pub fn new() -> Self { - Self { - watchers: Vec::new(), - compress_limit: 30, - invalidation_count: 0, - } - } - - /// Register a compiled loop's invalidation flag. - /// quasiimmut.py: register_loop_token(wref) - pub fn register(&mut self, flag: &Arc) { - if self.watchers.len() > self.compress_limit { - self.compress(); - } - self.watchers.push(Arc::downgrade(flag)); - } - - /// quasiimmut.py: compress_looptokens_list() - /// Remove dead weak references and update compress_limit. - pub fn compress(&mut self) { - self.watchers.retain(|w| w.strong_count() > 0); - self.compress_limit = (self.watchers.len() + 15) * 2; - } - - /// Invalidate all registered loops. - /// quasiimmut.py: invalidate(descr_repr) - pub fn invalidate(&mut self) { - let mut invalidated = 0u64; - for watcher in &self.watchers { - if let Some(flag) = watcher.upgrade() { - invalidated += 1; - flag.store(true, Ordering::Release); - } - } - self.invalidation_count += invalidated; - self.watchers.clear(); - } - - /// Number of live watchers. - pub fn num_watchers(&self) -> usize { - self.watchers - .iter() - .filter(|w| w.strong_count() > 0) - .count() - } - - /// Check if any watchers are still alive. - pub fn has_watchers(&self) -> bool { - self.watchers.iter().any(|w| w.strong_count() > 0) - } - - /// Remove all dead references without invalidating. - pub fn cleanup(&mut self) { - self.watchers.retain(|w| w.strong_count() > 0); - } -} - -impl Default for QuasiImmut { - fn default() -> Self { - Self::new() - } -} - -/// quasiimmut.py: QuasiImmutDescr — descriptor binding a field to a QuasiImmut. -/// Associates a specific object field with a quasi-immutable notifier -/// and the cached constant value. -#[derive(Clone, Debug)] -pub struct QuasiImmutDescr { - /// quasiimmut.py:121 `self.struct` — the object whose field is - /// quasi-immutable. - pub obj_ref: u64, - /// quasiimmut.py:122 `self.fielddescr` — the field descriptor index. - pub field_descr_idx: u32, - /// quasiimmut.py:125 `self.constantfieldbox` — the cached constant value - /// (snapshot at descr creation). - pub cached_value: i64, - /// quasiimmut.py:123 `self.mutatefielddescr` — a handle to the object's - /// mutate field (read upstream via `cpu.bh_getfield_gc_r(struct, - /// mutatefielddescr)`), modeled as the clearable cell that holds the - /// current `QuasiImmut` (or NULL). This cell is owned by the object, so - /// every descriptor for the same `(struct, mutatefielddescr)` is built - /// from the SAME cell and shares one `QuasiImmut`; the caller supplies it. - /// `do_force_quasi_immutable` clears this cell before invalidating. - pub mutate_field: Arc>>>>, - /// quasiimmut.py:124 `self.qmut` — the `QuasiImmut` captured at descr - /// creation; `is_still_valid_for` compares the field's current qmut - /// against this identity. - pub qmut: Arc>, -} - -impl QuasiImmutDescr { - /// quasiimmut.py:119-125 `QuasiImmutDescr.__init__` — capture the current - /// QuasiImmut instance from the object's mutate field. `mutate_field` is - /// the object's shared cell (`get_current_qmut_instance(cpu, struct, - /// mutatefielddescr)`), so two descriptors for the same field share a qmut. - pub fn new( - obj_ref: u64, - field_descr_idx: u32, - cached_value: i64, - mutate_field: Arc>>>>, - ) -> Self { - let qmut = get_current_qmut_instance(&mutate_field); - QuasiImmutDescr { - obj_ref, - field_descr_idx, - cached_value, - mutate_field, - qmut, - } - } - - /// Register a compiled loop that depends on this quasi-immutable value. - /// quasiimmut.py: `descr.qmut.register_loop_token(wref)`. - pub fn register_loop(&self, flag: &Arc) { - if let Ok(mut qi) = self.qmut.lock() { - qi.register(flag); - } - } - - /// quasiimmut.py: get_parent_descr() - /// Return the field descriptor index. - pub fn get_parent_descr(&self) -> u32 { - self.field_descr_idx - } - - /// quasiimmut.py: get_index() - /// Return the descriptor index (delegates to field_descr_idx). - pub fn get_index(&self) -> u32 { - self.field_descr_idx - } - - /// quasiimmut.py: get_current_constant_fieldvalue() - /// - /// Read the current value of the quasi-immutable field from the - /// concrete object. Returns the raw value at the field offset. - pub fn get_current_constant_fieldvalue(&self, field_offset: usize) -> i64 { - if self.obj_ref == 0 { - return 0; - } - unsafe { *((self.obj_ref as *const u8).add(field_offset) as *const i64) } - } - - /// quasiimmut.py:146-158 is_still_valid_for(structconst) - /// - /// Same object identity, same mutate-field qmut identity, AND same field - /// value as cached. After `do_force_quasi_immutable` clears the mutate - /// field, `get_current_qmut_instance` mints a fresh `QuasiImmut`, so the - /// identity check below fails and the descriptor is no longer valid. - pub fn is_still_valid_for(&self, struct_ref: u64, field_offset: usize) -> bool { - if self.obj_ref != struct_ref { - return false; - } - let qmut = get_current_qmut_instance(&self.mutate_field); - if !Arc::ptr_eq(&qmut, &self.qmut) { - return false; - } - let current = self.get_current_constant_fieldvalue(field_offset); - current == self.cached_value - } -} - -/// quasiimmut.py:46-51 do_force_quasi_immutable(cpu, p, mutatefielddescr) -/// -/// Read the mutate field; if it holds a `QuasiImmut`, clear the field (set to -/// NULL) BEFORE invalidating all dependent compiled loops. Clearing first is -/// what makes a later `is_still_valid_for` see a fresh qmut and fail. -/// -/// Called by the interpreter when a quasi-immutable field is written. -pub fn do_force_quasi_immutable(descr: &QuasiImmutDescr) { - let qmut = descr - .mutate_field - .lock() - .expect("quasi-immutable mutate field mutex poisoned") - .take(); - if let Some(qmut) = qmut { - qmut.lock() - .expect("quasi-immutable instance mutex poisoned") - .invalidate(); - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_get_mutate_field_name() { - assert_eq!(get_mutate_field_name("inst_value"), "mutate_value"); - } - - #[test] - #[should_panic(expected = "value")] - fn test_get_mutate_field_name_rejects_non_instance_field() { - get_mutate_field_name("value"); - } - - #[test] - fn test_get_current_qmut_instance_reuses_mutate_field() { - let mutate_field = Mutex::new(None); - let qmut1 = get_current_qmut_instance(&mutate_field); - let qmut2 = get_current_qmut_instance(&mutate_field); - assert!(Arc::ptr_eq(&qmut1, &qmut2)); - } - - #[test] - fn test_make_invalidation_function_clears_and_invalidates() { - let mutate_field = Arc::new(Mutex::new(None)); - let qmut = get_current_qmut_instance(&mutate_field); - let flag = Arc::new(AtomicBool::new(false)); - qmut.lock().unwrap().register(&flag); - - let invalidate = make_invalidation_function(mutate_field.clone()); - invalidate(); - - assert!(flag.load(Ordering::Acquire)); - assert!(mutate_field.lock().unwrap().is_none()); - } - - #[test] - fn test_register_and_invalidate() { - let mut qi = QuasiImmut::new(); - let flag1 = Arc::new(AtomicBool::new(false)); - let flag2 = Arc::new(AtomicBool::new(false)); - let flag3 = Arc::new(AtomicBool::new(false)); - - qi.register(&flag1); - qi.register(&flag2); - qi.register(&flag3); - assert_eq!(qi.num_watchers(), 3); - - qi.invalidate(); - assert!(flag1.load(Ordering::Acquire)); - assert!(flag2.load(Ordering::Acquire)); - assert!(flag3.load(Ordering::Acquire)); - } - - #[test] - fn test_dead_refs_cleaned() { - let mut qi = QuasiImmut::new(); - let flag1 = Arc::new(AtomicBool::new(false)); - qi.register(&flag1); - - { - let flag2 = Arc::new(AtomicBool::new(false)); - qi.register(&flag2); - } - // flag2 is dropped - - // quasiimmut.py: compress_looptokens_list() removes dead refs - qi.compress(); - assert_eq!(qi.num_watchers(), 1); // dead ref removed - - qi.invalidate(); - assert!(flag1.load(Ordering::Acquire)); - // quasiimmut.py: invalidate() clears the list - assert_eq!(qi.num_watchers(), 0); - } - - #[test] - fn test_multiple_invalidations() { - let mut qi = QuasiImmut::new(); - let flag = Arc::new(AtomicBool::new(false)); - qi.register(&flag); - - qi.invalidate(); - assert!(flag.load(Ordering::Acquire)); - - // Reset and re-register - flag.store(false, Ordering::Release); - qi.register(&flag); - qi.invalidate(); - assert!(flag.load(Ordering::Acquire)); - } - - #[test] - fn test_quasi_immut_descr() { - let mutate_field = Arc::new(Mutex::new(None)); - let descr = QuasiImmutDescr::new(0x1000, 42, 99, mutate_field); - assert_eq!(descr.obj_ref, 0x1000); - assert_eq!(descr.field_descr_idx, 42); - assert_eq!(descr.cached_value, 99); - - // Register and force through the descr - let flag = Arc::new(AtomicBool::new(false)); - descr.register_loop(&flag); - do_force_quasi_immutable(&descr); - assert!(flag.load(Ordering::Acquire)); - // The mutate field is cleared, so the descr is no longer valid. - assert!(!descr.is_still_valid_for(0x1000, 0)); - } - - #[test] - fn test_quasi_immut_descr_shares_object_mutate_field() { - // Two descriptors for the same object field share the mutate-field - // cell, hence the same QuasiImmut (quasiimmut.py:124 via - // get_current_qmut_instance reading the object's field). - let mutate_field = Arc::new(Mutex::new(None)); - let descr1 = QuasiImmutDescr::new(0x3000, 7, 1, mutate_field.clone()); - let descr2 = QuasiImmutDescr::new(0x3000, 7, 1, mutate_field); - assert!(Arc::ptr_eq(&descr1.qmut, &descr2.qmut)); - - // Forcing through one clears the shared field, invalidating both. - let f1 = Arc::new(AtomicBool::new(false)); - let f2 = Arc::new(AtomicBool::new(false)); - descr1.register_loop(&f1); - descr2.register_loop(&f2); - do_force_quasi_immutable(&descr1); - assert!(f1.load(Ordering::Acquire)); - assert!(f2.load(Ordering::Acquire)); - assert!(!descr2.is_still_valid_for(0x3000, 0)); - } - - #[test] - fn test_has_watchers() { - let mut qi = QuasiImmut::new(); - assert!(!qi.has_watchers()); - - let flag = Arc::new(AtomicBool::new(false)); - qi.register(&flag); - assert!(qi.has_watchers()); - - drop(flag); - qi.cleanup(); - assert!(!qi.has_watchers()); - } - - #[test] - fn test_quasi_immut_descr_multi_loop() { - let mutate_field = Arc::new(Mutex::new(None)); - let descr = QuasiImmutDescr::new(0x2000, 10, 55, mutate_field); - let f1 = Arc::new(AtomicBool::new(false)); - let f2 = Arc::new(AtomicBool::new(false)); - descr.register_loop(&f1); - descr.register_loop(&f2); - // Force the mutate field — both flags should be set. - do_force_quasi_immutable(&descr); - assert!(f1.load(Ordering::Acquire)); - assert!(f2.load(Ordering::Acquire)); - } - - #[test] - fn test_num_watchers_after_invalidate() { - let mut qi = QuasiImmut::new(); - let f1 = Arc::new(AtomicBool::new(false)); - let f2 = Arc::new(AtomicBool::new(false)); - qi.register(&f1); - qi.register(&f2); - assert_eq!(qi.num_watchers(), 2); - qi.invalidate(); - // quasiimmut.py: invalidate() clears the watcher list. - assert_eq!(qi.num_watchers(), 0); - assert_eq!(qi.invalidation_count, 2); - } - - #[test] - fn test_debug_format() { - let qi = QuasiImmut::new(); - let debug = format!("{:?}", qi); - assert!(debug.contains("QuasiImmut")); - } -} diff --git a/majit/majit-trace/src/heapcache.rs b/majit/majit-trace/src/heapcache.rs index ed073b3471b..01e1e39b226 100644 --- a/majit/majit-trace/src/heapcache.rs +++ b/majit/majit-trace/src/heapcache.rs @@ -349,10 +349,6 @@ pub struct HeapCache { /// traces — not a ref. RPython: CacheEntry 내부. Vec indexed by OpRef.0. known_class: Vec>, - /// Quasi-immutable fields known in this trace. - /// heapcache.py: `quasi_immut_known`. - quasi_immut_known: IndexSet<(OpRef, u32)>, - /// RPython: FrontendOp flag. BitSet indexed by OpRef.0. is_unescaped: BitSet, @@ -415,7 +411,6 @@ impl HeapCache { heap_cache: vecset::VecMap::new(), heap_array_cache: vecset::VecMap::new(), known_class: Vec::new(), - quasi_immut_known: IndexSet::new(), is_unescaped: BitSet::new(), seen_allocation: BitSet::new(), known_nullity: Vec::new(), @@ -1051,7 +1046,7 @@ impl HeapCache { /// emitted into the op-graph (`replaced_with_const` / /// `loopinvariant_result` / `CacheEntry` field values), so a stale one /// is a use-after-move. The `cache_anything` / `cache_seen_allocation` - /// / `quasi_immut_known` *keys* are deliberately left stale: a forwarded + /// / `quasiimmut_seen_refs` *keys* are deliberately left stale: a forwarded /// lookup key simply misses the stale-keyed entry and the cache /// repopulates (same contract as the `call_pure_results` cache), and an /// in-place key rewrite would break the sorted-`VecMap` ordering. @@ -1756,16 +1751,83 @@ impl HeapCache { } } - // ── Quasi-immutable tracking (RPython heapcache.py quasi_immut_known) ── + // ── Quasi-immutable tracking (heapcache.py:604-627) ── - /// Record that a quasi-immutable field is known. - pub fn quasi_immut_now_known(&mut self, obj: OpRef, field_index: u32) { - self.quasi_immut_known.insert((obj, field_index)); + /// The `quasiimmut_seen_refs` key: `box.getref_base()` + /// (heapcache.py:609/622). Upstream reads the raw GC pointer off the + /// `ConstPtr` box; pyre's constant `OpRef` carries that pointer inline + /// (`OpRef::const_ptr(GcRef)`), so `inline_const_bits` is the same read. + fn quasiimmut_seen_ref_key(obj: OpRef) -> usize { + obj.inline_const_bits().unwrap_or(0) as usize } - /// Check if a quasi-immutable field is already known. - pub fn is_quasi_immut_known(&self, obj: OpRef, field_index: u32) -> bool { - self.quasi_immut_known.contains(&(obj, field_index)) + /// heapcache.py:604-613 + /// + /// ```text + /// def is_quasi_immut_known(self, fielddescr, box): + /// cache = self.heap_cache.get(fielddescr, None) + /// if cache is not None: + /// if isinstance(box, Const): + /// if cache.quasiimmut_seen_refs is not None: + /// return box.getref_base() in cache.quasiimmut_seen_refs + /// else: + /// if cache.quasiimmut_seen is not None: + /// return box in cache.quasiimmut_seen + /// return False + /// ``` + /// + /// The two sets live on the per-descr [`CacheEntry`], so + /// `_clear_cache_on_write` and `_invalidate_unescaped` clear them + /// alongside the value caches. That lifetime is load-bearing: a + /// residual call goes through `clear_caches_varargs` + /// (heapcache.py:341-370), which both arms + /// `need_guard_not_invalidated` and drops the "already marked" bit, so + /// the next read of the field re-emits `QUASIIMMUT_FIELD` and that op + /// in turn emits the second `GUARD_NOT_INVALIDATED`. + pub fn is_quasi_immut_known(&self, field_index: u32, obj: OpRef) -> bool { + let Some(cache) = self.heap_cache.get(&field_index) else { + return false; + }; + if obj.is_constant() { + if let Some(seen) = &cache.quasiimmut_seen_refs { + return seen.contains(&Self::quasiimmut_seen_ref_key(obj)); + } + } else if let Some(seen) = &cache.quasiimmut_seen { + return seen.contains(&obj); + } + false + } + + /// heapcache.py:615-627 + /// + /// ```text + /// def quasi_immut_now_known(self, fielddescr, box): + /// cache = self.heap_cache.get(fielddescr, None) + /// if cache is None: + /// cache = self.heap_cache[fielddescr] = CacheEntry(self) + /// if isinstance(box, Const): + /// if cache.quasiimmut_seen_refs is None: + /// cache.quasiimmut_seen_refs = new_ref_dict() + /// cache.quasiimmut_seen_refs[box.getref_base()] = None + /// else: + /// if cache.quasiimmut_seen is not None: + /// cache.quasiimmut_seen[box] = None + /// else: + /// cache.quasiimmut_seen = {box: None} + /// ``` + pub fn quasi_immut_now_known(&mut self, field_index: u32, obj: OpRef) { + let cache = self.heap_cache.entry(field_index).or_default(); + if obj.is_constant() { + cache + .quasiimmut_seen_refs + .get_or_insert_with(IndexSet::new) + .insert(Self::quasiimmut_seen_ref_key(obj)); + } else { + cache + .quasiimmut_seen + .get_or_insert_with(IndexSet::new) + .insert(obj); + } } // ── Nullity tracking (heapcache.py nullity_now_known / is_nullity_known) ── @@ -2038,7 +2100,6 @@ impl HeapCache { // majit-only: standalone Vec flags are not version-gated, so // a version bump cannot invalidate them. Clear them explicitly. self.known_class.clear(); - self.quasi_immut_known.clear(); self.is_unescaped = BitSet::new(); self.seen_allocation = BitSet::new(); self.known_nullity.clear(); @@ -2514,6 +2575,59 @@ mod tests { assert_eq!(cache.is_nullity_known(obj, |_| None), Some(false)); } + /// heapcache.py:604-627 — the mark is per (fielddescr, box), and a + /// constant receiver keys on `getref_base()`, so two distinct `ConstPtr` + /// `OpRef`s naming the same object share it while a different object or a + /// different descr does not. + #[test] + fn quasi_immut_mark_is_keyed_per_descr_and_per_ref() { + let mut cache = HeapCache::new(); + let obj = OpRef::const_ptr(GcRef(0x1000)); + let same_obj = OpRef::const_ptr(GcRef(0x1000)); + let other_obj = OpRef::const_ptr(GcRef(0x2000)); + + assert!(!cache.is_quasi_immut_known(7, obj)); + cache.quasi_immut_now_known(7, obj); + assert!(cache.is_quasi_immut_known(7, obj)); + assert!(cache.is_quasi_immut_known(7, same_obj)); + assert!(!cache.is_quasi_immut_known(7, other_obj)); + assert!(!cache.is_quasi_immut_known(8, obj)); + } + + /// heapcache.py:121-129 `invalidate_unescaped` clears + /// `quasiimmut_seen{,_refs}` — the lifetime that makes the second + /// `GUARD_NOT_INVALIDATED` possible. `clear_caches_varargs` + /// (heapcache.py:341-370) runs this for every general call while also + /// arming `need_guard_not_invalidated`, so the next read of the field + /// re-emits `QUASIIMMUT_FIELD` and that op emits the guard. + /// + /// The receiver here is an escaped (non-allocated) box, which is what a + /// pinned type constant is. + #[test] + fn quasi_immut_mark_is_dropped_by_the_call_invalidation() { + let mut cache = HeapCache::new(); + let obj = OpRef::const_ptr(GcRef(0x1000)); + + cache.quasi_immut_now_known(7, obj); + assert!(cache.is_quasi_immut_known(7, obj)); + + cache.invalidate_caches_for_escaped(); + assert!(!cache.is_quasi_immut_known(7, obj)); + } + + /// heapcache.py:70-77 `_clear_cache_on_write` clears the same two sets, so + /// a store to the field drops the mark as well. + #[test] + fn quasi_immut_mark_is_dropped_by_a_store_to_the_field() { + let mut cache = HeapCache::new(); + let obj = OpRef::const_ptr(GcRef(0x1000)); + let value = OpRef::int_op(3); + + cache.quasi_immut_now_known(7, obj); + cache.setfield_cached(obj, 7, value, IDENTITY_ORACLE); + assert!(!cache.is_quasi_immut_known(7, obj)); + } + #[test] fn test_arraylen_caching() { let mut cache = HeapCache::new(); diff --git a/pyre/pyre-interpreter/src/jit_fnaddr.rs b/pyre/pyre-interpreter/src/jit_fnaddr.rs index ac0baa20bef..e11cc52ec3b 100644 --- a/pyre/pyre-interpreter/src/jit_fnaddr.rs +++ b/pyre/pyre-interpreter/src/jit_fnaddr.rs @@ -1886,11 +1886,12 @@ pub fn jit_trace_fnaddrs() -> Vec<(&'static str, i64)> { "pyre_object::next_version_tag_serial", next_version_tag_serial as *const (), ); + // `quasiimmut.py:129-134 _invalidate_now`, shared by both `?` fields. push_alias_pair( &mut entries, - "pyre_object::celldict::sweep_version_watchers", - "pyre_object::sweep_version_watchers", - pyre_object::celldict::sweep_version_watchers as *const (), + "pyre_object::quasiimmut::sweep_quasi_immut_field", + "pyre_object::sweep_quasi_immut_field", + pyre_object::quasiimmut::sweep_quasi_immut_field as *const (), ); // The three typed-storage promotions: `IndexMap` construction and refill // end to end, so the residual boundary is the whole migration. diff --git a/pyre/pyre-jit-trace/src/descr.rs b/pyre/pyre-jit-trace/src/descr.rs index ab37ff06063..b8f3211d577 100644 --- a/pyre/pyre-jit-trace/src/descr.rs +++ b/pyre/pyre-jit-trace/src/descr.rs @@ -2214,6 +2214,36 @@ pub fn type_version_tag_descr() -> DescrRef { TYPE_VERSION_TAG_FIELD_DESCR.clone() } +/// `celldict.py:32 ModuleDictStrategy.version` — the module-namespace version +/// tag (`u64`, 8 bytes, unsigned) on the strategy box. +/// +/// Quasi-immutable, per `celldict.py:34 _immutable_fields_ = ["version?"]`, +/// which is the same declaration `getdictvalue_no_unwrapping` promotes before +/// its elidable lookup (`celldict.py:47-55`). The `LOAD_GLOBAL` / `STORE_GLOBAL` +/// cell folds bake the slot's stored cell as a `ConstPtr` under a +/// `QUASIIMMUT_FIELD` on this field: `_setitem_str_cell_known` +/// (`celldict.py:80-90`) calls `mutated()` before every write that replaces the +/// stored pointer, and an in-place cell write leaves the pointer alone, so the +/// version is exactly the datum that proves the baked address still stands. +/// +/// `offset_of!` rather than a literal because `ModuleDictStrategy` is not +/// `repr(C)`. +/// +/// One object per run, for the reason spelled out on +/// [`TYPE_VERSION_TAG_FIELD_DESCR`]. +static MODULE_DICT_VERSION_FIELD_DESCR: LazyLock = LazyLock::new(|| { + make_quasi_immutable_field_descr( + core::mem::offset_of!(pyre_object::celldict::ModuleDictStrategy, version), + 8, + Type::Int, + false, + ) +}); + +pub fn module_dict_version_descr() -> DescrRef { + MODULE_DICT_VERSION_FIELD_DESCR.clone() +} + /// `W_ObjectObject` SizeDescr group (`objectobject.rs:34-46`) — the instance /// layout `[ob_type | w_class | map | storage]`. Built with a parent SizeDescr /// (unlike a bare [`make_field_descr`]) so a `getfield_gc` on `map` / `storage` diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index fad1e814d05..2b5c51f968f 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -7278,6 +7278,38 @@ fn walker_pin_type_version_tag( walker_flush_guard_not_invalidated(ctx, op_pc) } +/// The `celldict.py:34 _immutable_fields_ = ["version?"]` twin of +/// [`walker_pin_type_version_tag`]: pin the module namespace's strategy version +/// so the folds that bake a slot's stored cell (or the absence of a name) are +/// revoked by `mutated()` instead of re-reading the dict each iteration. +/// +/// The marker attaches to the strategy box, not the dict object, because +/// `version` lives on the strategy — `getdictvalue_no_unwrapping` +/// (celldict.py:47-55) promotes `self` and then `self.version` for the same +/// reason. One marker covers every fold against one namespace, since they all +/// depend on that single field. +/// +/// `Ok(false)` when the dict has no strategy box to pin, which declines the +/// fold rather than baking a constant nothing watches. +fn walker_pin_namespace_version( + ctx: &mut WalkContext<'_, '_, Sym>, + op_pc: usize, + ns: pyre_object::PyObjectRef, +) -> Result { + let strategy = unsafe { pyre_object::dictmultiobject::w_module_dict_get_strategy(ns) }; + if strategy.is_null() { + return Ok(false); + } + let strategy_const = ctx.trace_ctx.const_ref(strategy as i64); + crate::state::record_quasiimmut_field( + ctx.trace_ctx, + strategy_const, + crate::descr::module_dict_version_descr(), + ); + walker_flush_guard_not_invalidated(ctx, op_pc)?; + Ok(true) +} + fn walker_record_getfield_gc_r_uncached( ctx: &mut WalkContext<'_, '_, Sym>, obj: OpRef, @@ -7747,19 +7779,14 @@ fn emit_namespace_cell_fold( if guard_frame_globals && !guard_current_frame_globals_identity(ctx, op_pc, ns)? { return Ok(false); } - let ns_const = ctx.trace_ctx.const_ref(ns as i64); - let slot_const = ctx.trace_ctx.const_int(slot as i64); - crate::state::record_namespace_quasiimmut_field( - ctx.trace_ctx, - ns_const, - slot_const, - slot as u32, - ); - walker_flush_guard_not_invalidated(ctx, op_pc)?; + if !walker_pin_namespace_version(ctx, op_pc, ns)? { + return Ok(false); + } // Bake the immovable cell as a `ConstPtr` (pypy `ConstPtr(cell)`). The - // `QuasiimmutField(ns, slot)` guard above invalidates the loop on a - // rebind / strategy-version bump (`optimize_QUASIIMMUT_FIELD` watches the - // `(dict, slot)` pair, not the cell), and the caller's `can_move` check + // `QuasiimmutField(strategy, version)` guard above invalidates the loop on a + // rebind / strategy-version bump (`_setitem_str_cell_known` calls + // `mutated()` before every write that replaces the stored pointer), and the + // caller's `can_move` check // guarantees the address is stable — the optimizer already folds the // equivalent elidable `jit_namespace_cell_lookup` down to this same const // ptr. A genuine constant (not the elidable call's `RefOp` result, which @@ -7837,16 +7864,10 @@ fn emit_namespace_cell_store_fold( stored: pyre_object::PyObjectRef, raw_int: OpRef, new_int: i64, -) -> Result<(), DispatchError> { - let ns_const = ctx.trace_ctx.const_ref(ns as i64); - let slot_const = ctx.trace_ctx.const_int(slot as i64); - crate::state::record_namespace_quasiimmut_field( - ctx.trace_ctx, - ns_const, - slot_const, - slot as u32, - ); - walker_flush_guard_not_invalidated(ctx, op_pc)?; +) -> Result { + if !walker_pin_namespace_version(ctx, op_pc, ns)? { + return Ok(false); + } // Bake the immovable cell as a `ConstPtr`, identical to the LOAD fold // (`emit_namespace_cell_fold`), so this `setfield_gc_i` and the LOAD's // `getfield_gc_i` canonicalise onto one trace-heapcache slot via @@ -7907,7 +7928,7 @@ fn emit_namespace_cell_store_fold( // exception clear exactly as [`emit_namespace_cell_fold`] does. ctx.last_exc_value = None; ctx.last_exc_value_concrete = ConcreteValue::Null; - Ok(()) + Ok(true) } /// #67 shape fix: append virtualizable data boxes so the walker merge-point diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs index e229ba1be90..b92e8e2af1c 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs @@ -8918,26 +8918,17 @@ pub(crate) fn try_walker_load_global_cell_fold( return Ok(false); } // Guard (a): the name must stay ABSENT from the module dict so the lookup - // keeps falling through to builtins. A `QUASIIMMUT_FIELD` on the module - // dict registers the loop flag on the module-dict `version` watcher; a - // later `mutated()` (the new-key insert that shadows the builtin) fails - // GUARD_NOT_INVALIDATED. The slot operand is unused for version keying - // (the watcher is per-`version`, not per-slot, and the registration - // ignores the slot); use `usize::MAX` as a past-the-end sentinel so the - // `quasi_immut_cache` key cannot collide with a real cell fold's slot for - // a DIFFERENT present name on the same module dict. + // keeps falling through to builtins. Pinning the module dict's `version?` + // is what proves that: the new-key insert that would shadow the builtin + // runs `mutated()`, which fails GUARD_NOT_INVALIDATED. It is the same + // field a present-name fold on this namespace pins, so the two share one + // marker. if !guard_current_frame_globals_identity(ctx, op_pc, w_globals)? { return Ok(false); } - let abs_ns_const = ctx.trace_ctx.const_ref(w_globals as i64); - let abs_slot_const = ctx.trace_ctx.const_int(usize::MAX as i64); - crate::state::record_namespace_quasiimmut_field( - ctx.trace_ctx, - abs_ns_const, - abs_slot_const, - u32::MAX, - ); - walker_flush_guard_not_invalidated(ctx, op_pc)?; + if !walker_pin_namespace_version(ctx, op_pc, w_globals)? { + return Ok(false); + } // Guard (b): the builtins value for `name` must be unchanged. The // `emit_namespace_cell_fold` below records a `QUASIIMMUT_FIELD` on the // builtins dict + the elidable cell lookup, so a rebind/del of the @@ -9080,6 +9071,5 @@ pub(crate) fn try_walker_store_name_cell_fold( let Some(majit_ir::Value::Int(new_int)) = ctx.trace_ctx.box_value(raw_int) else { return Ok(false); }; - emit_namespace_cell_store_fold(ctx, op_pc, w_globals, slot, stored, raw_int, new_int)?; - Ok(true) + emit_namespace_cell_store_fold(ctx, op_pc, w_globals, slot, stored, raw_int, new_int) } diff --git a/pyre/pyre-jit-trace/src/state.rs b/pyre/pyre-jit-trace/src/state.rs index 4f9a7a7984a..c73d32046a2 100644 --- a/pyre/pyre-jit-trace/src/state.rs +++ b/pyre/pyre-jit-trace/src/state.rs @@ -3392,7 +3392,7 @@ pub(crate) fn opimpl_getfield_gc_i(ctx: &mut TraceCtx, obj: OpRef, descr: DescrR // generate_guard parity). Instead, set a flag on ctx so the caller // (PyreSym with_ctx block) can emit it with full resume data. if descr.is_quasi_immutable() { - if ctx.heap_cache().is_quasi_immut_known(obj, field_index) { + if ctx.heap_cache().is_quasi_immut_known(field_index, obj) { // pyjitpl.py:1077-1080 cache hit: // if heapcache.is_quasi_immut_known(fielddescr, box): // profiler.count_ops(rop.QUASIIMMUT_FIELD, HEAPCACHED_OPS) @@ -3402,7 +3402,7 @@ pub(crate) fn opimpl_getfield_gc_i(ctx: &mut TraceCtx, obj: OpRef, descr: DescrR majit_metainterp::counters::HEAPCACHED_OPS, ); } else { - ctx.heap_cache_mut().quasi_immut_now_known(obj, field_index); + ctx.heap_cache_mut().quasi_immut_now_known(field_index, obj); ctx.record_op_with_descr(OpCode::QuasiimmutField, &[obj], descr.clone()); if ctx.heap_cache_mut().check_and_clear_guard_not_invalidated() { ctx.set_pending_guard_not_invalidated(Some(ctx.last_traced_pc)); @@ -3498,14 +3498,14 @@ pub(crate) fn opimpl_getfield_gc_r(ctx: &mut TraceCtx, obj: OpRef, descr: DescrR return cached; } if descr.is_quasi_immutable() { - if ctx.heap_cache().is_quasi_immut_known(obj, field_index) { + if ctx.heap_cache().is_quasi_immut_known(field_index, obj) { // pyjitpl.py:1077-1080 cache hit (see opimpl_getfield_gc_i above). ctx.profiler().count_ops( OpCode::QuasiimmutField, majit_metainterp::counters::HEAPCACHED_OPS, ); } else { - ctx.heap_cache_mut().quasi_immut_now_known(obj, field_index); + ctx.heap_cache_mut().quasi_immut_now_known(field_index, obj); ctx.record_op_with_descr(OpCode::QuasiimmutField, &[obj], descr.clone()); if ctx.heap_cache_mut().check_and_clear_guard_not_invalidated() { ctx.set_pending_guard_not_invalidated(Some(ctx.last_traced_pc)); @@ -4406,56 +4406,87 @@ pub(crate) fn module_dict_cell_value_direct(obj: PyObjectRef, slot: usize) -> Op unsafe { pyre_object::dictmultiobject::module_dict_cell_at(obj, slot) } } -/// pyjitpl.py:1074-1089 `opimpl_record_quasiimmut_field` for namespace -/// slot folds: record the dependency marker and arm the pending -/// GUARD_NOT_INVALIDATED once per heapcache epoch. -pub(crate) fn record_namespace_quasiimmut_field( - ctx: &mut TraceCtx, - obj: OpRef, - slot: OpRef, - slot_index: u32, -) { - if ctx.heap_cache().is_quasi_immut_known(obj, slot_index) { - ctx.profiler().count_ops( - OpCode::QuasiimmutField, - majit_metainterp::counters::HEAPCACHED_OPS, - ); - return; - } - ctx.heap_cache_mut().quasi_immut_now_known(obj, slot_index); - ctx.record_op(OpCode::QuasiimmutField, &[obj, slot]); - if ctx.heap_cache_mut().check_and_clear_guard_not_invalidated() { - ctx.set_pending_guard_not_invalidated(Some(ctx.last_traced_pc)); - } -} - -/// pyjitpl.py:1074-1089 `opimpl_record_quasiimmut_field` for a real struct -/// field, the [`record_namespace_quasiimmut_field`] twin. +/// pyjitpl.py:1074-1089 `opimpl_record_quasiimmut_field`: record the +/// dependency marker and arm the pending GUARD_NOT_INVALIDATED once per +/// heapcache epoch. /// -/// The namespace variant keys on a synthetic `(dict, slot)` pair because a -/// module-dict cell has no field descriptor; a genuine quasi-immutable field -/// carries one, so the op is recorded with the descr and the heapcache keys on -/// `descr.index()` the same way [`opimpl_getfield_gc_i`] does. +/// The heapcache keys on `descr.index()` and the receiver, the same way +/// [`opimpl_getfield_gc_i`] does. /// /// The caller has already resolved the field's value and is baking it as a /// constant, so unlike `opimpl_getfield_gc_i` this records no load — that is /// exactly what quasi-immutability buys. pub(crate) fn record_quasiimmut_field(ctx: &mut TraceCtx, obj: OpRef, descr: DescrRef) { let field_index = descr.index(); - if ctx.heap_cache().is_quasi_immut_known(obj, field_index) { + if ctx.heap_cache().is_quasi_immut_known(field_index, obj) { ctx.profiler().count_ops( OpCode::QuasiimmutField, majit_metainterp::counters::HEAPCACHED_OPS, ); return; } - ctx.heap_cache_mut().quasi_immut_now_known(obj, field_index); - ctx.record_op_with_descr(OpCode::QuasiimmutField, &[obj], descr); + // quasiimmut.py:125 `self.constantfieldbox = + // self.get_current_constant_fieldvalue()` — the field's value at the moment + // the trace baked it. `heap.py:803 is_still_valid_for` compares it against + // the live value and abandons the loop when they disagree, so it has to be + // captured here; by the time the optimizer runs, the change it is looking + // for has already happened. + let constantfieldbox = current_quasiimmut_field_value(ctx, obj, &descr); + ctx.heap_cache_mut().quasi_immut_now_known(field_index, obj); + // Upstream carries the captured value on the per-op `QuasiImmutDescr` + // (quasiimmut.py:113-159), a descr minted fresh for every recorded + // QUASIIMMUT_FIELD. Pyre's descrs are registry-indexed singletons, so a + // fresh one per op would mint a fresh registry entry per read; the value + // rides on the op instead. + match constantfieldbox { + Some(value) => ctx.record_op_with_descr(OpCode::QuasiimmutField, &[obj, value], descr), + None => ctx.record_op_with_descr(OpCode::QuasiimmutField, &[obj], descr), + }; if ctx.heap_cache_mut().check_and_clear_guard_not_invalidated() { ctx.set_pending_guard_not_invalidated(Some(ctx.last_traced_pc)); } } +/// quasiimmut.py:135-143 `QuasiImmutDescr.get_current_constant_fieldvalue`. +/// +/// ```text +/// def get_current_constant_fieldvalue(self): +/// struct = self.struct +/// fielddescr = self.fielddescr +/// if self.fielddescr.is_pointer_field(): +/// return ConstPtr(self.cpu.bh_getfield_gc_r(struct, fielddescr)) +/// elif self.fielddescr.is_float_field(): +/// return ConstFloat(self.cpu.bh_getfield_gc_f(struct, fielddescr)) +/// else: +/// return ConstInt(self.cpu.bh_getfield_gc_i(struct, fielddescr)) +/// ``` +/// +/// `field_sanity_load` is the `cpu.bh_getfield_gc_*` triple behind one +/// `field_type()` dispatch. `None` when the receiver is not a concrete +/// pointer or the descr is not a field descr — the optimizer then skips the +/// revalidation, matching heap.py:794-796, which ignores a QUASIIMMUT_FIELD +/// whose struct did not fold to a constant. +fn current_quasiimmut_field_value( + ctx: &mut TraceCtx, + obj: OpRef, + descr: &DescrRef, +) -> Option { + let field_type = descr.as_field_descr()?.field_type(); + let majit_ir::Value::Ref(struct_ref) = ctx.box_value(obj)? else { + return None; + }; + let struct_ptr = struct_ref.0 as i64; + if struct_ptr == 0 || struct_ptr == usize::MAX as i64 { + return None; + } + match ctx.field_sanity_load(struct_ptr, descr, field_type)? { + majit_ir::Value::Int(n) => Some(ctx.const_int(n)), + majit_ir::Value::Ref(gcref) => Some(ctx.const_ref(gcref.0 as i64)), + majit_ir::Value::Float(f) => Some(ctx.const_float(f.to_bits() as i64)), + majit_ir::Value::Void => None, + } +} + /// virtualizable.py:44 + interp_jit.py:25-31 — /// `locals_cells_stack_w[*]` is declared as a W_Root array, so every /// item's JIT type is GCREF (Type::Ref). W_IntObject/W_FloatObject are diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index e7eea903d0d..491aa848aeb 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -456,6 +456,11 @@ unsafe fn type_object_destructor(obj_addr: usize) { if !weak_subclasses.is_null() { drop(unsafe { Box::from_raw(weak_subclasses) }); } + // The `mutate__version_tag` instance (`quasiimmut.py:116-126 + // get_current_qmut_instance`) is Rust-owned and off-GC too. A type that was + // compiled against and never mutated afterwards still holds one, so without + // this the box outlives the only pointer to it. + drop(unsafe { (*t).quasi_immut_watchers.take() }); } /// Custom trace for `GeneratorIterator` (generator.py GeneratorIterator). @@ -5250,12 +5255,36 @@ pub fn make_green_key(code_ptr: *const (), pc: usize) -> u64 { // source of truth. call_depth() reads it. No more Box allocation. /// RPython compile.py:204-207 (record_loop_or_bridge) parity: -/// Register the compiled artifact's invalidation flag with all quasi-immutable -/// dependencies collected during optimization. The optimizer records -/// namespace pointers in quasi_immutable_deps when processing -/// QUASIIMMUT_FIELD ops. After compilation, this function reads them -/// from MetaInterp and registers watchers so GUARD_NOT_INVALIDATED -/// fails when the namespace mutates. +/// +/// ```text +/// if loop.quasi_immutable_deps is not None: +/// for qmut in loop.quasi_immutable_deps: +/// qmut.register_loop_token(wref) +/// ``` +/// +/// Upstream's set holds the `QuasiImmut` instances the optimizer already +/// resolved through each `QuasiImmutDescr`. Pyre records the pair that +/// identifies one — the owning object and the registry index of the +/// quasi-immutable field — and resolves the instance here, which is why the +/// field index has to select the registrar: a `ModuleDictStrategy` box carries +/// no `PyObject` header, so offering it to the type-keyed registrar would read +/// one that is not there. +/// +/// The two `?` fields: +/// +/// `celldict.py:34 _immutable_fields_ = ["version?"]` — the global cell fast +/// path bakes a slot's stored cell as a `ConstPtr` under a +/// `QUASIIMMUT_FIELD(strategy, version)`. `mutated()` (new key, `del`, +/// `switch_to_object_strategy`, and every write that replaces the stored +/// pointer) flips the flag; a same-key reassign mutates the cell in place +/// without bumping the version and is observed by the live `cell.w_value` read +/// instead. +/// +/// `typeobject.py:177 _immutable_fields_ = ['_version_tag?']` — the +/// LOAD_METHOD / LOAD_ATTR method-cache fold bakes a type's `version_tag` as a +/// constant under a `QUASIIMMUT_FIELD(w_type, _version_tag)`. `mutated()` +/// (typeobject.py:285-291) bumps the tag and walks subclasses, and the setter +/// revokes each level's loops. fn register_quasi_immutable_deps(_green_key: u64) { let (driver, _) = driver_pair(); let deps: Vec<(u64, u32)> = @@ -5266,29 +5295,20 @@ fn register_quasi_immutable_deps(_green_key: u64) { let Some(flag) = driver.last_compiled_artifact_invalidation_flag() else { return; }; - // `celldict.py:34 _immutable_fields_ = ["version?"]`: the global cell - // fast path's `QUASIIMMUT_FIELD(ns, slot)` is keyed on the module - // dict's `ModuleDictStrategy.version`, not a per-slot index, so every - // recorded dep registers the loop flag against that single version - // watcher. `mutated()` (new key, `del`, `switch_to_object_strategy`) - // then flips the flag; a same-key value reassign mutates the cell in - // place without bumping the version and is observed by the live - // `cell.w_value` read instead. `ns_ptr` is the `const_ref`-folded - // `w_globals` object pointer; `slot` is unused for version keying. - // - // `typeobject.py:177 _immutable_fields_ = ['_version_tag?']`: the - // LOAD_METHOD / LOAD_ATTR method-cache fold bakes a type's `version_tag` - // as a constant under a `QUASIIMMUT_FIELD(w_type)`, so the same loop flag - // registers against the type. `mutated()` (typeobject.py:285-291) bumps - // the tag and walks subclasses, and the setter revokes each level's loops. - // - // Both registrations self-filter on the object's kind, so a dep of either - // kind reaches exactly its own watcher list. - for (dep_ptr, _slot) in deps { - let obj = dep_ptr as pyre_object::PyObjectRef; + let module_dict_version = pyre_jit_trace::descr::module_dict_version_descr().index(); + for (dep_ptr, field_index) in deps { unsafe { - pyre_object::dictmultiobject::module_dict_register_version_watcher(obj, &flag); - pyre_object::typeobject::w_type_register_quasi_immut_watcher(obj, &flag); + if field_index == module_dict_version { + pyre_object::dictmultiobject::module_dict_strategy_register_version_watcher( + dep_ptr as *mut pyre_object::celldict::ModuleDictStrategy, + &flag, + ); + } else { + pyre_object::typeobject::w_type_register_quasi_immut_watcher( + dep_ptr as pyre_object::PyObjectRef, + &flag, + ); + } } } } @@ -8475,6 +8495,12 @@ fn compile_and_run_once( .clear_tracing_flag(starting_tracing_key); if !had_compiled && driver.has_compiled_loop(compiled_key) { register_quasi_immutable_deps(compiled_key); + } else { + // `register_quasi_immutable_deps` is the only drain of + // `last_quasi_immutable_deps`. An attempt that produced no artifact + // to attach them to has to clear them anyway, or the next compile + // registers this trace's dependencies against its flag. + driver.meta_interp_mut().last_quasi_immutable_deps.clear(); } } diff --git a/pyre/pyre-object/src/celldict.rs b/pyre/pyre-object/src/celldict.rs index e5f321258a5..344afbc3fa7 100644 --- a/pyre/pyre-object/src/celldict.rs +++ b/pyre/pyre-object/src/celldict.rs @@ -593,14 +593,20 @@ pub struct ModuleDictStrategy { pub caches: std::sync::Mutex< Option>>>, >, - /// JIT loop-invalidation flags watching the `version?` quasi-immutable - /// field. Each compiled loop whose trace promoted `self.version` - /// (and folded a module-global lookup keyed on it) registers its - /// `JitCellToken` invalidation flag here. `mutated()` reassigns - /// `version`, which under `_immutable_fields_ = ["version?"]` must - /// invalidate every such loop, so it flips all live flags. Weak refs - /// so a dead loop token drops out without keeping the flag alive. - version_watchers: Vec>, + /// The hidden `mutate_version` field for `celldict.py:34 + /// _immutable_fields_ = ["version?"]`. Each compiled loop whose trace + /// promoted `self.version` (and folded a module-global lookup keyed on it) + /// registers its `JitCellToken` invalidation flag here; `mutated()` + /// reassigns `version`, which under the `?` declaration must invalidate + /// every such loop. + /// + /// The same [`crate::quasiimmut::QuasiImmutField`] `W_TypeObject`'s + /// `_version_tag?` uses, as upstream's one `QuasiImmut` class serves every + /// quasi-immutable field. Before that it was a bare `Vec` pushed to + /// without synchronisation while `mutated()` swept it from another thread, + /// and it had no `compress_looptokens_list`, so a module recompiled against + /// many times and never mutated grew one entry per compile. + version_watchers: crate::quasiimmut::QuasiImmutField, } /// Runtime-assigned GC type id for the [`ModuleDictStrategy`] box. @@ -618,36 +624,6 @@ pub fn module_dict_strategy_gc_type_id() -> u32 { MODULE_DICT_STRATEGY_GC_TYPE_ID.load(std::sync::atomic::Ordering::Relaxed) } -/// Flip every live watcher flag and drop the dead weak refs — the non-empty -/// half of [`ModuleDictStrategy::notify_version_watchers`]. -/// -/// This is the `version?` quasi-immutable invalidation walk, and upstream runs -/// the whole of it outside traced code: `QuasiImmut.invalidate` -/// (`metainterp/quasiimmut.py`) iterates `looptokens_wrefs` and calls -/// `invalidate_loop` from the residual `jit_force_quasi_immutable` path, never -/// from a trace. Residualise it here for the same reason -/// (`@dont_look_inside`, `rlib/jit.py:139`); the `Vec::retain` closure over -/// `Weak::upgrade` has no lowering either way. The `is_empty` early-out stays -/// traced, so the common no-watcher mutation still makes no call. -#[majit_macros::dont_look_inside] -pub fn sweep_version_watchers(watchers: &mut Vec>) { - // `QuasiImmut.invalidate` takes the list and empties it BEFORE walking it - // (`wrefs = self.looptokens_wrefs; self.looptokens_wrefs = []`), so a loop - // is invalidated exactly once and then drops out. Keeping the live - // watchers instead would be doubly wrong: the flag is already `true`, so - // re-storing it does nothing, and the list would only ever grow — one - // entry per compiled loop that folded a module-global. A module-level - // `except X as e:` runs `del e` every iteration, and `delitem` calls - // `mutated()`, so retaining would make each iteration walk every loop ever - // compiled in the module: O(compiled loops) per store, and the JIT - // compiling more loops would make the interpreter slower. - for w in std::mem::take(watchers) { - if let Some(flag) = w.upgrade() { - flag.store(true, std::sync::atomic::Ordering::Release); - } - } -} - impl Default for ModuleDictStrategy { fn default() -> Self { Self::new() @@ -660,32 +636,32 @@ impl ModuleDictStrategy { Self { version: VersionTag::fresh(), caches: std::sync::Mutex::new(None), - version_watchers: Vec::new(), + version_watchers: crate::quasiimmut::QuasiImmutField::new(), } } /// Register a JIT loop's invalidation flag against the `version?` - /// quasi-immutable field. The compile-time glue - /// (`register_quasi_immutable_deps` analogue) calls this once per + /// quasi-immutable field (`quasiimmut.py:116-126 + /// get_current_qmut_instance` + `:72-75 register_loop_token`). The + /// compile-time glue (`register_quasi_immutable_deps`) calls this once per /// version-keyed module-global dependency, passing the - /// `JitCellToken.invalidation_flag()`. Mirrors - /// `DictStorage::register_slot_watcher` but keyed on the strategy - /// version rather than a per-slot index. - pub fn register_version_watcher( - &mut self, - flag: &std::sync::Arc, - ) { - self.version_watchers.push(std::sync::Arc::downgrade(flag)); + /// `JitCellToken.invalidation_flag()`. + pub fn register_version_watcher(&self, flag: &std::sync::Arc) { + self.version_watchers.register_loop_token(flag); } - /// Invalidate every loop watching `version`. Sets each live flag to - /// `true` (the polarity `GuardNotInvalidated` tests) and drops dead - /// weak refs. Mirrors `DictStorage::notify_slot_watchers`. - fn notify_version_watchers(&mut self) { - if self.version_watchers.is_empty() { + /// Invalidate every loop watching `version` + /// (`quasiimmut.py:129-134 _invalidate_now`). Sets each live flag to + /// `true`, the polarity `GuardNotInvalidated` tests. + /// + /// The installed check stays traced and the sweep is residual, for the + /// reason upstream's own walk is out of line: it hangs off + /// `jit_force_quasi_immutable`, never a trace. + fn notify_version_watchers(&self) { + if !self.version_watchers.is_installed() { return; } - sweep_version_watchers(&mut self.version_watchers); + unsafe { crate::quasiimmut::sweep_quasi_immut_field(&self.version_watchers) }; } /// `celldict.py:214-240 get_global_cache`: @@ -1251,9 +1227,10 @@ mod tests { strategy.register_version_watcher(&flag); // Drop the only strong ref: the weak watcher can no longer upgrade. drop(flag); - // notify (via mutated) must not panic and must purge the dead weak. + // notify (via mutated) must not panic, and `_invalidate_now` unlinks + // the instance whether or not any flag could still be upgraded. strategy.mutated(); - assert!(strategy.version_watchers.is_empty()); + assert!(!strategy.version_watchers.is_installed()); } #[test] diff --git a/pyre/pyre-object/src/dictmultiobject.rs b/pyre/pyre-object/src/dictmultiobject.rs index e4be946b6c6..9ccadff16fa 100644 --- a/pyre/pyre-object/src/dictmultiobject.rs +++ b/pyre/pyre-object/src/dictmultiobject.rs @@ -964,28 +964,26 @@ pub unsafe fn module_dict_storage_len(obj: PyObjectRef) -> Option { Some((*md.dstorage).len()) } -/// Register a compiled loop's invalidation `flag` against the module -/// dict's `ModuleDictStrategy.version?` quasi-immutable field -/// (`celldict.py:34 _immutable_fields_ = ["version?"]`). The compile-time -/// glue calls this once per version-keyed module-global dependency so a -/// later `mutated()` (new key, `del`, or `switch_to_object_strategy`) -/// flips the flag and fails the loop's `GUARD_NOT_INVALIDATED`. No-op -/// when `obj` is not a `W_ModuleDictObject`. +/// Register a compiled loop's invalidation `flag` against a module dict +/// strategy's `version?` quasi-immutable field (`celldict.py:34 +/// _immutable_fields_ = ["version?"]`). The compile-time glue calls this once +/// per version-keyed module-global dependency so a later `mutated()` (new key, +/// `del`, `switch_to_object_strategy`, or a write that replaces a stored cell) +/// flips the flag and fails the loop's `GUARD_NOT_INVALIDATED`. +/// +/// Takes the strategy rather than the dict because that is what the trace pins: +/// `version` lives on the strategy, so the `QUASIIMMUT_FIELD` names it directly. /// /// # Safety -/// `obj` must be null or a valid PyObjectRef. -pub unsafe fn module_dict_register_version_watcher( - obj: PyObjectRef, +/// `strategy` must be null or point at a valid `ModuleDictStrategy`. +pub unsafe fn module_dict_strategy_register_version_watcher( + strategy: *mut crate::celldict::ModuleDictStrategy, flag: &std::sync::Arc, ) { - if obj.is_null() || !is_module_dict(obj) { - return; - } - let md = &*(obj as *const W_ModuleDictObject); - if md.mstrategy.is_null() { + if strategy.is_null() { return; } - (*md.mstrategy).register_version_watcher(flag); + (*strategy).register_version_watcher(flag); } /// Allocate a new empty dict per `dictmultiobject.py:67-69 diff --git a/pyre/pyre-object/src/lib.rs b/pyre/pyre-object/src/lib.rs index 7bfe9ff619a..98d87f7187b 100644 --- a/pyre/pyre-object/src/lib.rs +++ b/pyre/pyre-object/src/lib.rs @@ -50,6 +50,7 @@ pub mod object_array; pub mod objectobject; pub mod operation; pub mod pyobject; +pub mod quasiimmut; pub mod rbigint; pub mod rutf8; pub mod setobject; diff --git a/pyre/pyre-object/src/quasiimmut.rs b/pyre/pyre-object/src/quasiimmut.rs new file mode 100644 index 00000000000..b5e6ac14270 --- /dev/null +++ b/pyre/pyre-object/src/quasiimmut.rs @@ -0,0 +1,353 @@ +//! `rpython/jit/metainterp/quasiimmut.py` — the loop-invalidation half of an +//! `_immutable_fields_` entry spelled with a `?`. +//! +//! Upstream has one `QuasiImmut` class serving every quasi-immutable field: the +//! rtyper synthesises a hidden `mutate_` pointer field per declaration +//! (`get_mutate_field_name`), `get_current_qmut_instance` fills it in on the +//! first registration, and the invalidation function the rtyper installs on the +//! real field's write path nulls it and sweeps it. Pyre has no rtyper, so +//! [`QuasiImmutField`] is that hidden field written out, and the two `?` fields +//! this tree declares — `W_TypeObject._version_tag` +//! (typeobject.py:177) and `ModuleDictStrategy.version` (celldict.py:34) — +//! share it. + +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicPtr, Ordering}; + +/// `quasiimmut.py:55-109 QuasiImmut` — the loops that baked one quasi-immutable +/// field's value as a constant, and must be revoked when it changes. +/// +/// The flag stands in for upstream's `looptoken` + `cpu.invalidate_loop`: the +/// backend already routes `GUARD_NOT_INVALIDATED` through a per-artifact +/// `AtomicBool`, so setting it is what `looptoken.invalidated = True` buys. +pub struct QuasiImmut { + /// `quasiimmut.py:62-63` — weak so a retired loop drops out instead of + /// being kept alive by the object whose field it read. + looptokens_wrefs: Vec>, + /// `quasiimmut.py:57 compress_limit = 30`. + compress_limit: usize, +} + +impl Default for QuasiImmut { + fn default() -> Self { + Self::new() + } +} + +impl QuasiImmut { + /// `quasiimmut.py:59-64 __init__`. The initial limit is the growth formula + /// in [`Self::compress_looptokens_list`] evaluated at length zero. + pub fn new() -> Self { + Self { + looptokens_wrefs: Vec::new(), + compress_limit: 30, + } + } + + /// `quasiimmut.py:72-75 register_loop_token`. + pub fn register_loop_token(&mut self, flag: &Arc) { + if self.looptokens_wrefs.len() > self.compress_limit { + self.compress_looptokens_list(); + } + self.looptokens_wrefs.push(Arc::downgrade(flag)); + } + + /// `quasiimmut.py:77-82 compress_looptokens_list` — drop the entries whose + /// loop is gone and re-derive the limit from what is left, so an object that + /// is recompiled against many times and never mutated cannot grow an + /// unbounded list. + /// + /// Upstream's note that already-invalidated tokens must be kept applies + /// here too: the flag stays live while its artifact does, and re-flipping + /// an already-set flag is what keeps a multiply-invalidated loop revoked. + fn compress_looptokens_list(&mut self) { + self.looptokens_wrefs.retain(|w| w.strong_count() > 0); + self.compress_limit = (self.looptokens_wrefs.len() + 15) * 2; + } + + /// `quasiimmut.py:84-109 invalidate` — every loop recorded here becomes + /// invalid, so each `GUARD_NOT_INVALIDATED` in it (and in its bridges) must + /// now fail. The list is emptied like upstream's `self.looptokens_wrefs = + /// []`; the caller then drops the instance itself, so a loop that recompiles + /// registers against a fresh one. + /// + /// Taking the list is what bounds the walk. Keeping the live entries would + /// be doubly wrong: the flag is already `true`, so re-storing it does + /// nothing, and the list would only ever grow — one entry per compiled loop + /// that folded a field read. A module-level `except X as e:` runs `del e` + /// every iteration and `delitem` calls `mutated()`, so retaining would make + /// each iteration walk every loop ever compiled against the object: + /// O(compiled loops) per store, and the JIT compiling more loops would make + /// the interpreter slower. + pub fn invalidate(&mut self) { + for wref in std::mem::take(&mut self.looptokens_wrefs) { + if let Some(flag) = wref.upgrade() { + flag.store(true, Ordering::Release); + } + } + } + + /// Number of entries still recorded, for tests. + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + self.looptokens_wrefs.len() + } + + /// The current growth limit, for tests. + #[cfg(test)] + pub(crate) fn compress_limit(&self) -> usize { + self.compress_limit + } +} + +/// The hidden `mutate_` field the rtyper synthesises for one `?` +/// declaration (`quasiimmut.py get_mutate_field_name`), written out because +/// pyre has no rtyper to synthesise it. +/// +/// Null until the first loop registers (`get_current_qmut_instance`, +/// quasiimmut.py:116-126); nulled and dropped by the invalidation function +/// (`make_invalidation_function._invalidate_now`, quasiimmut.py:129-134), so +/// the next registration starts from a fresh instance and the identity a +/// revalidation compares really did change. +/// +/// The pointer is the single source of truth, exactly as upstream's field is. +/// The lock is the nogil adaptation: upstream gets the null-then-sweep pair for +/// free from the GIL, while pyre runs Python threads on real OS threads, so +/// without it two mutators free one instance twice and a mutator can free the +/// instance a compiling thread is pushing into. Only the dereferencing paths +/// take it — [`Self::is_installed`] deliberately does not, because that bare +/// test is the whole cost a mutation on an object no loop watches has to pay. +/// +/// Both owners are allocated non-moving (`try_gc_alloc_stable_raw` / +/// `malloc_typed`), so the lock cannot be remapped out from under a holder and +/// no address-striped indirection is needed. The critical section allocates +/// nothing GC-managed and crosses no safepoint, so it cannot park a mutator the +/// collector is waiting for. +pub struct QuasiImmutField { + ptr: AtomicPtr, + lock: parking_lot::Mutex<()>, +} + +impl Default for QuasiImmutField { + fn default() -> Self { + Self::new() + } +} + +impl QuasiImmutField { + pub const fn new() -> Self { + Self { + ptr: AtomicPtr::new(std::ptr::null_mut()), + lock: parking_lot::Mutex::new(()), + } + } + + /// Whether any loop has registered since the last invalidation — the + /// `if not qmut_ptr` test that guards `_invalidate_now`'s body + /// (quasiimmut.py:130). Lock-free so it can stay inside a trace. + #[inline] + pub fn is_installed(&self) -> bool { + !self.ptr.load(Ordering::Acquire).is_null() + } + + /// `quasiimmut.py:116-126 get_current_qmut_instance` followed by + /// `:72-75 register_loop_token`: create the instance if the field is still + /// null, then record this loop's invalidation flag on it. + pub fn register_loop_token(&self, flag: &Arc) { + let _guard = self.lock.lock(); + let mut qmut_ptr = self.ptr.load(Ordering::Acquire); + if qmut_ptr.is_null() { + qmut_ptr = Box::into_raw(Box::new(QuasiImmut::new())); + self.ptr.store(qmut_ptr, Ordering::Release); + } + // Safe: the pointer is only ever cleared under the same lock, and the + // instance is freed by whoever wins that clear. + unsafe { (*qmut_ptr).register_loop_token(flag) }; + } + + /// `quasiimmut.py:129-134 make_invalidation_function._invalidate_now` — + /// unlink the instance, then flip every loop flag it recorded. + /// + /// Re-reads the pointer under the lock rather than trusting an earlier + /// [`Self::is_installed`]: two threads mutating the same object both see it + /// installed, and only the one that wins the swap owns the [`Box`]. + pub fn invalidate(&self) { + let Some(mut qmut) = self.take() else { + return; + }; + qmut.invalidate(); + } + + /// Unlink the instance and hand it to the caller. The owner's destructor + /// uses this to reclaim a box that was never invalidated. + pub fn take(&self) -> Option> { + let qmut_ptr = { + let _guard = self.lock.lock(); + self.ptr.swap(std::ptr::null_mut(), Ordering::AcqRel) + }; + if qmut_ptr.is_null() { + return None; + } + // Safe: the swap is what transfers ownership, and it happens once. + Some(unsafe { Box::from_raw(qmut_ptr) }) + } +} + +impl Drop for QuasiImmutField { + fn drop(&mut self) { + let qmut_ptr = *self.ptr.get_mut(); + if !qmut_ptr.is_null() { + drop(unsafe { Box::from_raw(qmut_ptr) }); + } + } +} + +/// The residual half of an invalidation: unlink the instance and flip every +/// loop flag it recorded. +/// +/// `#[dont_look_inside]` (`@jit.dont_look_inside`, `rlib/jit.py:139`) for the +/// reason upstream's own walk is out of line — it hangs off the residual +/// `jit_force_quasi_immutable` path and never appears in a trace — and because +/// the lock and the `Vec` walk have no lowering. A free function taking a raw +/// pointer, because that is the shape `majit_macros` can emit a call target +/// for; the callers keep their [`QuasiImmutField::is_installed`] test traced, +/// so a mutation on an object no loop watches still makes no call. +/// +/// # Safety +/// `field` must point at a live [`QuasiImmutField`]. +#[majit_macros::dont_look_inside] +pub unsafe fn sweep_quasi_immut_field(field: *const QuasiImmutField) { + unsafe { (*field).invalidate() }; +} + +#[cfg(test)] +mod tests { + use super::*; + + /// `quasiimmut.py:84-109 invalidate` flips every registered flag and empties + /// the list, and a flag whose loop is gone is simply skipped. + #[test] + fn invalidate_flips_live_flags_and_clears() { + let mut qi = QuasiImmut::new(); + let live = Arc::new(AtomicBool::new(false)); + let dead = Arc::new(AtomicBool::new(false)); + qi.register_loop_token(&live); + qi.register_loop_token(&dead); + drop(dead); + + qi.invalidate(); + assert!(live.load(Ordering::Acquire), "a live loop must be revoked"); + assert_eq!( + qi.len(), + 0, + "invalidate empties the list; a recompile registers again", + ); + + // A second invalidate with nothing registered is a no-op, and the flag + // stays set — `GUARD_NOT_INVALIDATED` has no un-set edge. + qi.invalidate(); + assert!(live.load(Ordering::Acquire)); + } + + /// `quasiimmut.py:72-82` — an object recompiled against many times and never + /// mutated must not grow an unbounded watcher list. + #[test] + fn register_compresses_dead_loop_tokens() { + let mut qi = QuasiImmut::new(); + assert_eq!(qi.compress_limit(), 30, "quasiimmut.py:57 compress_limit"); + for _ in 0..500 { + // Each "recompile" drops its artifact immediately, so every + // registered weak ref is already dead by the next round. + let flag = Arc::new(AtomicBool::new(false)); + qi.register_loop_token(&flag); + } + assert!( + qi.len() <= qi.compress_limit() + 1, + "compress must bound the list, got {} against limit {}", + qi.len(), + qi.compress_limit(), + ); + // With every entry dead the limit collapses back to the empty-list + // value rather than ratcheting upward. + assert_eq!(qi.compress_limit(), 30); + } + + /// `_invalidate_now` nulls the field before sweeping, so a later + /// registration starts from a fresh instance. + #[test] + fn invalidate_unlinks_before_sweeping() { + let field = QuasiImmutField::new(); + assert!(!field.is_installed()); + + let flag = Arc::new(AtomicBool::new(false)); + field.register_loop_token(&flag); + assert!(field.is_installed()); + + field.invalidate(); + assert!(flag.load(Ordering::Acquire)); + assert!( + !field.is_installed(), + "the field is nulled before the sweep" + ); + + // A second invalidation with nothing installed must not double-free. + field.invalidate(); + + // A recompile registers against a fresh instance rather than reviving + // the swept one. Only the address would witness that directly, and the + // allocator is free to hand the freed box straight back, so the + // observable is that the new flag rides its own list. + let flag2 = Arc::new(AtomicBool::new(false)); + field.register_loop_token(&flag2); + assert!(field.is_installed()); + field.invalidate(); + assert!(flag2.load(Ordering::Acquire)); + } + + /// Registration runs on the compiling thread while any other Python thread + /// can be mutating the same object. Upstream is safe here only because of + /// the GIL; pyre has none, so the get-or-create, the push and the free have + /// to be serialised or this races the instance to a double free. + /// + /// Reverting [`QuasiImmutField::take`] to an unlocked load/store aborts this + /// with heap corruption inside `Vec`. + #[test] + fn publication_is_serialised_across_threads() { + const ROUNDS: usize = 200_000; + + let field = QuasiImmutField::new(); + let stop = AtomicBool::new(false); + + std::thread::scope(|scope| { + // Two compiling threads keep re-installing the instance, so the + // mutators below keep finding one to free. + for _ in 0..2 { + let field = &field; + let stop = &stop; + scope.spawn(move || { + let flag = Arc::new(AtomicBool::new(false)); + while !stop.load(Ordering::Relaxed) { + field.register_loop_token(&flag); + } + }); + } + let mutators: Vec<_> = (0..4) + .map(|_| { + let field = &field; + scope.spawn(move || { + for _ in 0..ROUNDS { + field.invalidate(); + } + }) + }) + .collect(); + for mutator in mutators { + mutator.join().expect("mutator thread faulted"); + } + stop.store(true, Ordering::Relaxed); + }); + + field.invalidate(); + assert!(!field.is_installed()); + } +} diff --git a/pyre/pyre-object/src/typeobject.rs b/pyre/pyre-object/src/typeobject.rs index 646cb8898bd..fb8767b3c49 100644 --- a/pyre/pyre-object/src/typeobject.rs +++ b/pyre/pyre-object/src/typeobject.rs @@ -221,12 +221,17 @@ pub struct W_TypeObject { /// The hidden `mutate__version_tag` field for `typeobject.py:177 /// _immutable_fields_ = ['_version_tag?']` — see [`QuasiImmut`]. /// - /// Heap allocated on the first registration and null until then, exactly - /// like [`W_TypeObject::weak_subclasses`], which it also matches in never - /// being freed: this tree has no `W_TypeObject` teardown path. Holds no GC - /// pointers, so the `W_TYPE_GC_TYPE_ID` custom trace has nothing to walk - /// here. - pub quasi_immut_watchers: *mut QuasiImmut, + /// Allocated on the first registration (`get_current_qmut_instance`, + /// quasiimmut.py:116-126), null until then, and unlinked + freed on + /// invalidation (`_invalidate_now`, quasiimmut.py:129-134), so a type nobody + /// has mutated since its last compile is the only one holding a box. Holds + /// no GC pointers, so the `W_TYPE_GC_TYPE_ID` custom trace has nothing to + /// walk here. + /// + /// Shares [`crate::quasiimmut::QuasiImmutField`] with + /// `ModuleDictStrategy.version?`, the tree's other `?` declaration, the way + /// upstream's one `QuasiImmut` class serves every quasi-immutable field. + pub quasi_immut_watchers: crate::quasiimmut::QuasiImmutField, } /// Source of fresh `version_tag` identities (`VersionTag()`, typeobject.py:73). @@ -397,7 +402,7 @@ pub fn w_type_new(name: &str, bases: PyObjectRef, dict_ptr: *mut u8) -> PyObject flag_disallow_instantiation: std::sync::atomic::AtomicBool::new(false), flag_abstract: std::sync::atomic::AtomicBool::new(false), // Allocated lazily on the first loop registration. - quasi_immut_watchers: std::ptr::null_mut(), + quasi_immut_watchers: crate::quasiimmut::QuasiImmutField::new(), }; let (w_type, gc_managed) = if !raw.is_null() { unsafe { std::ptr::write(raw as *mut W_TypeObject, value) }; @@ -516,7 +521,7 @@ pub fn w_type_new_builtin( flag_disallow_instantiation: std::sync::atomic::AtomicBool::new(false), flag_abstract: std::sync::atomic::AtomicBool::new(false), // Allocated lazily on the first loop registration. - quasi_immut_watchers: std::ptr::null_mut(), + quasi_immut_watchers: crate::quasiimmut::QuasiImmutField::new(), }) as PyObjectRef; // A builtin type is Box-immortal, so its namespace values and `bases` are reachable only // through `walk_builtin_type_dicts_gc` (`pyre_interpreter::eval`). @@ -747,85 +752,10 @@ pub unsafe fn w_type_set_version_tag(obj: PyObjectRef, v: u64) { .store(v, std::sync::atomic::Ordering::Release); } -/// `quasiimmut.py:55-109 QuasiImmut` — the loops that baked one quasi-immutable -/// field's value as a constant, and must be revoked when it changes. -/// -/// Upstream reaches this object through the hidden `mutate_` field the -/// rtyper adds for each `_immutable_fields_` entry spelled with a `?` -/// (`get_mutate_field_name`), created on demand by -/// `get_current_qmut_instance`. Pyre has no rtyper to synthesise the field, so -/// [`W_TypeObject::quasi_immut_watchers`] is that field, spelled out and -/// likewise allocated on the first registration. -/// -/// The flag stands in for upstream's `looptoken` + `cpu.invalidate_loop`: the -/// backend already routes `GUARD_NOT_INVALIDATED` through a per-artifact -/// `AtomicBool`, so setting it is what `looptoken.invalidated = True` buys. -pub struct QuasiImmut { - /// `quasiimmut.py:62-63` — weak so a retired loop drops out instead of - /// being kept alive by the type that it read. - looptokens_wrefs: Vec>, - /// `quasiimmut.py:57 compress_limit = 30`. - compress_limit: usize, -} - -impl Default for QuasiImmut { - fn default() -> Self { - Self::new() - } -} - -impl QuasiImmut { - /// `quasiimmut.py:59-64 __init__`. The initial limit is the growth formula - /// in [`Self::compress_looptokens_list`] evaluated at length zero. - pub fn new() -> Self { - Self { - looptokens_wrefs: Vec::new(), - compress_limit: 30, - } - } - - /// `quasiimmut.py:72-75 register_loop_token`. - fn register_loop_token(&mut self, flag: &std::sync::Arc) { - if self.looptokens_wrefs.len() > self.compress_limit { - self.compress_looptokens_list(); - } - self.looptokens_wrefs.push(std::sync::Arc::downgrade(flag)); - } - - /// `quasiimmut.py:77-82 compress_looptokens_list` — drop the entries whose - /// loop is gone and re-derive the limit from what is left, so a type that - /// is recompiled many times and never mutated cannot grow without bound. - /// - /// Upstream's note that already-invalidated tokens must be kept applies - /// here too: the flag stays live while its artifact does, and re-flipping - /// an already-set flag is what keeps a multiply-invalidated loop revoked. - fn compress_looptokens_list(&mut self) { - self.looptokens_wrefs.retain(|w| w.strong_count() > 0); - self.compress_limit = (self.looptokens_wrefs.len() + 15) * 2; - } - - /// `quasiimmut.py:84-109 invalidate` — every loop recorded here becomes - /// invalid, so each `GUARD_NOT_INVALIDATED` in it (and in its bridges) must - /// now fail. The list is emptied like upstream's `self.looptokens_wrefs = - /// []`; a loop that recompiles registers again. - /// - /// `#[dont_look_inside]` (`@jit.dont_look_inside`, `rlib/jit.py:139`): - /// upstream never reaches `invalidate` from traced code — it hangs off the - /// residual `jit_force_quasi_immutable` path — and the `Vec` walk has no - /// lowering either way. The caller's null check stays traced, so the - /// common no-watcher mutation still makes no call. - #[majit_macros::dont_look_inside] - fn invalidate(&mut self) { - for wref in std::mem::take(&mut self.looptokens_wrefs) { - if let Some(flag) = wref.upgrade() { - flag.store(true, std::sync::atomic::Ordering::Release); - } - } - } -} - /// Register a compiled loop's invalidation flag against this type's -/// `_version_tag?` (`quasiimmut.py:72-74 QuasiImmut.register_loop_token`). +/// `_version_tag?` (`quasiimmut.py:72-74 QuasiImmut.register_loop_token`), +/// creating the instance on demand exactly like `get_current_qmut_instance` +/// (quasiimmut.py:116-126). /// /// The compile-time glue (`register_quasi_immutable_deps`) calls this once per /// type-keyed dependency the optimizer collected from a `QUASIIMMUT_FIELD`, so @@ -841,20 +771,31 @@ pub unsafe fn w_type_register_quasi_immut_watcher( if obj.is_null() || !is_type(obj) { return; } - let w_type = &mut *(obj as *mut W_TypeObject); - if w_type.quasi_immut_watchers.is_null() { - w_type.quasi_immut_watchers = Box::into_raw(Box::new(QuasiImmut::new())); - } - (*w_type.quasi_immut_watchers).register_loop_token(flag); + (*(obj as *const W_TypeObject)) + .quasi_immut_watchers + .register_loop_token(flag); } /// Revoke every loop that baked this type's `version_tag` as a constant -/// (`quasiimmut.py:84-109 QuasiImmut.invalidate`). +/// (`quasiimmut.py:129-134 make_invalidation_function._invalidate_now`). +/// +/// ```text +/// def _invalidate_now(p): +/// qmut_ptr = getattr(p, mutatefieldname) +/// setattr(p, mutatefieldname, lltype.nullptr(rclass.OBJECT)) +/// qmut = cast_base_ptr_to_instance(QuasiImmut, qmut_ptr) +/// qmut.invalidate(descr_repr) +/// ``` /// -/// Called from [`w_type_set_version_tag`] right before the new tag is -/// published. Upstream runs the whole walk outside traced code — `invalidate` -/// is reached from the residual `jit_force_quasi_immutable` path, never from a -/// trace — so the sweep is residualised the same way. The null check stays +/// The field is unlinked *before* the sweep and the instance becomes garbage +/// right after it, so the next registration allocates a fresh one; dropping the +/// [`Box`] here is that collection. Called from [`w_type_set_version_tag`] right +/// before the new tag is published. +/// +/// Upstream runs the whole walk outside traced code — `_invalidate_now` is +/// reached from the residual `jit_force_quasi_immutable` path, never from a +/// trace — so the sweep is residualised the same way +/// ([`crate::quasiimmut::sweep_quasi_immut_field`]). The installed check stays /// traced, so mutating a type no loop depends on still makes no call. /// /// # Safety @@ -863,11 +804,11 @@ pub unsafe fn w_type_notify_quasi_immut_watchers(obj: PyObjectRef) { if obj.is_null() || !is_type(obj) { return; } - let w_type = &mut *(obj as *mut W_TypeObject); - if w_type.quasi_immut_watchers.is_null() { + let field = &(*(obj as *const W_TypeObject)).quasi_immut_watchers; + if !field.is_installed() { return; } - (*w_type.quasi_immut_watchers).invalidate(); + crate::quasiimmut::sweep_quasi_immut_field(field); } /// typeobject.py:183-185 `uses_object_getattribute` reader. Returns the @@ -1268,9 +1209,6 @@ pub unsafe fn w_type_add_subclass(w_parent: PyObjectRef, w_subclass: PyObjectRef return; } let parent = &mut *(w_parent as *mut W_TypeObject); - // Builtin parents need the prebuilt root walk; this is harmless for a - // GC-managed heap parent. - crate::gc_roots::mark_prebuilt_roots_dirty(); if parent.weak_subclasses.is_null() { parent.weak_subclasses = Box::into_raw(Box::new(Vec::new())); } @@ -1287,11 +1225,34 @@ pub unsafe fn w_type_add_subclass(w_parent: PyObjectRef, w_subclass: PyObjectRef } if existing.is_null() { subs[i] = newref; - crate::gc_hook::try_gc_write_barrier(w_parent as *mut u8); + note_weak_subclass_store(w_parent); return; } } subs.push(newref); + note_weak_subclass_store(w_parent); +} + +/// Record the store of a freshly allocated weakref into `weak_subclasses`. +/// +/// The mark covers builtin parents, whose list is off-GC and reachable only +/// through `walk_builtin_type_dicts_gc`; the write barrier covers GC-managed +/// heap parents, whose list is forwarded by the `W_TYPE_GC_TYPE_ID` custom +/// trace. +/// +/// Order matters, and it is the safepoint that fixes it, not the allocation: +/// host-side allocation cannot collect (`dynasm_alloc_nursery_typed` routes to +/// `try_alloc_nursery_no_collect_typed` and spills to old-gen on nursery full), +/// but `try_gc_write_barrier` reaches `gc_sync::gc_op`, which leaves RUNNING and +/// parks on `gc_mutex` — an entry-style safepoint where another thread's +/// stop-the-world collection runs. The dirty bit is consumable +/// (`gc_roots::clear_prebuilt_roots_dirty` after each walk), so marking on the +/// far side of that safepoint would let a collection walk the prebuilt family +/// with the slot already updated and the bit still clear, and nothing else roots +/// the young weakref. Mark first, then take the barrier. +#[inline] +unsafe fn note_weak_subclass_store(w_parent: PyObjectRef) { + crate::gc_roots::mark_prebuilt_roots_dirty(); crate::gc_hook::try_gc_write_barrier(w_parent as *mut u8); } @@ -1468,57 +1429,48 @@ mod tests { ); } - /// `quasiimmut.py:84-109 invalidate` flips every registered flag and empties - /// the list, and a flag whose loop is gone is simply skipped. + /// The `_version_tag?` wiring: publishing a new tag runs the invalidation + /// function (`quasiimmut.py:129-134 _invalidate_now`), so the loops that + /// baked the old tag are revoked and the field is left uninstalled. + /// `quasiimmut::tests` covers the field itself. #[test] - fn quasi_immut_invalidate_flips_live_flags_and_clears() { + fn version_tag_write_unlinks_the_instance_and_revokes_its_loops() { use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; - let mut qi = QuasiImmut::new(); - let live = Arc::new(AtomicBool::new(false)); - let dead = Arc::new(AtomicBool::new(false)); - qi.register_loop_token(&live); - qi.register_loop_token(&dead); - drop(dead); + let obj = w_type_new("Quasi", PY_NULL, std::ptr::null_mut()); + let w_type = unsafe { &*(obj as *const W_TypeObject) }; + assert!( + !w_type.quasi_immut_watchers.is_installed(), + "no instance until the first registration", + ); + + let flag = Arc::new(AtomicBool::new(false)); + unsafe { w_type_register_quasi_immut_watcher(obj, &flag) }; + assert!(w_type.quasi_immut_watchers.is_installed()); - qi.invalidate(); - assert!(live.load(Ordering::Acquire), "a live loop must be revoked"); + unsafe { w_type_set_version_tag(obj, new_version_tag()) }; + assert!(flag.load(Ordering::Acquire), "the loop must be revoked"); assert!( - qi.looptokens_wrefs.is_empty(), - "invalidate empties the list; a recompile registers again", + !w_type.quasi_immut_watchers.is_installed(), + "the field is nulled before the sweep", ); - // A second invalidate with nothing registered is a no-op, and the flag - // stays set — `GUARD_NOT_INVALIDATED` has no un-set edge. - qi.invalidate(); - assert!(live.load(Ordering::Acquire)); + // A second bump with nothing registered must not double-free. + unsafe { w_type_set_version_tag(obj, new_version_tag()) }; } - /// `quasiimmut.py:72-82` — a type recompiled many times and never mutated - /// must not grow an unbounded watcher list. + /// A non-type pointer must not be walked as one. #[test] - fn quasi_immut_register_compresses_dead_loop_tokens() { + fn quasi_immut_watcher_helpers_ignore_non_types() { use std::sync::Arc; use std::sync::atomic::AtomicBool; - let mut qi = QuasiImmut::new(); - assert_eq!(qi.compress_limit, 30, "quasiimmut.py:57 compress_limit"); - for _ in 0..500 { - // Each "recompile" drops its artifact immediately, so every - // registered weak ref is already dead by the next round. - let flag = Arc::new(AtomicBool::new(false)); - qi.register_loop_token(&flag); + let flag = Arc::new(AtomicBool::new(false)); + unsafe { + w_type_register_quasi_immut_watcher(PY_NULL, &flag); + w_type_notify_quasi_immut_watchers(PY_NULL); } - assert!( - qi.looptokens_wrefs.len() <= qi.compress_limit + 1, - "compress must bound the list, got {} against limit {}", - qi.looptokens_wrefs.len(), - qi.compress_limit, - ); - // With every entry dead the limit collapses back to the empty-list - // value rather than ratcheting upward. - assert_eq!(qi.compress_limit, 30); } #[test]