From 3ef05264e40e2f5990274345fe8585ddb29b04ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 17:22:26 +0200 Subject: [PATCH 1/7] fix(perf_hooks): enforce receiver validation (cherry picked from commit 40cd9c6e9fa1ae2710e8117de3a37388fae52164) --- changelog.d/9202-perf-hooks-validation.md | 3 + .../src/object/class_registry/construct.rs | 5 + .../perry-runtime/src/object/native_module.rs | 11 +- .../native_module/constructor_exports.rs | 19 +++ .../native_module/perf_instance_bind.rs | 15 ++ crates/perry-runtime/src/perf_hooks.rs | 16 ++- .../src/perf_hooks/prototypes.rs | 114 ++++++++++++--- .../prototypes/performance_methods.rs | 133 ++++++++++++++++++ .../perf_hooks/shapes/receiver-branding.ts | 8 ++ 9 files changed, 303 insertions(+), 21 deletions(-) create mode 100644 changelog.d/9202-perf-hooks-validation.md create mode 100644 crates/perry-runtime/src/perf_hooks/prototypes/performance_methods.rs diff --git a/changelog.d/9202-perf-hooks-validation.md b/changelog.d/9202-perf-hooks-validation.md new file mode 100644 index 0000000000..334a1b9231 --- /dev/null +++ b/changelog.d/9202-perf-hooks-validation.md @@ -0,0 +1,3 @@ +Make `Performance` methods reject invalid receivers and preserve +`ERR_ILLEGAL_CONSTRUCTOR` when histogram constructor values are invoked with +`new`. diff --git a/crates/perry-runtime/src/object/class_registry/construct.rs b/crates/perry-runtime/src/object/class_registry/construct.rs index 597cac4f79..15ec9f744a 100644 --- a/crates/perry-runtime/src/object/class_registry/construct.rs +++ b/crates/perry-runtime/src/object/class_registry/construct.rs @@ -331,6 +331,11 @@ pub unsafe extern "C-unwind" fn js_new_function_construct( return result; } } + if module == "perf_histogram" + && matches!(method.as_str(), "RecordableHistogram" | "ELDHistogram") + { + return crate::perf_hooks::js_perf_illegal_constructor(); + } if module == "sqlite" && matches!( method.as_str(), diff --git a/crates/perry-runtime/src/object/native_module.rs b/crates/perry-runtime/src/object/native_module.rs index 08e282c625..b7440e4687 100644 --- a/crates/perry-runtime/src/object/native_module.rs +++ b/crates/perry-runtime/src/object/native_module.rs @@ -23,7 +23,7 @@ mod callable_export_check; mod callable_export_table; pub(crate) mod callable_exports; mod perf_instance_bind; -pub(crate) use perf_instance_bind::instance_bound_perf_method; +pub(crate) use perf_instance_bind::{instance_bound_perf_method, performance_namespace_method}; mod constants; mod constants_tables; mod constructor_exports; @@ -1188,6 +1188,12 @@ pub extern "C" fn js_native_module_bind_method( } } + if let Some(value) = + performance_namespace_method(&module_name, property_name, namespace.get_nanbox_f64()) + { + return value; + } + // Check for known constant properties first if let Some(val) = unsafe { get_native_module_constant(&module_name, property_name, namespace.get_nanbox_f64()) @@ -1827,6 +1833,9 @@ unsafe fn vt_get_own_field( if let Some(value) = super::field_get_set::native_module_own_field_by_key(obj, key) { return Some(value); } + if let Some(value) = performance_namespace_method(&module_name, property_name, nb_ptr) { + return Some(JSValue::from_bits(value.to_bits())); + } // #3687: node:cluster default-import EventEmitter methods on the // distinct `cluster.default` namespace (see original comment at the // pre-relocation site in field_get_set.rs history). diff --git a/crates/perry-runtime/src/object/native_module/constructor_exports.rs b/crates/perry-runtime/src/object/native_module/constructor_exports.rs index f0f670e57c..c61960817b 100644 --- a/crates/perry-runtime/src/object/native_module/constructor_exports.rs +++ b/crates/perry-runtime/src/object/native_module/constructor_exports.rs @@ -16,6 +16,13 @@ pub(crate) fn is_native_module_constructor_export(module: &str, property: &str) let module = normalize_native_module_alias(module); let property = canonical_native_callable_property(module, property); + // Histogram constructors are only reachable through an instance's + // `constructor` property. They are callable-shaped internal exports, and + // their construct path deliberately throws ERR_ILLEGAL_CONSTRUCTOR. + if module == "perf_histogram" && matches!(property, "RecordableHistogram" | "ELDHistogram") { + return true; + } + if !is_native_module_callable_export(module, property) { return false; } @@ -207,4 +214,16 @@ mod tests { "WriteStream" )); } + + #[test] + fn histogram_class_values_are_constructor_shaped() { + assert!(is_native_module_constructor_export( + "perf_histogram", + "RecordableHistogram" + )); + assert!(is_native_module_constructor_export( + "perf_histogram", + "ELDHistogram" + )); + } } diff --git a/crates/perry-runtime/src/object/native_module/perf_instance_bind.rs b/crates/perry-runtime/src/object/native_module/perf_instance_bind.rs index 55ecbdc210..eeff7cb3e3 100644 --- a/crates/perry-runtime/src/object/native_module/perf_instance_bind.rs +++ b/crates/perry-runtime/src/object/native_module/perf_instance_bind.rs @@ -54,3 +54,18 @@ pub(crate) fn instance_bound_perf_method( name.len(), )) } + +/// Return the receiver-aware method installed on `Performance.prototype` for +/// reads from the canonical `performance` singleton. The singleton shares the +/// `perf_hooks` dispatch tag with the module namespace, so identity distinguishes +/// these methods from ordinary native-module exports. +pub(crate) fn performance_namespace_method( + module_name: &str, + property_name: &str, + receiver: f64, +) -> Option { + if module_name != "perf_hooks" || !crate::perf_hooks::is_performance_namespace_value(receiver) { + return None; + } + crate::perf_hooks::performance_prototype_method_value(property_name) +} diff --git a/crates/perry-runtime/src/perf_hooks.rs b/crates/perry-runtime/src/perf_hooks.rs index c737dcadd5..68690d9d80 100644 --- a/crates/perry-runtime/src/perf_hooks.rs +++ b/crates/perry-runtime/src/perf_hooks.rs @@ -38,7 +38,10 @@ use std::time::{Instant, SystemTime, UNIX_EPOCH}; mod prototypes; -pub(crate) use prototypes::{attach_perf_hooks_constructor, perf_supported_entry_types_value}; +pub(crate) use prototypes::{ + attach_perf_hooks_constructor, perf_supported_entry_types_value, + performance_prototype_method_value, +}; use prototypes::{is_perf_constructor_name, link_perf_prototype}; const ENTRY_TYPE_MARK: u8 = 0; @@ -262,6 +265,17 @@ pub(crate) fn is_performance_object_value(value: f64) -> bool { false } +/// True only for the canonical `performance` singleton. The broader +/// `is_performance_object_value` predicate also accepts legacy perf-hooks +/// namespace objects for `instanceof` compatibility, but Performance +/// prototype methods require the object's actual internal brand. +pub(crate) fn is_performance_namespace_value(value: f64) -> bool { + PERFORMANCE_NS.with(|c| { + let cached = c.get(); + cached != 0 && cached == value.to_bits() + }) +} + pub(crate) fn is_perf_observer_list_value(value: f64) -> bool { unsafe { let Some(obj) = as_object_ptr(value) else { diff --git a/crates/perry-runtime/src/perf_hooks/prototypes.rs b/crates/perry-runtime/src/perf_hooks/prototypes.rs index 768eaaaec1..b10d16a729 100644 --- a/crates/perry-runtime/src/perf_hooks/prototypes.rs +++ b/crates/perry-runtime/src/perf_hooks/prototypes.rs @@ -1,5 +1,12 @@ use super::*; +mod performance_methods; +use performance_methods::{ + clear_marks, clear_measures, clear_resource_timings, event_loop_utilization, get_entries, + get_entries_by_name, get_entries_by_type, mark, mark_resource_timing, measure, now, + set_resource_timing_buffer_size, timerify, to_json, +}; + const PERF_CONSTRUCTOR_NAMES: &[&str] = &[ "Performance", "PerformanceEntry", @@ -216,6 +223,41 @@ fn perf_constructor_prototype(class_name: &str) -> f64 { crate::closure::closure_get_dynamic_prop(ptr, "prototype") } +/// Return the shared method installed on `Performance.prototype`. +/// +/// The `performance` object uses the same native-module tag as the top-level +/// `perf_hooks` namespace, whose generic bind path creates module-bound +/// closures. Route reads on the exact singleton back through its prototype so +/// extracted methods retain their receiver checks. +pub(crate) fn performance_prototype_method_value(name: &str) -> Option { + if !matches!( + name, + "clearMarks" + | "clearMeasures" + | "clearResourceTimings" + | "getEntries" + | "getEntriesByName" + | "getEntriesByType" + | "mark" + | "measure" + | "now" + | "setResourceTimingBufferSize" + | "toJSON" + | "eventLoopUtilization" + | "markResourceTiming" + | "timerify" + ) { + return None; + } + let scope = crate::gc::RuntimeHandleScope::new(); + let proto = scope.root_nanbox_f64(perf_constructor_prototype("Performance")); + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + let obj = + JSValue::from_bits(proto.get_nanbox_u64()).as_pointer::(); + let value = js_object_get_field_by_name(obj, key); + (value.bits() != crate::value::TAG_UNDEFINED).then(|| f64::from_bits(value.bits())) +} + /// Link runtime-created perf objects through their built-in class hierarchy. /// /// This is class-default wiring, not a user `Object.setPrototypeOf` override. @@ -261,25 +303,59 @@ pub(crate) unsafe fn attach_perf_hooks_constructor( match class_name { "Performance" => { - for method in [ - "clearMarks", - "clearMeasures", - "clearResourceTimings", - "getEntries", - "getEntriesByName", - "getEntriesByType", - "mark", - "measure", - "now", - "setResourceTimingBufferSize", - "toJSON", - ] { - let value = crate::object::bound_native_callable_export_value("perf_hooks", method); - install_perf_method(proto, method, value, true); - } - for method in ["eventLoopUtilization", "markResourceTiming", "timerify"] { - let value = crate::object::bound_native_callable_export_value("perf_hooks", method); - install_perf_method(proto, method, value, false); + let methods = [ + ("clearMarks", clear_marks as *const u8, 0, true), + ("clearMeasures", clear_measures as *const u8, 0, true), + ( + "clearResourceTimings", + clear_resource_timings as *const u8, + 0, + true, + ), + ("getEntries", get_entries as *const u8, 0, true), + ( + "getEntriesByName", + get_entries_by_name as *const u8, + 1, + true, + ), + ( + "getEntriesByType", + get_entries_by_type as *const u8, + 1, + true, + ), + ("mark", mark as *const u8, 1, true), + ("measure", measure as *const u8, 1, true), + ("now", now as *const u8, 0, true), + ( + "setResourceTimingBufferSize", + set_resource_timing_buffer_size as *const u8, + 1, + true, + ), + ("toJSON", to_json as *const u8, 0, true), + ( + "eventLoopUtilization", + event_loop_utilization as *const u8, + 2, + false, + ), + ( + "markResourceTiming", + mark_resource_timing as *const u8, + 7, + false, + ), + ("timerify", timerify as *const u8, 1, false), + ]; + for (method, thunk, arity, enumerable) in methods { + install_perf_method( + proto, + method, + perf_method_value(thunk, method, arity), + enumerable, + ); } let getter = perf_method_value( perf_time_origin_getter_thunk as *const u8, diff --git a/crates/perry-runtime/src/perf_hooks/prototypes/performance_methods.rs b/crates/perry-runtime/src/perf_hooks/prototypes/performance_methods.rs new file mode 100644 index 0000000000..a1d6ad18a9 --- /dev/null +++ b/crates/perry-runtime/src/perf_hooks/prototypes/performance_methods.rs @@ -0,0 +1,133 @@ +//! Receiver-aware `Performance.prototype` method thunks. + +use super::*; + +fn require_performance_receiver() { + if !is_performance_namespace_value(crate::object::js_implicit_this_get()) { + invalid_perf_receiver("Performance"); + } +} + +pub(super) extern "C" fn clear_marks( + _closure: *const crate::closure::ClosureHeader, + name: f64, +) -> f64 { + require_performance_receiver(); + js_perf_clear_marks(name) +} + +pub(super) extern "C" fn clear_measures( + _closure: *const crate::closure::ClosureHeader, + name: f64, +) -> f64 { + require_performance_receiver(); + js_perf_clear_measures(name) +} + +pub(super) extern "C" fn clear_resource_timings( + _closure: *const crate::closure::ClosureHeader, +) -> f64 { + require_performance_receiver(); + js_perf_clear_resource_timings() +} + +pub(super) extern "C" fn get_entries(_closure: *const crate::closure::ClosureHeader) -> f64 { + require_performance_receiver(); + js_perf_get_entries() +} + +pub(super) extern "C" fn get_entries_by_name( + _closure: *const crate::closure::ClosureHeader, + name: f64, + entry_type: f64, +) -> f64 { + require_performance_receiver(); + js_perf_get_entries_by_name(name, entry_type) +} + +pub(super) extern "C" fn get_entries_by_type( + _closure: *const crate::closure::ClosureHeader, + entry_type: f64, +) -> f64 { + require_performance_receiver(); + js_perf_get_entries_by_type(entry_type) +} + +pub(super) extern "C" fn mark( + _closure: *const crate::closure::ClosureHeader, + name: f64, + options: f64, +) -> f64 { + require_performance_receiver(); + js_perf_mark(name, options) +} + +pub(super) extern "C" fn measure( + _closure: *const crate::closure::ClosureHeader, + name: f64, + start_or_options: f64, + end: f64, +) -> f64 { + require_performance_receiver(); + js_perf_measure(name, start_or_options, end) +} + +pub(super) extern "C" fn now(_closure: *const crate::closure::ClosureHeader) -> f64 { + require_performance_receiver(); + crate::date::js_performance_now() +} + +pub(super) extern "C" fn set_resource_timing_buffer_size( + _closure: *const crate::closure::ClosureHeader, + size: f64, +) -> f64 { + require_performance_receiver(); + js_perf_set_resource_timing_buffer_size(size) +} + +pub(super) extern "C" fn to_json(_closure: *const crate::closure::ClosureHeader) -> f64 { + require_performance_receiver(); + js_perf_to_json() +} + +pub(super) extern "C" fn event_loop_utilization( + _closure: *const crate::closure::ClosureHeader, + utilization1: f64, + utilization2: f64, +) -> f64 { + require_performance_receiver(); + js_perf_event_loop_utilization(utilization1, utilization2) +} + +pub(super) extern "C" fn mark_resource_timing( + _closure: *const crate::closure::ClosureHeader, + timing_info: f64, + requested_url: f64, + initiator_type: f64, + global: f64, + cache_mode: f64, + body_info: f64, + response_status: f64, + delivery_type: f64, +) -> f64 { + require_performance_receiver(); + js_perf_mark_resource_timing( + timing_info, + requested_url, + initiator_type, + global, + cache_mode, + body_info, + response_status, + delivery_type, + ) +} + +pub(super) extern "C" fn timerify( + _closure: *const crate::closure::ClosureHeader, + function: f64, + options: f64, +) -> f64 { + require_performance_receiver(); + js_perf_timerify(function, options) +} diff --git a/test-parity/node-suite/perf_hooks/shapes/receiver-branding.ts b/test-parity/node-suite/perf_hooks/shapes/receiver-branding.ts index 49a9c984e9..e5986fdf30 100644 --- a/test-parity/node-suite/perf_hooks/shapes/receiver-branding.ts +++ b/test-parity/node-suite/perf_hooks/shapes/receiver-branding.ts @@ -19,6 +19,14 @@ outcome( "performance.mark", () => Reflect.apply(Object.getPrototypeOf(performance).mark, {}, ["x"]), ); +outcome( + "performance.now direct", + () => Reflect.apply(performance.now, {}, []), +); +outcome( + "performance.clearMarks direct", + () => Reflect.apply(performance.clearMarks, {}, []), +); outcome( "entry.toJSON", () => Reflect.apply(PerformanceEntry.prototype.toJSON, {}, []), From abdc0475f304445f45287d9b89ecfe02d7354bdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 17:22:59 +0200 Subject: [PATCH 2/7] chore: key changeset to PR 9894 (cherry picked from commit 77ddaee2b7be782be9b238d5e32fa050a4ae2864) --- ...202-perf-hooks-validation.md => 9894-perf-hooks-validation.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{9202-perf-hooks-validation.md => 9894-perf-hooks-validation.md} (100%) diff --git a/changelog.d/9202-perf-hooks-validation.md b/changelog.d/9894-perf-hooks-validation.md similarity index 100% rename from changelog.d/9202-perf-hooks-validation.md rename to changelog.d/9894-perf-hooks-validation.md From 9d2b5f639d0db9157cc0af40713d5cd790abcd3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 17:45:14 +0200 Subject: [PATCH 3/7] fix(codegen): publish constructor fallback artifacts (cherry picked from commit 2a79769120d25c38394b366b572c0ce1a9547168) --- .../9890-constructor-lowering-artifacts.md | 2 + crates/perry-codegen/src/codegen/method.rs | 124 ++++++++++++------ 2 files changed, 87 insertions(+), 39 deletions(-) create mode 100644 changelog.d/9890-constructor-lowering-artifacts.md diff --git a/changelog.d/9890-constructor-lowering-artifacts.md b/changelog.d/9890-constructor-lowering-artifacts.md new file mode 100644 index 0000000000..7b1164f9cb --- /dev/null +++ b/changelog.d/9890-constructor-lowering-artifacts.md @@ -0,0 +1,2 @@ +Preserve declarations, inline-cache globals, raw globals, and module counters +when constructor lowering exits through its no-callable-parent fallback. diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index ff861e76f8..4c1a1f061e 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -33,6 +33,48 @@ pub(super) use typed::{ compile_typed_i32_method, compile_typed_string_method, }; +struct LoweredFnArtifacts { + ic_globals: Vec, + typed_parse_rodata: Vec, + ic_end: u32, + pending_declares: Vec<(String, LlvmType, Vec)>, + buffer_alias_used: u32, + native_rep_records: Vec, +} + +/// Detach everything a function body accumulated before releasing its borrow +/// of the module-owned LLVM function. +fn take_lowered_fn_artifacts(ctx: &mut FnCtx<'_>) -> LoweredFnArtifacts { + LoweredFnArtifacts { + ic_globals: std::mem::take(&mut ctx.ic_globals), + typed_parse_rodata: std::mem::take(&mut ctx.typed_parse_rodata), + ic_end: ctx.ic_site_counter, + pending_declares: std::mem::take(&mut ctx.pending_declares), + buffer_alias_used: ctx.buffer_data_slots.len() as u32, + native_rep_records: std::mem::take(&mut ctx.native_rep_records), + } +} + +/// Publish the module-level artifacts emitted while lowering one function. +/// Every exit after body lowering must go through this path: the function IR +/// already references these names even when constructor setup bails out early. +fn publish_lowered_fn_artifacts(llmod: &mut LlModule, artifacts: LoweredFnArtifacts) { + llmod.ic_counter = artifacts.ic_end; + llmod.buffer_alias_counter += artifacts.buffer_alias_used; + llmod + .native_rep_records + .extend(artifacts.native_rep_records); + for (name, ret, params) in artifacts.pending_declares { + llmod.declare_function(&name, ret, ¶ms); + } + for ic_name in artifacts.ic_globals { + llmod.add_raw_global(crate::expr::inline_cache_global_definition(&ic_name)); + } + for raw in artifacts.typed_parse_rodata { + llmod.add_raw_global(raw); + } +} + /// Compile a class instance method as a top-level LLVM function with the /// signature `perry_method__(this_box: double, args: double…) /// -> double`. The first parameter (`this`) is stored in a slot whose @@ -924,9 +966,9 @@ pub(super) fn compile_method( )); ctx.block().ret(DOUBLE, &undef); } - let _ = std::mem::take(&mut ctx.ic_globals); - let _ = std::mem::take(&mut ctx.typed_parse_rodata); - let _ = std::mem::take(&mut ctx.pending_declares); + let artifacts = take_lowered_fn_artifacts(&mut ctx); + drop(ctx); + publish_lowered_fn_artifacts(llmod, artifacts); return Ok(()); } } else if let Some(ctor) = ctx.imported_class_ctors.get(&pname_owned).cloned() { @@ -1303,12 +1345,7 @@ pub(super) fn compile_method( ctx.block().ret(DOUBLE, &return_value); } } - let ic_globals = std::mem::take(&mut ctx.ic_globals); - let typed_parse_rodata = std::mem::take(&mut ctx.typed_parse_rodata); - let ic_end = ctx.ic_site_counter; - let pending = std::mem::take(&mut ctx.pending_declares); - let buffer_alias_used = ctx.buffer_data_slots.len() as u32; - let native_rep_records = std::mem::take(&mut ctx.native_rep_records); + let artifacts = take_lowered_fn_artifacts(&mut ctx); drop(ctx); // Under native roots, ordinary `force_inline` is intentionally only an @@ -1329,18 +1366,7 @@ pub(super) fn compile_method( lowered.pre_statepoint_inline = true; } } - llmod.ic_counter = ic_end; - llmod.buffer_alias_counter += buffer_alias_used; - llmod.native_rep_records.extend(native_rep_records); - for (name, ret, params) in pending { - llmod.declare_function(&name, ret, ¶ms); - } - for ic_name in &ic_globals { - llmod.add_raw_global(crate::expr::inline_cache_global_definition(ic_name)); - } - for raw in &typed_parse_rodata { - llmod.add_raw_global(raw.clone()); - } + publish_lowered_fn_artifacts(llmod, artifacts); // The Phase 5a and nonnegative-index clones are purely additive: the // public symbol (and its trampoline/forwarder, if any) belongs to the // primary invocation. Emitting it again here would define that symbol @@ -1852,24 +1878,44 @@ pub(super) fn compile_static_method( ctx.block().ret(DOUBLE, &undef); } } - let ic_globals = std::mem::take(&mut ctx.ic_globals); - let typed_parse_rodata = std::mem::take(&mut ctx.typed_parse_rodata); - let ic_end = ctx.ic_site_counter; - let pending = std::mem::take(&mut ctx.pending_declares); - let buffer_alias_used = ctx.buffer_data_slots.len() as u32; - let native_rep_records = std::mem::take(&mut ctx.native_rep_records); + let artifacts = take_lowered_fn_artifacts(&mut ctx); drop(ctx); - llmod.ic_counter = ic_end; - llmod.buffer_alias_counter += buffer_alias_used; - llmod.native_rep_records.extend(native_rep_records); - for (name, ret, params) in pending { - llmod.declare_function(&name, ret, ¶ms); - } - for ic_name in &ic_globals { - llmod.add_raw_global(crate::expr::inline_cache_global_definition(ic_name)); - } - for raw in &typed_parse_rodata { - llmod.add_raw_global(raw.clone()); - } + publish_lowered_fn_artifacts(llmod, artifacts); Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn lowered_function_artifacts_are_published_as_one_unit() { + let mut llmod = LlModule::new(crate::codegen::default_target_triple()); + llmod.ic_counter = 3; + llmod.buffer_alias_counter = 7; + + publish_lowered_fn_artifacts( + &mut llmod, + LoweredFnArtifacts { + ic_globals: vec!["perry_ic_9890".to_string()], + typed_parse_rodata: vec![ + "@issue_9890_rodata = private constant i64 9890".to_string() + ], + ic_end: 11, + pending_declares: vec![("js_issue_9890".to_string(), DOUBLE, vec![I64])], + buffer_alias_used: 2, + native_rep_records: Vec::new(), + }, + ); + + assert_eq!(llmod.ic_counter, 11); + assert_eq!(llmod.buffer_alias_counter, 9); + let ir = llmod.to_ir(); + assert!(ir.contains("@perry_ic_9890 ="), "{ir}"); + assert!( + ir.contains("@issue_9890_rodata = private constant i64 9890"), + "{ir}" + ); + assert!(ir.contains("declare double @js_issue_9890(i64)"), "{ir}"); + } +} From 22863ef48bf9653dbc2fdd3c4fdde15d76a45126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 17:45:36 +0200 Subject: [PATCH 4/7] chore: key changeset to PR 9896 (cherry picked from commit 49a4f99c01565d840c682f44f643a45960702c65) --- ...wering-artifacts.md => 9896-constructor-lowering-artifacts.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog.d/{9890-constructor-lowering-artifacts.md => 9896-constructor-lowering-artifacts.md} (100%) diff --git a/changelog.d/9890-constructor-lowering-artifacts.md b/changelog.d/9896-constructor-lowering-artifacts.md similarity index 100% rename from changelog.d/9890-constructor-lowering-artifacts.md rename to changelog.d/9896-constructor-lowering-artifacts.md From c6d36eb2b790db4ee9b2f8c1afc30c1f244e7953 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 14:27:56 +0200 Subject: [PATCH 5/7] perf(gc): swap-remove for keys-array families, with a spill index IdList::remove was Vec::remove(pos), which shifts every element past the removed position. The removals that matter come from the dead-owner prune (prune_dead_owner_side_tables_post_trace -> remove_descriptor_indexed_under) against a `families` list -- the descriptor ids indexed under one keys-array address. Measured on the compiled claude-code TUI, single 3300-char replies, two hosts, fourteen draws: * the removals sit at position ~0.31 of the list -- essentially always the FRONT, which is the worst case for a tail shift; * one owned-keys array per process grows its family to 404k-514k while every other list stays small; * so the same ~3.7M removals memmove up to 848 GB in a single turn; * 100.000% of those bytes are in `families`. `by_facts` moves ZERO: its longest list is 1 in every draw, exactly as its doc claims. * retire_owned_shape_siblings is NOT involved -- it never sees a family longer than 16. THE ORDER QUESTION, because a swap-remove is only available if order is not load-bearing, and the answer differs per index: * `by_facts` IS ordered -- facts_push_front installs a process-global id as the canonical answer ahead of an equivalent local one, read first-wins. It keeps remove_ordered, and that costs nothing: length 1. * `families` is NOT. Its only order-touching reader is the "one descriptor stands for the family" choice in the two rekey walks, which breaks on the first carrier and otherwise takes any present member -- and the chosen descriptor feeds exactly one expression, old_carrier || cache_carrier, whose value is the same for every carrier and the same for every non-carrier. The outcome is a function of the SET. So removal comes in two flavours and THE CALLER DECLARES THE CONTRACT, because the caller is the one that knows whether its order matters; a single remove() that guessed would be the bug. A spilled list gains an id -> index map, built once it passes SPILL_INDEX_MIN (32) and empty below it, where a scan of a few entries is one cache line and a hash probe is not. The index makes position/contains/remove O(1) on the lists that get long, which also removes the linear membership scan in family_push_back -- recorded at this file's own append_unchecked as 6.2% of main-thread leaf samples, 95% of it under that one caller. Three tests. The guard asserts its bound as a MULTIPLE of N, so it is about the complexity class rather than about one N: 2,000 front removals may move at most 4N elements, where the ordered path moves N(N-1)/2 = 1,999,000. Sabotage: point remove_unordered at remove_ordered (250x the bound); raising SPILL_INDEX_MIN above N fails the scan half of the same test. The second checks the index against a plain Vec oracle after front, middle and back removals, because an index that drifts is a WRONG ANSWER -- a descriptor that cannot be found -- not a slow one; its sabotage is dropping the fixup for the element the swap relocated. The third pins that a short spilled list builds no index at all. [gc-idlist] under PERRY_GC_DIAG=1 reports removals, elements moved and positions scanned. NOT CLAIMED: that the memmove explains the bimodal turn CPU. On the pre-fix binary one draw moved 335 GB and was as fast as one that moved 16 GB, so bytes moved is necessary but not sufficient for the slow mode. What this removes is unambiguously wasted work. NOT ADDRESSED: why one family reaches half a million descriptors. That is a separate defect, still being measured, and will be a separate change. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp (cherry picked from commit 01313fc2e1861678218c7586529e2ef45ddfa357) --- .../9879-gc-family-list-swap-remove.md | 29 ++ crates/perry-runtime/src/gc/copying.rs | 1 + crates/perry-runtime/src/object/shapes.rs | 24 +- .../perry-runtime/src/object/shapes_store.rs | 466 ++++++++++++++++-- 4 files changed, 466 insertions(+), 54 deletions(-) create mode 100644 changelog.d/9879-gc-family-list-swap-remove.md diff --git a/changelog.d/9879-gc-family-list-swap-remove.md b/changelog.d/9879-gc-family-list-swap-remove.md new file mode 100644 index 0000000000..d27b0ef599 --- /dev/null +++ b/changelog.d/9879-gc-family-list-swap-remove.md @@ -0,0 +1,29 @@ +### Fixed + +- **gc:** a keys-array family's descriptor list no longer memmoves its whole + tail on every removal, and no longer scans linearly to find an id. + + `IdList::remove` was `Vec::remove(pos)`, which shifts everything past the + removed position. Measured on the compiled claude-code TUI, one 3300-char + reply, ten draws across two hosts: the removals sit at position **~0.31** of + the list — i.e. essentially always the front — and the longest list reaches + **514,030** entries, so the same ~3.7 M removals memmove up to **848 GB** in + a single turn. The removals come from the dead-owner prune + (`prune_dead_owner_side_tables_post_trace`). + + No claim is made that this explains the turn's bimodal CPU: one draw moved + 335 GB and was as fast as one that moved 16 GB, so bytes moved is necessary + but not sufficient for the slow mode. What is removed here is unambiguously + wasted work; how much time that is worth is for the A/B to say. + + A spilled list now carries an `id -> index` map, built once it passes 32 + entries, and `families` removes through a swap-remove that moves one element + regardless of position. `by_facts` keeps the order-preserving removal it + needs (its first entry is the canonical answer for exact-facts interning) and + is unaffected — measured at max length **1**, so it never builds an index. + + The same index also removes the linear membership scan in + `family_push_back`, previously **6.2 %** of main-thread leaf samples. + + This does not address why one family reaches half a million descriptors, + which is a separate defect and a separate change. diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index d0b25515f6..2eb134d64f 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -1899,6 +1899,7 @@ pub(super) fn run_copied_minor_attempt( } crate::arena::alloc_sample::report("minor"); super::diag_sites::report_primitive_dispatch("minor"); + crate::object::shapes::id_list_report(); report_forwarding_refusals("copying_minor"); super::scanner_profile::report_and_reset("copying_minor"); CopiedMinorAttempt::Done(Some(CopiedMinorFastPathOutcome { diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 3e0ebf1a01..b0d47aac53 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -264,6 +264,15 @@ struct ShapeTableInner { const SHAPE_YOUNG_LOG_NAME: &str = "shapes.families+indices"; +/// Re-export of the id-list operation counters' report, so the collector does +/// not have to name a private sibling module. One `[gc-idlist]` line per +/// copying minor under `PERRY_GC_DIAG=1`; `elems_moved` is the falsifier for +/// the swap-remove change. +#[inline] +pub(crate) fn id_list_report() { + shapes_store::id_list_report(); +} + impl ShapeTableInner { /// Rule 1 of `gc/young_log.rs`: log a keys address BEFORE a family or a /// slot index is published under it, when the keys array is not old. @@ -305,7 +314,14 @@ impl ShapeTableInner { let Some(ids) = self.families.get_mut(&keys) else { return false; }; - let removed = ids.remove(id); + // UNORDERED: a family's readers are set-valued (see `IdList`'s type + // doc), and the ordered removal was memmoving the whole tail of a list + // measured at up to 514,030 entries, from position ~0.31, 3.7 M times + // per 3300-char reply. The dominant caller is the dead-owner prune + // (`prune_dead_owner_side_tables_post_trace` -> + // `remove_descriptor_indexed_under`); `retire_owned_shape_siblings` + // never sees a family longer than 16. + let removed = ids.remove_unordered(id); if ids.is_empty() { self.families.remove(&keys); } @@ -334,7 +350,11 @@ impl ShapeTableInner { let Some(ids) = self.by_facts.get_mut(&facts) else { return false; }; - let removed = ids.remove(id); + // ORDERED, and it must stay ordered: `facts_push_front` is how an + // installed process-global id becomes the canonical answer ahead of an + // equivalent local one, and this list is read first-wins. Measured at + // max length 1 on cc, so the order costs nothing to keep. + let removed = ids.remove_ordered(id); if ids.is_empty() { self.by_facts.remove(&facts); } diff --git a/crates/perry-runtime/src/object/shapes_store.rs b/crates/perry-runtime/src/object/shapes_store.rs index d5cb498ce7..fdba57fe38 100644 --- a/crates/perry-runtime/src/object/shapes_store.rs +++ b/crates/perry-runtime/src/object/shapes_store.rs @@ -467,23 +467,225 @@ impl ShapeSlab { } } +/// MEASUREMENT that this structure is judged on, and the test's instrument. +/// +/// Two counters on the id-list mutation path, kept unconditionally because the +/// rig falsifier and the unit guard both read them and a `cfg(test)` counter +/// can only prove the test's own arithmetic. Deliberately a THREE-field struct +/// in one `Cell`: a thread-local `Cell` get/set copies `T` on every +/// operation, and this path runs millions of times per turn, so the width of +/// this type is itself a cost. +#[derive(Default, Clone, Copy)] +pub(crate) struct IdListOpStats { + /// Removals that found their id. + pub(crate) removals: u64, + /// Elements shifted by a removal. Bytes = this x 4. Swap-remove moves + /// none; `Vec::remove` moves the whole tail past the removed position. + pub(crate) elems_moved: u64, + /// Entries touched by a linear membership or position scan. The other + /// half of the same defect: the index removes this too. + pub(crate) positions_scanned: u64, +} + +thread_local! { + pub(crate) static ID_LIST_OP_STATS: std::cell::Cell = + const { + std::cell::Cell::new(IdListOpStats { + removals: 0, + elems_moved: 0, + positions_scanned: 0, + }) + }; +} + +#[inline] +fn note_scan(entries: usize) { + ID_LIST_OP_STATS.with(|c| { + let mut st = c.get(); + st.positions_scanned += entries as u64; + c.set(st); + }); +} + +#[inline] +fn note_removal(elems_moved: usize) { + ID_LIST_OP_STATS.with(|c| { + let mut st = c.get(); + st.removals += 1; + st.elems_moved += elems_moved as u64; + c.set(st); + }); +} + +/// One `[gc-idlist]` line per copying minor under `PERRY_GC_DIAG=1`, +/// cumulative. `elems_moved` is the rig falsifier for this change. +pub(crate) fn id_list_report() { + if !crate::gc::gc_diag_enabled() { + return; + } + let st = ID_LIST_OP_STATS.with(std::cell::Cell::get); + if st.removals == 0 { + return; + } + eprintln!( + "[gc-idlist] removals={} elems_moved={} bytes_moved={} positions_scanned={}", + st.removals, + st.elems_moved, + st.elems_moved * 4, + st.positions_scanned, + ); +} + +/// A spilled id list: the ids, plus an `id -> index` map built once the list +/// is large enough for a linear scan to cost more than a hash probe. +/// +/// The index is what makes `remove_unordered`, `contains` and `position` O(1) +/// on the lists that actually get long. Below [`SPILL_INDEX_MIN`] it stays +/// empty and every operation is the linear scan it always was, because for a +/// handful of entries the scan is a single cache line and the map is not. +#[derive(Clone, Debug, Default)] +pub(super) struct SpillList { + ids: Vec, + /// Empty while `ids.len() < SPILL_INDEX_MIN`; complete above it. + /// + /// `PtrHasher` (#8125) is the right hasher here for the same reason it is + /// on the maps around it: shape ids come from a monotonic counter, so the + /// key is a small dense integer and the avalanche step is what keeps every + /// one of them off bucket 0. + pos: crate::fast_hash::PtrHashMap, +} + +/// Where the index starts paying. Measured shape of the problem: `families` +/// lists reach 514,030 entries on a claude-code reply while `by_facts` lists +/// are length 1, so anything in the low tens is far below the case that hurts +/// and far above the case where the map would be pure overhead. +const SPILL_INDEX_MIN: usize = 32; + +impl SpillList { + #[inline] + fn indexed(&self) -> bool { + !self.pos.is_empty() + } + + /// Build the index if the list has just crossed the threshold. Called + /// after every growth, so the map exists from the first entry past it. + #[inline] + fn maybe_build_index(&mut self) { + if self.pos.is_empty() && self.ids.len() >= SPILL_INDEX_MIN { + self.pos.reserve(self.ids.len()); + for (i, &id) in self.ids.iter().enumerate() { + self.pos.insert(id, i as u32); + } + } + } + + /// Position of `id`, O(1) when indexed and a counted linear scan below the + /// threshold. + #[inline] + fn position(&self, id: u32) -> Option { + if self.indexed() { + return self.pos.get(&id).map(|&i| i as usize); + } + note_scan(self.ids.len()); + self.ids.iter().position(|&x| x == id) + } + + #[inline] + fn push(&mut self, id: u32) { + let i = self.ids.len(); + self.ids.push(id); + if self.indexed() { + self.pos.insert(id, i as u32); + } else { + self.maybe_build_index(); + } + } + + /// ORDER-PRESERVING removal, for a list whose order is load-bearing. + /// O(n) in the tail by construction — that is what "preserve the order" + /// costs — and it reindexes the shifted suffix. + fn remove_ordered(&mut self, id: u32) -> Option { + let pos = self.position(id)?; + let moved = self.ids.len() - 1 - pos; + note_removal(moved); + self.ids.remove(pos); + if self.indexed() { + self.pos.remove(&id); + for (i, &other) in self.ids.iter().enumerate().skip(pos) { + self.pos.insert(other, i as u32); + } + } + Some(pos) + } + + /// UNORDERED removal: the last element takes the removed one's slot. + /// Moves ONE element regardless of position, which is the whole point — + /// the measured removals sit at position ~0.31 of a list up to 514,030 + /// long, so `Vec::remove` was shifting essentially the entire list every + /// time. + /// + /// What this does NOT claim: that the memmove explains the bimodal turn + /// CPU. On perrymaster one draw moved 335 GB and was as fast as a draw + /// that moved 16 GB, so bytes moved is necessary but not sufficient for + /// the slow mode. This removes work that is unambiguously wasted; how much + /// TIME it removes is the A/B's to say. + fn remove_unordered(&mut self, id: u32) -> Option { + let pos = self.position(id)?; + note_removal(if pos + 1 == self.ids.len() { 0 } else { 1 }); + let last = self.ids.len() - 1; + self.ids.swap_remove(pos); + if self.indexed() { + self.pos.remove(&id); + if pos != last { + // The element that was last now lives at `pos`. + self.pos.insert(self.ids[pos], pos as u32); + } + } + Some(pos) + } + + #[inline] + fn replace(&mut self, old: u32, new: u32) -> bool { + let Some(pos) = self.position(old) else { + return false; + }; + self.ids[pos] = new; + if self.indexed() { + self.pos.remove(&old); + self.pos.insert(new, pos as u32); + } + true + } +} + /// A compact list of descriptor ids: up to three inline, then a spilled -/// `Vec`. Sized so a family-index bucket is `(u64, IdList)` = 24 bytes. +/// `Vec` with an `id -> index` map (see [`SpillList`]). Sized so a family-index +/// bucket is `(u64, IdList)` = 24 bytes. +/// +/// # Order +/// Order is meaningful **for `by_facts` only**: [`IdList::push_front`] is how +/// an installed process-global id becomes the canonical answer for exact-facts +/// interning ahead of an equivalent local id (`install_external_shape_id`), and +/// that list is read first-wins. `families` is NOT order-sensitive: its only +/// order-touching reader is the "one descriptor stands for the family" choice +/// in the two rekey walks, which breaks on the first carrier and otherwise +/// takes any present member — and the chosen descriptor feeds exactly one +/// expression, `old_carrier || cache_carrier`, whose value is the same for +/// every carrier and the same for every non-carrier. The outcome is a function +/// of the SET, not of the order. /// -/// Order is meaningful: [`IdList::push_front`] is how an installed -/// process-global id becomes the canonical answer for exact-facts interning -/// ahead of an equivalent local id (`install_external_shape_id`). +/// That asymmetry is why removal comes in two flavours: +/// [`IdList::remove_ordered`] for `by_facts` and [`IdList::remove_unordered`] +/// for `families`. **The caller declares the contract**, because the caller is +/// the one that knows whether its order is load-bearing; a single `remove` that +/// guessed would be the bug. #[derive(Clone, Debug)] pub(super) enum IdList { - Inline { - len: u8, - ids: [u32; 3], - }, - // The `Box` is the point: an inline `Vec` is 24 bytes and would make every - // bucket 32; the spill is the rare case, so its extra indirection is - // cheaper than eight bytes on every family. - #[allow(clippy::box_collection)] - Spill(Box>), + Inline { len: u8, ids: [u32; 3] }, + // The `Box` is the point: an inline `SpillList` is far wider and would make + // every bucket pay for it; the spill is the rare case, so its extra + // indirection is cheaper than those bytes on every family. + Spill(Box), } const _: () = assert!(std::mem::size_of::() == 16); @@ -502,7 +704,7 @@ impl IdList { pub(super) fn as_slice(&self) -> &[u32] { match self { IdList::Inline { len, ids } => &ids[..*len as usize], - IdList::Spill(v) => v.as_slice(), + IdList::Spill(v) => v.ids.as_slice(), } } @@ -518,12 +720,18 @@ impl IdList { #[inline] pub(super) fn contains(&self, id: u32) -> bool { - self.as_slice().contains(&id) + match self { + IdList::Inline { len, ids } => ids[..*len as usize].contains(&id), + IdList::Spill(v) => v.position(id).is_some(), + } } - fn spill(&mut self) -> &mut Vec { + fn spill(&mut self) -> &mut SpillList { if let IdList::Inline { len, ids } = self { - let v = ids[..*len as usize].to_vec(); + let v = SpillList { + ids: ids[..*len as usize].to_vec(), + pos: crate::fast_hash::new_ptr_hash_map(), + }; *self = IdList::Spill(Box::new(v)); } match self { @@ -554,6 +762,11 @@ impl IdList { /// main-thread leaf samples on a claude-code streamed reply, 95 % of it /// under `ShapeTableInner::family_push_back`. /// + /// The spill index now removes that scan for the callers that cannot use + /// this entry point, which is why `contains` is O(1) above + /// [`SPILL_INDEX_MIN`]. This function stays because skipping the probe + /// entirely is still cheaper than performing it. + /// /// Callers that re-file an EXISTING id (the metadata rekey when a keys /// array moves) must keep using [`push_back`]: those ids can already be in /// the destination list. @@ -567,7 +780,9 @@ impl IdList { } } - /// Prepend `id` unless already present. + /// Prepend `id` unless already present. Order-preserving by definition, so + /// it stays O(n) on a spilled list; only `by_facts` and the external-id + /// install use it, and neither is on a hot path. pub(super) fn push_front(&mut self, id: u32) { if self.contains(id) { return; @@ -578,39 +793,62 @@ impl IdList { ids[0] = id; *len += 1; } - _ => self.spill().insert(0, id), + _ => { + let v = self.spill(); + v.ids.insert(0, id); + if v.indexed() { + v.pos.clear(); + } + v.maybe_build_index(); + } } } - /// Drop `id` if present; returns whether it was. - pub(super) fn remove(&mut self, id: u32) -> bool { + /// Drop `id` if present, PRESERVING the order of what remains; returns + /// whether it was there. For a list whose order is load-bearing — + /// `by_facts`, where the first entry is the canonical answer. + pub(super) fn remove_ordered(&mut self, id: u32) -> bool { match self { - IdList::Inline { len, ids } => { - let n = *len as usize; - let Some(pos) = ids[..n].iter().position(|&x| x == id) else { - return false; - }; - ids.copy_within(pos + 1..n, pos); - ids[n - 1] = 0; - *len -= 1; - true - } - IdList::Spill(v) => { - let Some(pos) = v.iter().position(|&x| x == id) else { - return false; - }; - v.remove(pos); - true - } + IdList::Inline { len, ids } => Self::remove_inline(len, ids, id), + IdList::Spill(v) => v.remove_ordered(id).is_some(), + } + } + + /// Drop `id` if present, WITHOUT preserving order; returns whether it was + /// there. For `families`, whose readers are set-valued (see the type doc). + /// + /// This is the change: on a spilled list it moves ONE element instead of + /// the whole tail. + pub(super) fn remove_unordered(&mut self, id: u32) -> bool { + match self { + // Three entries: the inline shift is a single register move and + // there is nothing to gain from disturbing the order. + IdList::Inline { len, ids } => Self::remove_inline(len, ids, id), + IdList::Spill(v) => v.remove_unordered(id).is_some(), } } + #[inline] + fn remove_inline(len: &mut u8, ids: &mut [u32; 3], id: u32) -> bool { + let n = *len as usize; + note_scan(n); + let Some(pos) = ids[..n].iter().position(|&x| x == id) else { + return false; + }; + note_removal(n - 1 - pos); + ids.copy_within(pos + 1..n, pos); + ids[n - 1] = 0; + *len -= 1; + true + } + /// Replace `old` with `new` in place (keeps its position); returns /// whether `old` was present. pub(super) fn replace(&mut self, old: u32, new: u32) -> bool { match self { IdList::Inline { len, ids } => { let n = *len as usize; + note_scan(n); match ids[..n].iter().position(|&x| x == old) { Some(pos) => { ids[pos] = new; @@ -619,21 +857,20 @@ impl IdList { None => false, } } - IdList::Spill(v) => match v.iter().position(|&x| x == old) { - Some(pos) => { - v[pos] = new; - true - } - None => false, - }, + IdList::Spill(v) => v.replace(old, new), } } - /// Bytes held outside the containing bucket. pub(super) fn heap_bytes(&self) -> usize { match self { IdList::Inline { .. } => 0, - IdList::Spill(v) => std::mem::size_of::>() + v.capacity() * 4, + IdList::Spill(v) => { + std::mem::size_of::() + + v.ids.capacity() * 4 + // The index is the structure's memory cost and is reported + // rather than hidden: it exists only above SPILL_INDEX_MIN. + + v.pos.capacity() * (std::mem::size_of::<(u32, u32)>() + 1) + } } } } @@ -782,8 +1019,10 @@ mod tests { assert_eq!(list.as_slice(), &[1, 2, 3, 4]); list.push_front(0); assert_eq!(list.as_slice(), &[0, 1, 2, 3, 4]); - assert!(list.remove(2)); - assert!(!list.remove(2)); + // The ORDERED removal keeps this list's order, which is what + // `by_facts` depends on. + assert!(list.remove_ordered(2)); + assert!(!list.remove_ordered(2)); assert_eq!(list.as_slice(), &[0, 1, 3, 4]); assert!(list.replace(3, 30)); assert!(!list.replace(3, 300)); @@ -794,13 +1033,136 @@ mod tests { inline.push_back(7); inline.push_back(8); inline.push_back(9); - assert!(inline.remove(8)); + assert!(inline.remove_ordered(8)); assert_eq!(inline.as_slice(), &[7, 9]); assert!(inline.replace(9, 10)); assert_eq!(inline.as_slice(), &[7, 10]); - assert!(inline.remove(7)); - assert!(inline.remove(10)); + assert!(inline.remove_ordered(7)); + assert!(inline.remove_ordered(10)); assert!(inline.is_empty()); assert_eq!(inline.heap_bytes(), 0); } + + /// THE GUARD for this change, and it is an asymmetric one: the unordered + /// removal must move O(1) elements per call, and the ordered one is + /// allowed to move O(n) because that is what preserving the order costs. + /// + /// Front removal is the measured shape of the defect — removals sit at + /// position ~0.31 of a list up to 514,030 long — so the test removes from + /// the front, which is the worst case for `Vec::remove` and the best case + /// for nothing. + /// + /// **Sabotage: point `remove_unordered` at `remove_ordered`.** The bound + /// below is `4 * N`; the O(n) path moves `N * (N - 1) / 2` = 1,999,000 + /// elements for N = 2,000, i.e. 250x the bound, and this fails. A bound + /// expressed as a MULTIPLE of N rather than an absolute is what makes the + /// assertion about the complexity class instead of about one N. + #[test] + fn unordered_removal_moves_o1_elements_and_scans_o1_entries() { + const N: u32 = 2_000; + + let baseline = ID_LIST_OP_STATS.with(std::cell::Cell::get); + let mut list = IdList::default(); + for id in 1..=N { + // The interning sites' entry point: no membership probe. + list.append_unchecked(id); + } + assert_eq!(list.len(), N as usize); + assert!(matches!(list, IdList::Spill(_))); + + // Remove every id from the FRONT of the list, in insertion order. + for id in 1..=N { + assert!(list.remove_unordered(id), "id {id} was not present"); + } + assert!(list.is_empty()); + + let after = ID_LIST_OP_STATS.with(std::cell::Cell::get); + let moved = after.elems_moved - baseline.elems_moved; + let scanned = after.positions_scanned - baseline.positions_scanned; + let removals = after.removals - baseline.removals; + assert_eq!(removals, u64::from(N)); + + // O(1) per removal, with room for the swap itself. + assert!( + moved <= 4 * u64::from(N), + "unordered removal moved {moved} elements for {N} removals — that \ + is the O(n) tail shift this structure exists to remove \ + (the ordered path would move {})", + u64::from(N) * (u64::from(N) - 1) / 2 + ); + // The index answers `position`, so no linear scan may be charged for + // a list this long. Sabotage: raise SPILL_INDEX_MIN above N and this + // fails with ~N*N/2 scanned entries. + assert!( + scanned <= 4 * u64::from(N), + "unordered removal scanned {scanned} entries for {N} removals — \ + the spill index is not answering `position`" + ); + } + + /// The index must agree with the vector after every operation, including + /// the swap that moves a third element nobody named. Checked exhaustively + /// against a plain `Vec` oracle, because an index that drifts is a wrong + /// ANSWER (a descriptor that cannot be found, or one found under the wrong + /// id), not a slow one. + /// + /// Sabotage: drop the `self.pos.insert(self.ids[pos], pos as u32)` fixup + /// in `remove_unordered` — the element the swap relocated keeps a stale + /// index and the `contains` check below fails. + #[test] + fn the_spill_index_agrees_with_the_vector_after_every_operation() { + let mut list = IdList::default(); + let mut oracle: Vec = Vec::new(); + for id in 1..=200u32 { + list.append_unchecked(id); + oracle.push(id); + } + // Remove a scattered third of them, front, middle and back. + for &id in &[1u32, 2, 3, 100, 101, 199, 200, 50, 150, 7] { + assert!(list.remove_unordered(id)); + oracle.retain(|&x| x != id); + } + // Same SET, whatever the order. + let mut got = list.as_slice().to_vec(); + got.sort_unstable(); + let mut want = oracle.clone(); + want.sort_unstable(); + assert_eq!(got, want); + // And every survivor is still findable through the index. + for &id in &want { + assert!(list.contains(id), "id {id} lost its index entry"); + } + for &id in &[1u32, 2, 3, 100, 101, 199, 200, 50, 150, 7] { + assert!(!list.contains(id), "removed id {id} is still findable"); + } + // `replace` must keep the index coherent too. + let survivor = want[0]; + assert!(list.replace(survivor, 9_999)); + assert!(!list.contains(survivor)); + assert!(list.contains(9_999)); + } + + /// A list that never reaches `SPILL_INDEX_MIN` must not allocate an index + /// — the map is the structure's memory cost and it is only worth paying + /// where the scan hurts. `by_facts` lists, measured at length 1 on cc, + /// live entirely in this regime. + #[test] + fn a_short_spilled_list_builds_no_index() { + let mut list = IdList::default(); + for id in 1..=8u32 { + list.append_unchecked(id); + } + assert!(matches!(list, IdList::Spill(_))); + match &list { + IdList::Spill(v) => assert!( + !v.indexed(), + "a list of 8 built an index; SPILL_INDEX_MIN is {SPILL_INDEX_MIN}" + ), + IdList::Inline { .. } => unreachable!(), + } + // Still correct without one. + assert!(list.remove_unordered(4)); + assert!(!list.contains(4)); + assert!(list.contains(8)); + } } From 5fc3143d1a19bf043194c50732839adc67deac19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 16:05:43 +0200 Subject: [PATCH 6/7] test(gc): the retirement test asserts membership, not family order perrymaster's run of the PR tree: 3192 tests, 1 failed -- owned_key_count_versions_are_retired_behind_the_current_one, at the post-retirement assert_eq!(test_shape_ids_for_keys(keys), vec![cached, current]); swap-removing stale_a/stale_b moved `current` in front of `cached`. THE TEST OVER-SPECIFIES, and the evidence is an enumeration of every production reader of `families` on this base, done with this test's subject in mind: shapes.rs:1408 retire_owned_shape_siblings filter all-but-keep SET shapes.rs:1906 prune_dead_shape_keys_young snapshot, remove all SET shapes.rs:1946 scan_shape_table_rekey_mut first-carrier-else-any SET shapes.rs:2067 move_shape_family wholesale remove/insert n/a shapes.rs:2092 relevant_shape_keys KEYS, sort+dedup normalised shapes.rs:2182 scan_shape_keys_address as 1946 SET shapes.rs:2337 census heap_bytes sum aggregate shapes.rs:2390 census len/max aggregate slot_list:402 retire all but `current` filter SET slot_list:597 ids.replace(old, new) position-PRESERVING n/a slot_list:686 retire all but `id` filter SET The two rekey walks are the only order-TOUCHING readers, and what they take from the list is a single choice fed to exactly one expression, old_carrier || cache_carrier, whose value is the same for every carrier and the same for every non-carrier -- so the outcome is a function of the set. Nothing else reads a position. And the helper the assertion uses, test_shape_ids_for_keys, is #[cfg(test)]: it renders families.as_slice().to_vec() for tests only. Its .first() sibling, test_shape_id_for_keys, is also #[cfg(test)] and is only ever called on families the caller has seeded with one descriptor. #9706's contract is about WHICH ids survive a same-address retirement, not the order they survive in; the assertion compared against a Vec because the helper returns one. So the post-retirement assertion becomes a sorted comparison, with the contract and the reader enumeration written at it. The pre-retirement assertion is left as-is -- no removal has happened there and it legitimately documents that adds append -- with a comment saying that is a property of the add path and not a contract. Corroborating: the suite ran 3192 with exactly ONE failure, so no other test in the tree asserts a family's order across a removal. (cherry picked from commit f9eb62c58ccc500a52132cba7419499a470839ca) --- .../perry-runtime/src/object/shapes_tests.rs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/crates/perry-runtime/src/object/shapes_tests.rs b/crates/perry-runtime/src/object/shapes_tests.rs index cd3a56feee..dd06428773 100644 --- a/crates/perry-runtime/src/object/shapes_tests.rs +++ b/crates/perry-runtime/src/object/shapes_tests.rs @@ -777,8 +777,9 @@ mod descriptor_tests_8067 { .expect("shape range unexpectedly exhausted"); let unrelated = shape_descriptor_ensure(unrelated_keys as *const ArrayHeader, 1, 1) .expect("shape range unexpectedly exhausted"); - // Before retirement every version is resolvable and the family lists - // them in mint order. + // Before retirement every version is resolvable. Adds append, so the + // family happens to be in mint order here; that is a property of the + // ADD path, not a contract (see the retirement assertion below). assert_eq!( test_shape_ids_for_keys(keys), vec![stale_a, stale_b, cached, current] @@ -795,7 +796,26 @@ mod descriptor_tests_8067 { ); assert!(shape_descriptor_by_id(current).is_some()); assert!(shape_descriptor_by_id(unrelated).is_some()); - assert_eq!(test_shape_ids_for_keys(keys), vec![cached, current]); + // MEMBERSHIP, not order. #9706's contract is "the growth history is + // retired behind the version its owner now carries, except one an + // optimization cache permanently owns" — a statement about WHICH ids + // survive. The order they survive in is not part of it, and no + // production reader of `families` depends on it: every one either + // filters the whole list, snapshots the whole list, aggregates it, or + // (the two rekey walks) picks "a carrier if the family has one, else + // any present member" and feeds that single choice to exactly one + // expression, `old_carrier || cache_carrier`, whose value is the same + // for every carrier and the same for every non-carrier. The two + // helpers this test uses are `#[cfg(test)]` renderings of the list. + // + // This assertion compared against a `Vec` because the helper returns + // one, which pinned mint order by accident; `families` now removes by + // swapping the last element into the hole, so a survivor can move. + let mut survivors = test_shape_ids_for_keys(keys); + survivors.sort_unstable(); + let mut expected = vec![cached, current]; + expected.sort_unstable(); + assert_eq!(survivors, expected); // Retired facts re-intern as FRESH ids: nothing can resolve the old ones. let reminted = shape_descriptor_ensure(keys as *const ArrayHeader, 1, 1) .expect("shape range unexpectedly exhausted"); From 490ccde014b390411e9916b11a4d96524d490dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 18:10:59 +0200 Subject: [PATCH 7/7] fix(train): classify and hot-route #9881's IdList op counters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ID_LIST_OP_STATS is three plain u64 counts in a Cell — no address, no NaN-boxed value — so it is not_a_gc_pointer for the holder inventory. It was also a raw thread_local!, and its increments are NOT diagnostic- gated: note_scan/note_removal run on every IdList scan and removal, which is the path #9881 measures at ~3.7M removals per turn. That is the hot case perry_thread_local! exists for, so it is converted rather than recorded as cold debt. --- crates/perry-runtime/src/object/shapes_store.rs | 2 +- scripts/gc_runtime_root_holders.json | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/perry-runtime/src/object/shapes_store.rs b/crates/perry-runtime/src/object/shapes_store.rs index fdba57fe38..37e72f4f84 100644 --- a/crates/perry-runtime/src/object/shapes_store.rs +++ b/crates/perry-runtime/src/object/shapes_store.rs @@ -487,7 +487,7 @@ pub(crate) struct IdListOpStats { pub(crate) positions_scanned: u64, } -thread_local! { +crate::perry_thread_local! { pub(crate) static ID_LIST_OP_STATS: std::cell::Cell = const { std::cell::Cell::new(IdListOpStats { diff --git a/scripts/gc_runtime_root_holders.json b/scripts/gc_runtime_root_holders.json index 1bbbb6b148..92d9286f61 100644 --- a/scripts/gc_runtime_root_holders.json +++ b/scripts/gc_runtime_root_holders.json @@ -613,6 +613,12 @@ "verdict": "not_a_gc_pointer", "why": "Megamorphic property-read stub cache, the read twin of WRITE_STUB: 2-way ways of (shape_token, key_bits, slot) plain u64s. The token is a shape id and the slot an index; key_bits are content-derived by construction, because read_stub_key_bits returns short_ascii_sso_bits(key) \u2014 the key's characters packed inline \u2014 and yields None for any key that would otherwise be stored under a pointer. No way holds a heap address, so nothing here keeps an object alive, and a stale entry cannot hit: receiver_shape_token returns None for a receiver with no live shape, and the token identifies the exact key set and order, so a shape change yields a different token." }, + { + "file": "crates/perry-runtime/src/object/shapes_store.rs", + "name": "ID_LIST_OP_STATS", + "verdict": "not_a_gc_pointer", + "why": "#9881: the IdList operation tally that measures the swap-remove win \u2014 `removals`, `elems_moved`, `positions_scanned`, three plain `u64` counts in a `Cell`. It holds NUMBERS, never an address or a NaN-boxed value: written only by the `IdList` remove/scan paths incrementing them and read only by the diagnostic that reports the memmove volume, so there is no slot for the collector to mark or rewrite." + }, { "file": "crates/perry-runtime/src/os/os_process_emitter.rs", "name": "PROCESS_EXIT_EVENT_EMITTED",