diff --git a/majit/majit-ir/src/descr.rs b/majit/majit-ir/src/descr.rs index af84018987b..d9b9095dd2c 100644 --- a/majit/majit-ir/src/descr.rs +++ b/majit/majit-ir/src/descr.rs @@ -754,59 +754,6 @@ static FIELD_INDEX_REDERIVED: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); static FIELD_INDEX_UNRESOLVED: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_DISAGREE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_OFFSET: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_SIZE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_TYPE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_IMMUTABILITY: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_VIRTUALIZABLE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_CACHE_HIT_INDEX_IN_PARENT: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); - -static FIELD_OFFSET_LAYOUT_HIT: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_OFFSET_ACCUMULATOR_FALLBACK: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static COMPUTE_STRUCT_SIZE_LAYOUT: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static COMPUTE_STRUCT_SIZE_HEURISTIC: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static COMPUTE_STRUCT_SIZE_FIELDS_MISSING: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELD_OWNER_ID_REGISTRY_MISS: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); - -static FIELDLESS_SIZE_SHELL_MINTS: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static FIELDLESS_SIZE_SHELL_UPGRADES: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); - -static EI_DESCR_MINT_DIFFERING: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_IDENTICAL: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_STRUCT_SIZE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_OFFSET: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_FIELD_SIZE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_FIELD_TYPE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_FLAG: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_INDEX_IN_PARENT: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_IMMUTABLE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); -static EI_DESCR_MINT_QUASI_IMMUTABLE: std::sync::atomic::AtomicUsize = - std::sync::atomic::AtomicUsize::new(0); - static FIELD_MINT_TRACE_ENABLED: OnceLock = OnceLock::new(); static FIELD_MINT_BACKTRACE_ENABLED: OnceLock = OnceLock::new(); @@ -818,74 +765,92 @@ pub enum StructSizePath { FieldsMissing, } -/// Release-safe field-mint census snapshotted across the build/runtime process -/// boundary by `pyre-jit-trace`'s build script. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] -pub struct FieldMintCensus { - pub cache_hit_disagree: usize, - pub cache_hit_offset: usize, - pub cache_hit_size: usize, - pub cache_hit_type: usize, - pub cache_hit_immutability: usize, - pub cache_hit_virtualizable: usize, - pub cache_hit_index_in_parent: usize, - pub offset_layout_hit: usize, - pub offset_accumulator_fallback: usize, - pub struct_size_layout: usize, - pub struct_size_heuristic: usize, - pub struct_size_fields_missing: usize, - pub owner_id_registry_miss: usize, - pub fieldless_size_shell_mints: usize, - pub fieldless_size_shell_upgrades: usize, - pub ei_differing: usize, - pub ei_identical: usize, - pub ei_struct_size: usize, - pub ei_offset: usize, - pub ei_field_size: usize, - pub ei_field_type: usize, - pub ei_flag: usize, - pub ei_index_in_parent: usize, - pub ei_immutable: usize, - pub ei_quasi_immutable: usize, -} - -impl std::ops::Add for FieldMintCensus { - type Output = Self; - - fn add(self, rhs: Self) -> Self { - Self { - cache_hit_disagree: self.cache_hit_disagree + rhs.cache_hit_disagree, - cache_hit_offset: self.cache_hit_offset + rhs.cache_hit_offset, - cache_hit_size: self.cache_hit_size + rhs.cache_hit_size, - cache_hit_type: self.cache_hit_type + rhs.cache_hit_type, - cache_hit_immutability: self.cache_hit_immutability + rhs.cache_hit_immutability, - cache_hit_virtualizable: self.cache_hit_virtualizable + rhs.cache_hit_virtualizable, - cache_hit_index_in_parent: self.cache_hit_index_in_parent - + rhs.cache_hit_index_in_parent, - offset_layout_hit: self.offset_layout_hit + rhs.offset_layout_hit, - offset_accumulator_fallback: self.offset_accumulator_fallback - + rhs.offset_accumulator_fallback, - struct_size_layout: self.struct_size_layout + rhs.struct_size_layout, - struct_size_heuristic: self.struct_size_heuristic + rhs.struct_size_heuristic, - struct_size_fields_missing: self.struct_size_fields_missing - + rhs.struct_size_fields_missing, - owner_id_registry_miss: self.owner_id_registry_miss + rhs.owner_id_registry_miss, - fieldless_size_shell_mints: self.fieldless_size_shell_mints - + rhs.fieldless_size_shell_mints, - fieldless_size_shell_upgrades: self.fieldless_size_shell_upgrades - + rhs.fieldless_size_shell_upgrades, - ei_differing: self.ei_differing + rhs.ei_differing, - ei_identical: self.ei_identical + rhs.ei_identical, - ei_struct_size: self.ei_struct_size + rhs.ei_struct_size, - ei_offset: self.ei_offset + rhs.ei_offset, - ei_field_size: self.ei_field_size + rhs.ei_field_size, - ei_field_type: self.ei_field_type + rhs.ei_field_type, - ei_flag: self.ei_flag + rhs.ei_flag, - ei_index_in_parent: self.ei_index_in_parent + rhs.ei_index_in_parent, - ei_immutable: self.ei_immutable + rhs.ei_immutable, - ei_quasi_immutable: self.ei_quasi_immutable + rhs.ei_quasi_immutable, +/// Which source supplied one field offset. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum FieldOffsetSource { + ConcreteHit, + TemplateHit, + AccumulatorFallback, +} + +macro_rules! define_field_mint_census { + ($($name:ident),+ $(,)?) => { + struct FieldMintCounters { + $($name: std::sync::atomic::AtomicUsize,)+ } - } + + static FIELD_MINT: FieldMintCounters = FieldMintCounters { + $($name: std::sync::atomic::AtomicUsize::new(0),)+ + }; + + /// Zero every field-mint counter, so a subsequent snapshot describes + /// only the work done after this call. `pyre-jit-trace`'s build script + /// runs code generation twice in one process under + /// `PYRE_CODEGEN_DETERMINISM_CHECK=in-process`; without this the second + /// generation's census would carry the first generation's increments. + pub fn reset_field_mint_census() { + $(FIELD_MINT.$name.store(0, std::sync::atomic::Ordering::Relaxed);)+ + } + + /// Release-safe field-mint census snapshotted across the build/runtime process + /// boundary by `pyre-jit-trace`'s build script. + #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] + pub struct FieldMintCensus { + $(pub $name: usize,)+ + } + + impl FieldMintCensus { + pub fn fields(&self) -> [(&'static str, usize); [$(stringify!($name)),+].len()] { + [$( (stringify!($name), self.$name), )+] + } + } + + impl std::ops::Add for FieldMintCensus { + type Output = Self; + + fn add(self, rhs: Self) -> Self { + Self { + $($name: self.$name + rhs.$name,)+ + } + } + } + + pub fn field_mint_census_snapshot() -> FieldMintCensus { + FieldMintCensus { + $($name: FIELD_MINT.$name.load(std::sync::atomic::Ordering::Relaxed),)+ + } + } + }; +} + +define_field_mint_census! { + cache_hit_disagree, + cache_hit_offset, + cache_hit_size, + cache_hit_type, + cache_hit_immutability, + cache_hit_virtualizable, + cache_hit_index_in_parent, + offset_layout_hit, + offset_accumulator_fallback, + struct_size_layout, + struct_size_heuristic, + struct_size_fields_missing, + owner_id_registry_miss, + fieldless_size_shell_mints, + fieldless_size_shell_upgrades, + ei_differing, + ei_identical, + ei_struct_size, + ei_offset, + ei_field_size, + ei_field_type, + ei_flag, + ei_index_in_parent, + ei_immutable, + ei_quasi_immutable, + offset_concrete_hit, + offset_template_hit, } pub fn field_mint_trace_enabled() -> bool { @@ -898,57 +863,38 @@ fn field_mint_backtrace_enabled() -> bool { *FIELD_MINT_BACKTRACE_ENABLED.get_or_init(|| std::env::var_os("RUST_BACKTRACE").is_some()) } -pub fn record_field_offset_source(layout_hit: bool) { +pub fn record_field_offset_source(source: FieldOffsetSource) { use std::sync::atomic::Ordering::Relaxed; - if layout_hit { - FIELD_OFFSET_LAYOUT_HIT.fetch_add(1, Relaxed); - } else { - FIELD_OFFSET_ACCUMULATOR_FALLBACK.fetch_add(1, Relaxed); + match source { + FieldOffsetSource::ConcreteHit => { + FIELD_MINT.offset_layout_hit.fetch_add(1, Relaxed); + FIELD_MINT.offset_concrete_hit.fetch_add(1, Relaxed); + } + FieldOffsetSource::TemplateHit => { + FIELD_MINT.offset_layout_hit.fetch_add(1, Relaxed); + FIELD_MINT.offset_template_hit.fetch_add(1, Relaxed); + } + FieldOffsetSource::AccumulatorFallback => { + FIELD_MINT.offset_accumulator_fallback.fetch_add(1, Relaxed); + } } } pub fn record_compute_struct_size_path(path: StructSizePath) { use std::sync::atomic::Ordering::Relaxed; match path { - StructSizePath::Layout => COMPUTE_STRUCT_SIZE_LAYOUT.fetch_add(1, Relaxed), - StructSizePath::Heuristic => COMPUTE_STRUCT_SIZE_HEURISTIC.fetch_add(1, Relaxed), - StructSizePath::FieldsMissing => COMPUTE_STRUCT_SIZE_FIELDS_MISSING.fetch_add(1, Relaxed), + StructSizePath::Layout => FIELD_MINT.struct_size_layout.fetch_add(1, Relaxed), + StructSizePath::Heuristic => FIELD_MINT.struct_size_heuristic.fetch_add(1, Relaxed), + StructSizePath::FieldsMissing => { + FIELD_MINT.struct_size_fields_missing.fetch_add(1, Relaxed) + } }; } pub fn record_field_owner_id_registry_miss() { - FIELD_OWNER_ID_REGISTRY_MISS.fetch_add(1, std::sync::atomic::Ordering::Relaxed); -} - -pub fn field_mint_census_snapshot() -> FieldMintCensus { - use std::sync::atomic::Ordering::Relaxed; - FieldMintCensus { - cache_hit_disagree: FIELD_CACHE_HIT_DISAGREE.load(Relaxed), - cache_hit_offset: FIELD_CACHE_HIT_OFFSET.load(Relaxed), - cache_hit_size: FIELD_CACHE_HIT_SIZE.load(Relaxed), - cache_hit_type: FIELD_CACHE_HIT_TYPE.load(Relaxed), - cache_hit_immutability: FIELD_CACHE_HIT_IMMUTABILITY.load(Relaxed), - cache_hit_virtualizable: FIELD_CACHE_HIT_VIRTUALIZABLE.load(Relaxed), - cache_hit_index_in_parent: FIELD_CACHE_HIT_INDEX_IN_PARENT.load(Relaxed), - offset_layout_hit: FIELD_OFFSET_LAYOUT_HIT.load(Relaxed), - offset_accumulator_fallback: FIELD_OFFSET_ACCUMULATOR_FALLBACK.load(Relaxed), - struct_size_layout: COMPUTE_STRUCT_SIZE_LAYOUT.load(Relaxed), - struct_size_heuristic: COMPUTE_STRUCT_SIZE_HEURISTIC.load(Relaxed), - struct_size_fields_missing: COMPUTE_STRUCT_SIZE_FIELDS_MISSING.load(Relaxed), - owner_id_registry_miss: FIELD_OWNER_ID_REGISTRY_MISS.load(Relaxed), - fieldless_size_shell_mints: FIELDLESS_SIZE_SHELL_MINTS.load(Relaxed), - fieldless_size_shell_upgrades: FIELDLESS_SIZE_SHELL_UPGRADES.load(Relaxed), - ei_differing: EI_DESCR_MINT_DIFFERING.load(Relaxed), - ei_identical: EI_DESCR_MINT_IDENTICAL.load(Relaxed), - ei_struct_size: EI_DESCR_MINT_STRUCT_SIZE.load(Relaxed), - ei_offset: EI_DESCR_MINT_OFFSET.load(Relaxed), - ei_field_size: EI_DESCR_MINT_FIELD_SIZE.load(Relaxed), - ei_field_type: EI_DESCR_MINT_FIELD_TYPE.load(Relaxed), - ei_flag: EI_DESCR_MINT_FLAG.load(Relaxed), - ei_index_in_parent: EI_DESCR_MINT_INDEX_IN_PARENT.load(Relaxed), - ei_immutable: EI_DESCR_MINT_IMMUTABLE.load(Relaxed), - ei_quasi_immutable: EI_DESCR_MINT_QUASI_IMMUTABLE.load(Relaxed), - } + FIELD_MINT + .owner_id_registry_miss + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); } /// One `FIELD_INDEX_UNRESOLVED` event, named, keyed so identical mints fold @@ -1401,7 +1347,9 @@ impl GcCache { sd.set_cache_key(*k); } sd.mark_fieldless_shell_mint(); - FIELDLESS_SIZE_SHELL_MINTS.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + FIELD_MINT + .fieldless_size_shell_mints + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); // descr.py:119: gccache.init_size_descr(STRUCT, sizedescr) // gc.py:536-542: sets descr.tid — must happen BEFORE Arc wrap. self.init_size_descr(&key, &mut sd); @@ -2089,24 +2037,24 @@ impl GcCache { || index_disagrees; if disagrees { use std::sync::atomic::Ordering::Relaxed; - FIELD_CACHE_HIT_DISAGREE.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_disagree.fetch_add(1, Relaxed); if offset_disagrees { - FIELD_CACHE_HIT_OFFSET.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_offset.fetch_add(1, Relaxed); } if size_disagrees { - FIELD_CACHE_HIT_SIZE.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_size.fetch_add(1, Relaxed); } if type_disagrees { - FIELD_CACHE_HIT_TYPE.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_type.fetch_add(1, Relaxed); } if immutability_disagrees { - FIELD_CACHE_HIT_IMMUTABILITY.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_immutability.fetch_add(1, Relaxed); } if virtualizable_disagrees { - FIELD_CACHE_HIT_VIRTUALIZABLE.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_virtualizable.fetch_add(1, Relaxed); } if index_disagrees { - FIELD_CACHE_HIT_INDEX_IN_PARENT.fetch_add(1, Relaxed); + FIELD_MINT.cache_hit_index_in_parent.fetch_add(1, Relaxed); } if field_mint_trace_enabled() { let cached_parent_has_positional_list = descr @@ -2555,7 +2503,9 @@ impl GcCache { .as_size_descr() .is_some_and(|sd| !sd.all_fielddescrs().is_empty()); if upgrades_fieldless_shell { - FIELDLESS_SIZE_SHELL_UPGRADES.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + FIELD_MINT + .fieldless_size_shell_upgrades + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); } if let Some(old) = self._cache_size.get(&key) && !arc_in_vec(&self._size_keepalive, old) @@ -2931,28 +2881,28 @@ pub fn record_ei_descr_mint(member: DescrSetMember, spec: DescrMintSpec) { return; }; if stored_struct_size != rejected_struct_size { - EI_DESCR_MINT_STRUCT_SIZE.fetch_add(1, Relaxed); + FIELD_MINT.ei_struct_size.fetch_add(1, Relaxed); } if stored_offset != rejected_offset { - EI_DESCR_MINT_OFFSET.fetch_add(1, Relaxed); + FIELD_MINT.ei_offset.fetch_add(1, Relaxed); } if stored_field_size != rejected_field_size { - EI_DESCR_MINT_FIELD_SIZE.fetch_add(1, Relaxed); + FIELD_MINT.ei_field_size.fetch_add(1, Relaxed); } if stored_field_type != rejected_field_type { - EI_DESCR_MINT_FIELD_TYPE.fetch_add(1, Relaxed); + FIELD_MINT.ei_field_type.fetch_add(1, Relaxed); } if stored_flag != rejected_flag { - EI_DESCR_MINT_FLAG.fetch_add(1, Relaxed); + FIELD_MINT.ei_flag.fetch_add(1, Relaxed); } if stored_index_in_parent != rejected_index_in_parent { - EI_DESCR_MINT_INDEX_IN_PARENT.fetch_add(1, Relaxed); + FIELD_MINT.ei_index_in_parent.fetch_add(1, Relaxed); } if stored_immutable != rejected_immutable { - EI_DESCR_MINT_IMMUTABLE.fetch_add(1, Relaxed); + FIELD_MINT.ei_immutable.fetch_add(1, Relaxed); } if stored_quasi_immutable != rejected_quasi_immutable { - EI_DESCR_MINT_QUASI_IMMUTABLE.fetch_add(1, Relaxed); + FIELD_MINT.ei_quasi_immutable.fetch_add(1, Relaxed); } } @@ -2964,9 +2914,9 @@ pub fn record_ei_descr_mint(member: DescrSetMember, spec: DescrMintSpec) { indexmap::map::Entry::Occupied(entry) => { let stored = entry.get(); if stored == &spec { - EI_DESCR_MINT_IDENTICAL.fetch_add(1, Relaxed); + FIELD_MINT.ei_identical.fetch_add(1, Relaxed); } else { - EI_DESCR_MINT_DIFFERING.fetch_add(1, Relaxed); + FIELD_MINT.ei_differing.fetch_add(1, Relaxed); count_field_axes(stored, &spec); if field_mint_trace_enabled() { eprintln!( diff --git a/majit/majit-translate/src/codewriter/call.rs b/majit/majit-translate/src/codewriter/call.rs index d50fdee642f..23616988112 100644 --- a/majit/majit-translate/src/codewriter/call.rs +++ b/majit/majit-translate/src/codewriter/call.rs @@ -1435,7 +1435,7 @@ pub struct StructLayout { } /// Single field within a `StructLayout`. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq)] pub struct StructFieldLayout { pub name: String, /// RPython: `cfield.offset` @@ -2493,13 +2493,27 @@ impl CallControl { } let (struct_size, struct_size_path) = compute_struct_size_with_path(self, owner_root); - let exact_field_offset = owner_id - .or(registry_struct_id) - .and_then(|sid| self.struct_layouts.get(&sid)) - .and_then(|l| l.fields.iter().find(|f| f.name.as_str() == field_name)) - .map(|f| f.offset); - majit_ir::descr::record_field_offset_source(exact_field_offset.is_some()); - let field_offset = exact_field_offset.unwrap_or(offset); + let offset_of = |sid| { + self.struct_layouts + .get(&sid) + .and_then(|l| l.fields.iter().find(|f| f.name.as_str() == field_name)) + .map(|f| f.offset) + }; + let concrete_offset = owner_id.and_then(offset_of); + let template_offset = if concrete_offset.is_none() { + registry_struct_id.and_then(offset_of) + } else { + None + }; + let field_offset_source = if concrete_offset.is_some() { + majit_ir::descr::FieldOffsetSource::ConcreteHit + } else if template_offset.is_some() { + majit_ir::descr::FieldOffsetSource::TemplateHit + } else { + majit_ir::descr::FieldOffsetSource::AccumulatorFallback + }; + majit_ir::descr::record_field_offset_source(field_offset_source); + let field_offset = concrete_offset.or(template_offset).unwrap_or(offset); let rank = self.field_immutability(Some(owner_root), field_name); let is_immutable = rank.map(|r| r.is_immutable()).unwrap_or(false); let is_quasi_immutable = rank.map(|r| r.is_quasi_immutable()).unwrap_or(false); diff --git a/majit/majit-translate/src/lib.rs b/majit/majit-translate/src/lib.rs index 1c7ea704b0e..a9ffdecaea3 100644 --- a/majit/majit-translate/src/lib.rs +++ b/majit/majit-translate/src/lib.rs @@ -861,18 +861,6 @@ fn expand_immutable_fields_to_all_spellings( out } -fn struct_layout_fields_equal(left: &[StructFieldLayout], right: &[StructFieldLayout]) -> bool { - left.len() == right.len() - && left.iter().zip(right).all(|(left, right)| { - left.name == right.name - && left.offset == right.offset - && left.size == right.size - && left.flag == right.flag - && left.field_type == right.field_type - && left.rank == right.rank - }) -} - fn struct_layout_census_enabled() -> bool { std::env::var_os("MAJIT_STRUCT_LAYOUT_CENSUS") .is_some_and(|value| value == std::ffi::OsStr::new("1")) @@ -1327,8 +1315,7 @@ fn analyze_pipeline_from_module_paths( if census_struct_layouts { let variants = struct_layout_variants.entry(sid).or_default(); if let Some((_, spellings)) = variants.iter_mut().find(|(observed, _)| { - observed.size == layout.size - && struct_layout_fields_equal(&observed.fields, &layout.fields) + observed.size == layout.size && observed.fields == layout.fields }) { spellings.push(struct_name.clone()); } else { diff --git a/majit/majit-translate/src/model.rs b/majit/majit-translate/src/model.rs index 1f9625c4644..550c4b0a813 100644 --- a/majit/majit-translate/src/model.rs +++ b/majit/majit-translate/src/model.rs @@ -2463,8 +2463,58 @@ pub fn eliminate_empty_blocks(graph: &mut FunctionGraph) { /// Returns the number of removed exits so the caller can gate the /// follow-up dead-condition sweep (`removeassert.py:35-37` — "now melt /// away the (hopefully) dead operation that compute the condition"). +/// The front model does not run `join_blocks` over this graph type, so +/// the assertion raise can remain behind a single-entry, single-exit block +/// that upstream `simplify.py:271-319` would have collapsed. pub fn remove_assertion_errors(graph: &mut FunctionGraph) -> usize { - use crate::flowspace::model::HOST_ENV; + use crate::flowspace::model::{HOST_ENV, HostObject}; + + fn targets_assertion_error( + graph: &FunctionGraph, + exit: &Link, + exceptblock: BlockId, + assert_err_class: &HostObject, + ) -> bool { + let indirect = exit.target != exceptblock; + let assertion_exit = if indirect { + let target = graph.block(exit.target); + if target.exits.len() != 1 { + return false; + } + &target.exits[0] + } else { + exit + }; + + let raises_assertion_error = assertion_exit.target == exceptblock + && matches!( + assertion_exit.args.first(), + Some(LinkArg::Const(c)) + if matches!( + &c.value, + ConstValue::HostObject(h) if h == assert_err_class + ) + ); + if !raises_assertion_error { + return false; + } + if !indirect { + return true; + } + // Only `join_blocks` would collapse the intermediate block into this + // one, and only when this exit is its sole entry — otherwise removing + // the exit strands the other predecessors. Counted last: it is the + // one whole-graph scan here, and the cheap shape tests above already + // reject every exit that is not a raise-block edge. + graph + .blocks + .iter() + .flat_map(|block| block.exits.iter()) + .filter(|candidate| candidate.target == exit.target) + .count() + == 1 + } + let assert_err_class = HOST_ENV .lookup_builtin("AssertionError") .expect("HOST_ENV missing AssertionError"); @@ -2481,16 +2531,7 @@ pub fn remove_assertion_errors(graph: &mut FunctionGraph) -> usize { }; // upstream: `if not (exit.target is graph.exceptblock and // exit.args[0] == Constant(AssertionError)): continue`. - let targets_except = exit.target == exceptblock; - let args_is_assert_err = matches!( - exit.args.first(), - Some(LinkArg::Const(c)) - if matches!( - &c.value, - ConstValue::HostObject(h) if *h == assert_err_class - ) - ); - if !(targets_except && args_is_assert_err) { + if !targets_assertion_error(graph, exit, exceptblock, &assert_err_class) { continue; } // upstream: `if len(block.exits) < 2: break`. @@ -6372,6 +6413,126 @@ mod tests { assert_eq!(entry_block.exits[0].args, vec![LinkArg::Value(value_var)]); } + fn assertion_error_except_link(graph: &FunctionGraph, exitcase: Option) -> Link { + Link::new_mixed( + vec![ + LinkArg::from(ConstValue::builtin("AssertionError")), + LinkArg::from(ConstValue::None), + ], + graph.exceptblock, + exitcase, + ) + .with_llexitcase_from_exitcase() + } + + #[test] + fn remove_assertion_errors_prunes_direct_exceptblock_exit() { + let mut graph = FunctionGraph::new("direct_assertion_error"); + let entry = graph.startblock; + let live = graph.create_block(); + let cond = graph + .push_op_var( + entry, + OpKind::Input { + name: "cond".into(), + ty: ValueType::Bool, + class_root: None, + }, + true, + ) + .unwrap(); + let survivor = Link::from_variables(&graph, vec![], live, Some(ExitCase::Bool(false))) + .with_llexitcase_from_exitcase(); + let assertion = assertion_error_except_link(&graph, Some(ExitCase::Bool(true))); + graph.set_control_flow_metadata( + entry, + Some(ExitSwitch::Value(cond)), + vec![survivor, assertion], + ); + + assert_eq!(remove_assertion_errors(&mut graph), 1); + let entry_block = graph.block(entry); + assert_eq!(entry_block.exits.len(), 1); + assert_eq!(entry_block.exits[0].target, live); + assert!(entry_block.exitswitch.is_none()); + assert!(entry_block.exits[0].exitcase.is_none()); + assert!(entry_block.exits[0].llexitcase.is_none()); + } + + #[test] + fn remove_assertion_errors_prunes_through_single_entry_raise_block() { + let mut graph = FunctionGraph::new("indirect_assertion_error"); + let entry = graph.startblock; + let live = graph.create_block(); + let raise = graph.create_block(); + let cond = graph + .push_op_var( + entry, + OpKind::Input { + name: "cond".into(), + ty: ValueType::Bool, + class_root: None, + }, + true, + ) + .unwrap(); + graph.set_branch(entry, cond, raise, vec![], live, vec![]); + graph.set_raise_implicit(raise, "ValueError"); + + assert_eq!(remove_assertion_errors(&mut graph), 1); + let entry_block = graph.block(entry); + assert_eq!(entry_block.exits.len(), 1); + assert_eq!(entry_block.exits[0].target, live); + assert!(entry_block.exitswitch.is_none()); + assert!(entry_block.exits[0].exitcase.is_none()); + assert!(entry_block.exits[0].llexitcase.is_none()); + } + + #[test] + fn remove_assertion_errors_keeps_raise_block_with_multiple_entries() { + let mut graph = FunctionGraph::new("shared_assertion_error"); + let entry = graph.startblock; + let other_predecessor = graph.create_block(); + let live = graph.create_block(); + let raise = graph.create_block(); + let cond = graph + .push_op_var( + entry, + OpKind::Input { + name: "cond".into(), + ty: ValueType::Bool, + class_root: None, + }, + true, + ) + .unwrap(); + graph.set_branch(entry, cond, raise, vec![], live, vec![]); + graph.set_goto(other_predecessor, raise, vec![]); + graph.set_raise_implicit(raise, "ValueError"); + + assert_eq!(remove_assertion_errors(&mut graph), 0); + assert_eq!(graph.block(entry).exits.len(), 2); + assert!( + graph + .block(entry) + .exits + .iter() + .any(|exit| exit.target == raise) + ); + assert_eq!(graph.block(other_predecessor).exits[0].target, raise); + } + + #[test] + fn remove_assertion_errors_keeps_whole_graph_single_exit_raise() { + let mut graph = FunctionGraph::new("whole_graph_assertion_error"); + let entry = graph.startblock; + graph.set_raise_implicit(entry, "ValueError"); + + assert_eq!(remove_assertion_errors(&mut graph), 0); + assert_eq!(graph.block(entry).exits.len(), 1); + assert_eq!(graph.block(entry).exits[0].target, graph.exceptblock); + } + #[test] fn set_raise_routes_assertionerror_class_and_instance_to_exceptblock() { use crate::flowspace::model::ConstValue; diff --git a/pyre/pyre-jit-trace/build.rs b/pyre/pyre-jit-trace/build.rs index a7766bad0a8..56f7b47943a 100644 --- a/pyre/pyre-jit-trace/build.rs +++ b/pyre/pyre-jit-trace/build.rs @@ -84,6 +84,21 @@ const HOST_ADDRESSED_OUTPUTS: &[&str] = &[ "static_ref_bindings.bin", ]; +/// Outputs whose contents count work against process-global state and therefore +/// cannot be compared between two generations in the same process. +/// +/// `field_mint_census.bin` includes `fieldless_size_shell_mints` and +/// `ei_identical`. The process-global GcCache and ei-descr ledger persist across +/// the two generations, so the second generation legitimately does less work: +/// it mints no fieldless shells and finds more identical entries already stored. +/// The reset at the start of each generation remains necessary so the file +/// describes the generation that wrote it instead of accumulating both runs. +/// +/// Excluded from the in-process verdict only. A cross-process cache comparison +/// still judges the census, unlike [`HOST_ADDRESSED_OUTPUTS`], whose exclusions +/// apply only across processes. +const IN_PROCESS_STATEFUL_OUTPUTS: &[&str] = &["field_mint_census.bin"]; + /// Opt-in self-check that the generated outputs are a function of the inputs /// [`codegen_cache_key`] hashes. Deliberately not part of that key: it changes /// nothing about what gets generated, it only compares two generations. @@ -928,6 +943,7 @@ fn real_main() { // `DeterminismCheck::InProcess` compares against; the parameter shadows // the outer `out_dir` so both calls read as "write into out_dir". let generate_into = |out_dir: &str| { + majit_ir::descr::reset_field_mint_census(); let pipeline = majit_translate::analyze_multiple_pipeline_with_modules( &module_path_refs, &analyze_config, @@ -1143,12 +1159,12 @@ fn real_main() { false, ) } else { - println!( - "cargo::warning=codegen determinism: the second in-process generation \ - panicked (see the panic above), so nothing was compared; the codegen does \ - not survive running twice in one process. Compare across processes with \ - {DETERMINISM_CHECK_ENV}=cache instead" - ); + report_determinism(&format!( + "the second in-process generation panicked (see the panic above), so \ + nothing was compared; the codegen does not survive running twice in one \ + process. Compare across processes with {DETERMINISM_CHECK_ENV}=cache \ + instead" + )); false } } @@ -1168,11 +1184,10 @@ fn real_main() { // say so rather than let an empty comparison read as clean. // Storing below is what gives the next run something to // compare against. - println!( - "cargo::warning=codegen determinism: cache key {cache_key} has no stored \ - entry, so nothing was compared; re-run this build to compare against the \ - entry it is about to store" - ); + report_determinism(&format!( + "cache key {cache_key} has no stored entry, so nothing was compared; \ + re-run this build to compare against the entry it is about to store" + )); true } } @@ -1194,10 +1209,9 @@ fn real_main() { ), _ => "refusing to store them".to_string(), }; - println!( - "cargo::warning=codegen determinism: the outputs at cache key {cache_key} are not \ - reproducible; {consequence}" - ); + report_determinism(&format!( + "the outputs at cache key {cache_key} are not reproducible; {consequence}" + )); return; } @@ -1407,10 +1421,12 @@ fn prune_codegen_cache(repo_root: &str, keep: &std::path::Path) { /// output that cannot be read on one side is an inconclusive comparison, and /// an inconclusive comparison must not report as a clean one. /// -/// Reports through `cargo::warning`, which cargo surfaces for a workspace -/// crate's build script. A bare `println!` / `eprintln!` needs `-vv`, which is -/// how a build-script line documenting the cache restore went unread long -/// enough to be mistaken for the restore not happening. +/// Reports through [`report_determinism`], which writes `cargo::warning` — +/// cargo surfaces that for a workspace crate's build script, where a bare +/// `println!` / `eprintln!` needs `-vv`, which is how a build-script line +/// documenting the cache restore went unread long enough to be mistaken for the +/// restore not happening — and repeats the same finding on stderr for a build +/// script invoked directly. /// /// Names the identical outputs as well as the differing ones. Which files hold /// still localises the cause faster than which files move: `jitcodes.bin` @@ -1421,10 +1437,25 @@ fn prune_codegen_cache(repo_root: &str, keep: &std::path::Path) { /// other only an order — so both lengths are reported, not just that they /// differ. /// -/// `cross_process` excuses [`HOST_ADDRESSED_OUTPUTS`] from the verdict. They -/// are still reported, on their own line: a check that files a by-design -/// difference as a defect is one people learn to ignore, and then it reports -/// nothing at all. +/// `cross_process` excuses [`HOST_ADDRESSED_OUTPUTS`] from the verdict; its +/// inverse excuses [`IN_PROCESS_STATEFUL_OUTPUTS`]. Both are still reported on +/// their own lines: a check that files a by-design difference as a defect is one +/// people learn to ignore, and then it reports nothing at all. +/// Emit one determinism finding on both streams. +/// +/// `cargo::warning` is honoured on stdout only, while every line this build +/// script narrates is an `eprintln!` on stderr. A build script invoked directly +/// — the prepass rig runs one with the two streams captured to separate files — +/// therefore put its pass sentence on stderr and its entire failure report on +/// stdout, so filtering stderr for `codegen determinism` yielded the two +/// exclusion lines and no verdict at all. That reads as a quieter pass, and a +/// failing generation was taken for a passing one on exactly that reading. The +/// direct run also exits 0 either way, so the exit code does not correct it. +fn report_determinism(finding: &str) { + println!("cargo::warning=codegen determinism: {finding}"); + eprintln!("[pyre-jit-trace build.rs] codegen determinism: {finding}"); +} + fn codegen_outputs_match( label: &str, baseline: &std::path::Path, @@ -1434,6 +1465,7 @@ fn codegen_outputs_match( let mut identical: Vec<&str> = Vec::new(); let mut differing: Vec = Vec::new(); let mut host_addressed: Vec<&str> = Vec::new(); + let mut in_process_stateful: Vec<&str> = Vec::new(); let mut unreadable: Vec = Vec::new(); for name in CODEGEN_OUTPUTS { match ( @@ -1444,6 +1476,9 @@ fn codegen_outputs_match( (Ok(_), Ok(_)) if cross_process && HOST_ADDRESSED_OUTPUTS.contains(name) => { host_addressed.push(name); } + (Ok(_), Ok(_)) if !cross_process && IN_PROCESS_STATEFUL_OUTPUTS.contains(name) => { + in_process_stateful.push(name); + } (Ok(a), Ok(b)) => { let offset = a .iter() @@ -1471,6 +1506,14 @@ fn codegen_outputs_match( host_addressed.join(" ") ); } + if !in_process_stateful.is_empty() { + eprintln!( + "[pyre-jit-trace build.rs] codegen determinism: {} process-stateful output(s) \ + differ from {label} as expected, excluded from the in-process verdict: {}", + in_process_stateful.len(), + in_process_stateful.join(" ") + ); + } if differing.is_empty() && unreadable.is_empty() { eprintln!( "[pyre-jit-trace build.rs] codegen determinism: all {} reproducible outputs are \ @@ -1480,17 +1523,21 @@ fn codegen_outputs_match( return true; } for entry in &differing { - println!("cargo::warning=codegen determinism: DIFFERS from {label}: {entry}"); + report_determinism(&format!("DIFFERS from {label}: {entry}")); } for entry in &unreadable { - println!("cargo::warning=codegen determinism: NOT COMPARED against {label}: {entry}"); + report_determinism(&format!("NOT COMPARED against {label}: {entry}")); } - println!( - "cargo::warning=codegen determinism: {} of {} outputs unchanged from {label}: {}", + report_determinism(&format!( + "{} of {} outputs unchanged from {label}: {}", identical.len(), CODEGEN_OUTPUTS.len(), identical.join(" ") - ); + )); + // Both exclusions already went to stderr above, in either outcome. Repeat + // them as warnings so the cargo-visible report of a failure carries what + // was left out of the verdict it just reported; a plain `println!` here + // keeps them off stderr a second time. if !host_addressed.is_empty() { println!( "cargo::warning=codegen determinism: excluded as host addresses (expected to differ \ @@ -1498,6 +1545,13 @@ fn codegen_outputs_match( host_addressed.join(" ") ); } + if !in_process_stateful.is_empty() { + println!( + "cargo::warning=codegen determinism: excluded as process-stateful (expected to \ + differ across in-process generations): {}", + in_process_stateful.join(" ") + ); + } false } diff --git a/pyre/pyre-jit-trace/src/jitcode_runtime.rs b/pyre/pyre-jit-trace/src/jitcode_runtime.rs index aeb25719df0..76e1f32f904 100644 --- a/pyre/pyre-jit-trace/src/jitcode_runtime.rs +++ b/pyre/pyre-jit-trace/src/jitcode_runtime.rs @@ -794,6 +794,27 @@ pub fn field_position_jit_stats() -> String { attached_misplaced, } = field_position_counts(); let mint = *BUILD_FIELD_MINT_CENSUS + majit_ir::descr::field_mint_census_snapshot(); + let mint_fields = mint + .fields() + .into_iter() + .map(|(name, value)| { + let report_name = if let Some(suffix) = name.strip_prefix("cache_hit_") { + format!("field_cache_hit_{suffix}") + } else if let Some(suffix) = name.strip_prefix("offset_") { + format!("field_offset_{suffix}") + } else if let Some(suffix) = name.strip_prefix("struct_size_") { + format!("compute_struct_size_{suffix}") + } else if let Some(suffix) = name.strip_prefix("ei_") { + format!("ei_descr_mint_{suffix}") + } else if name == "owner_id_registry_miss" { + "field_owner_id_registry_miss".to_owned() + } else { + name.to_owned() + }; + format!("{report_name}={value}") + }) + .collect::>() + .join(" "); let ( [published, fieldless, shadowing, aliased, aliased_multi], [slots, misplaced], @@ -820,50 +841,12 @@ pub fn field_position_jit_stats() -> String { field_pos_rederived={rederived} field_pos_unresolved={unresolved} \ field_pos_spec_checked={spec_checked} field_pos_spec_misplaced={spec_misplaced} \ field_pos_attached_checked={attached_checked} \ - field_pos_attached_misplaced={attached_misplaced} \ - field_cache_hit_disagree={} field_cache_hit_offset={} \ - field_cache_hit_size={} field_cache_hit_type={} \ - field_cache_hit_immutability={} field_cache_hit_virtualizable={} \ - field_cache_hit_index_in_parent={} field_offset_layout_hit={} \ - field_offset_accumulator_fallback={} compute_struct_size_layout={} \ - compute_struct_size_heuristic={} compute_struct_size_fields_missing={} \ - field_owner_id_registry_miss={} fieldless_size_shell_mints={} \ - fieldless_size_shell_upgrades={} ei_descr_mint_differing={} \ - ei_descr_mint_identical={} ei_descr_mint_struct_size={} \ - ei_descr_mint_offset={} ei_descr_mint_field_size={} \ - ei_descr_mint_field_type={} ei_descr_mint_flag={} \ - ei_descr_mint_index_in_parent={} ei_descr_mint_immutable={} \ - ei_descr_mint_quasi_immutable={} \ + field_pos_attached_misplaced={attached_misplaced} {mint_fields} \ size_shell_published={published} size_shell_fieldless={fieldless} \ size_shell_shadowing={shadowing} size_shell_aliased={aliased} \ size_shell_aliased_multi={aliased_multi} \ positional_slots={slots} positional_misplaced={misplaced} \ key_compared={key_compared} key_conflicting={key_conflicting}{sample}", - mint.cache_hit_disagree, - mint.cache_hit_offset, - mint.cache_hit_size, - mint.cache_hit_type, - mint.cache_hit_immutability, - mint.cache_hit_virtualizable, - mint.cache_hit_index_in_parent, - mint.offset_layout_hit, - mint.offset_accumulator_fallback, - mint.struct_size_layout, - mint.struct_size_heuristic, - mint.struct_size_fields_missing, - mint.owner_id_registry_miss, - mint.fieldless_size_shell_mints, - mint.fieldless_size_shell_upgrades, - mint.ei_differing, - mint.ei_identical, - mint.ei_struct_size, - mint.ei_offset, - mint.ei_field_size, - mint.ei_field_type, - mint.ei_flag, - mint.ei_index_in_parent, - mint.ei_immutable, - mint.ei_quasi_immutable, ) }