diff --git a/changelog.d/10613-box-cell-release.md b/changelog.d/10613-box-cell-release.md new file mode 100644 index 0000000000..55e6c61ee5 --- /dev/null +++ b/changelog.d/10613-box-cell-release.md @@ -0,0 +1,52 @@ +### Fixed: an ordinary frame now releases the box cells it minted (#10464) + +A `let`/`var` that a closure captures and something reassigns is stored in a +malloc-side **box cell**, and every registered cell is a strong GC root +(`scan_box_roots_mut`). The only release Perry emitted was the async-to-generator +transform's terminal `Stmt::ReleaseBoxes` (#7933/#8208/#8303), so a synchronous +function, method, arrow, generator — or an `async` function with no `await` — +leaked one registered root per boxed binding per call, plus everything that +binding last pointed at. `PERRY_GC_DIAG=1` reported `releases=0` for every +non-async workload; the issue's repro reached 601 MB RSS at 100k calls where the +equal-allocation control stayed at 69 MB, and real packages accumulated cells by +the hundred thousand (qs 792k, dayjs 434k). + +**Root cause.** `js_box_alloc_bits` registers each cell for the life of the +thread, and only `perry-transform`'s async step lowering produced the +`Stmt::ReleaseBoxes` that `emit_release_boxes` lowers. Nothing named the cells of +an ordinary frame, so nothing could ever reclaim them. + +**Fix.** Codegen registers every entry slot that holds a cell *this* frame minted +(`stmt/boxed_frame_release.rs`); the return-site rewrite that already injects +`js_shadow_frame_pop` now also emits `js_box_scope_release` for each of them +before every `ret`, and a declaration inside a loop releases the previous +iteration's cell before minting the next. The runtime publishes a cell no closure +captured (de-register, cache-evict, clear, free-list push) and marks a captured +one frame-released in its capture-edge record, so the last capture edge's GC +death publishes it — the same escape contract #8303 built for async activations, +including the full-trace ephemeron rule that keeps `box -> closure -> same box` +collectable. Two holders the runtime cannot count keep their cells: a sloppy-mode +mapped `arguments` object, and a plain-async step closure's own activation cells. +A step closure's capture of an *enclosing* frame's cell is now counted, because +the activation token never covered it and that frame does release its cells. + +Fixing this exposed a latent GC hole shared with #8303: a full trace that stops +rooting a released cell must still keep it in the box young log, because a minor +walks only that log. Without it the next minor had no root for a payload a live +closure still reads — `PERRY_GC_SCHEDULE_SEED=1 PERRY_GC_SCHEDULE_RATE=1 +PERRY_GC_PROTECT_FROMSPACE=1` faults on it immediately, and a unit test now +re-derives the rule. + +**Validation.** New gap test `test_gap_10464_box_cell_release.ts` (the repro's +memory shape compared against its own control in-process, plus escaping-closure, +per-iteration-binding, generator, class, async and self-cycle cases) differs from +Node on the parent commit and matches it here. GC stress over seeds 1-7 at +`PERRY_GC_SCHEDULE_RATE=1` with the from-space quarantine confirmed armed +(`[gc-fromspace-protect] retired_set=#0`, 141,635 hits over the run) held +byte-identical to Node on every deterministic output line, and +`PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1` (panics on any stale +forwarded-pointer read) passed clean. Instructions *improve* slightly on a hot +captured-variable loop (20M calls, -3.6%) and on 1M closure creations (-2.6%), +from reduced GC root-scanning pressure; the issue's `payload` repro reproduces +the RSS claim independently: 608 MB -> 152 MB peak RSS here (originally recorded +616 MB -> 163 MB on the destroyed build host). diff --git a/crates/perry-codegen/src/codegen/arguments.rs b/crates/perry-codegen/src/codegen/arguments.rs index 231a58fde4..ba72301e88 100644 --- a/crates/perry-codegen/src/codegen/arguments.rs +++ b/crates/perry-codegen/src/codegen/arguments.rs @@ -50,6 +50,34 @@ pub(crate) fn store_param_slot( slot } +/// #10464: a boxed parameter's cell is minted by this frame's entry block +/// (`store_param_slot`), so the frame releases it before every `ret`. +/// `materialize_arguments_object` withdraws a slot it maps into a sloppy-mode +/// `arguments` object, which holds the raw cell without a counted edge. +pub(crate) fn release_boxed_param_slots_at_exit( + lf: &mut crate::function::LlFunction, + params: &[Param], + boxed_vars: &HashSet, + slots: &std::collections::HashMap, +) { + for p in params { + if !boxed_vars.contains(&p.id) || p.arguments_object.is_some() { + continue; + } + if let Some(slot) = slots.get(&p.id) { + lf.add_pre_return_box_release(slot, "js_box_scope_release"); + } + } +} + +/// The parameter ids a synthesized `arguments` object aliases. +pub(crate) fn mapped_parameter_ids(params: &[Param]) -> HashSet { + mapped_arguments_params(params) + .into_iter() + .map(|(_, id)| id) + .collect() +} + pub(crate) fn materialize_arguments_object( ctx: &mut FnCtx<'_>, params: &[Param], @@ -110,6 +138,8 @@ pub(crate) fn materialize_arguments_object( ); for (arg_index, param_id) in mapped_arguments_params(params) { if let Some(param_slot) = ctx.locals.get(¶m_id).cloned() { + // #10464: the object aliases the cell for its own lifetime. + ctx.func.forget_pre_return_box_release(¶m_slot); let box_ptr = ctx.block().load(I64, ¶m_slot); ctx.block().call_void( "js_arguments_object_map_index", diff --git a/crates/perry-codegen/src/codegen/closure.rs b/crates/perry-codegen/src/codegen/closure.rs index 2772017112..8ce9a39ab6 100644 --- a/crates/perry-codegen/src/codegen/closure.rs +++ b/crates/perry-codegen/src/codegen/closure.rs @@ -639,6 +639,7 @@ pub(super) fn compile_closure( } map }; + super::arguments::release_boxed_param_slots_at_exit(lf, params, &closure_boxed_vars, &locals); // Start with the closure's own params as local_types, then // merge in the module-wide map so captured-from-outer ids have diff --git a/crates/perry-codegen/src/codegen/function.rs b/crates/perry-codegen/src/codegen/function.rs index f4522072b9..cbc2d25c47 100644 --- a/crates/perry-codegen/src/codegen/function.rs +++ b/crates/perry-codegen/src/codegen/function.rs @@ -863,6 +863,7 @@ pub(super) fn compile_function( } map }; + super::arguments::release_boxed_param_slots_at_exit(lf, &f.params, &boxed_vars, &locals); // Param types feed local_types so type-aware dispatch (e.g. string // concat detection on a `: string` parameter) works inside the body. diff --git a/crates/perry-codegen/src/codegen/method.rs b/crates/perry-codegen/src/codegen/method.rs index 17efc82af9..0e77806ec2 100644 --- a/crates/perry-codegen/src/codegen/method.rs +++ b/crates/perry-codegen/src/codegen/method.rs @@ -384,6 +384,12 @@ pub(super) fn compile_method( } (this_slot, map) }; + super::arguments::release_boxed_param_slots_at_exit( + lf, + &method.params, + &method_boxed_vars, + &locals, + ); let mut local_types: HashMap = module_global_types .iter() diff --git a/crates/perry-codegen/src/codegen/method_static.rs b/crates/perry-codegen/src/codegen/method_static.rs index f33abb28f3..eef81c0da4 100644 --- a/crates/perry-codegen/src/codegen/method_static.rs +++ b/crates/perry-codegen/src/codegen/method_static.rs @@ -122,6 +122,12 @@ pub(in crate::codegen) fn compile_static_method( } (this_slot, map) }; + crate::codegen::arguments::release_boxed_param_slots_at_exit( + lf, + &f.params, + &static_boxed_vars, + &locals, + ); // Seed with module-global declared types (mirrors compile_method / // compile_function): static-method bodies read module globals through diff --git a/crates/perry-codegen/src/expr/closure.rs b/crates/perry-codegen/src/expr/closure.rs index f53c588aef..7abb8bb0a8 100644 --- a/crates/perry-codegen/src/expr/closure.rs +++ b/crates/perry-codegen/src/expr/closure.rs @@ -12,57 +12,76 @@ use crate::types::{DOUBLE, I32, I64, PTR}; use super::{lower_expr, nanbox_pointer_inline, FnCtx}; -/// Whether this is the compiler-private step closure for a lowered plain -/// async activation. `ReleaseBoxes` is emitted only in that closure's -/// terminal arms; user-authored closures can never contain it. +/// The activation cells of the compiler-private step closure for a lowered +/// plain async activation: every id its terminal `ReleaseBoxes` arms name, or +/// `None` for any other closure. `ReleaseBoxes` is emitted only in that +/// closure's terminal arms; user-authored closures can never contain it. /// /// Queued and running instances of this closure are already covered by the -/// activation token's refcount. Counting its boxed capture slots as escaping +/// activation token's refcount. Counting those boxed capture slots as escaping /// GC-closure edges would make every cell in the complete activation frame -/// wait for a full collection, even when no user closure can observe it. -fn is_plain_async_step_body(stmts: &[Stmt]) -> bool { - stmts.iter().any(|stmt| match stmt { - Stmt::ReleaseBoxes(_) => true, - Stmt::If { - then_branch, - else_branch, - .. - } => { - is_plain_async_step_body(then_branch) - || else_branch.as_deref().is_some_and(is_plain_async_step_body) - } - Stmt::While { body, .. } | Stmt::DoWhile { body, .. } => is_plain_async_step_body(body), - Stmt::For { init, body, .. } => { - init.as_deref() - .is_some_and(|stmt| is_plain_async_step_body(std::slice::from_ref(stmt))) - || is_plain_async_step_body(body) - } - Stmt::Try { - body, - catch, - finally, - } => { - is_plain_async_step_body(body) - || catch - .as_ref() - .is_some_and(|catch| is_plain_async_step_body(&catch.body)) - || finally.as_deref().is_some_and(is_plain_async_step_body) +/// wait for a full collection, even when no user closure can observe it. A +/// cell from an ENCLOSING scope is not covered by that token (#10464: its +/// owner frame now releases it at scope exit), so only these ids go uncounted. +fn plain_async_step_release_ids(stmts: &[Stmt]) -> Option> { + fn walk(stmts: &[Stmt], out: &mut Option>) { + for stmt in stmts { + match stmt { + Stmt::ReleaseBoxes(ids) => out + .get_or_insert_with(Default::default) + .extend(ids.iter().copied()), + Stmt::If { + then_branch, + else_branch, + .. + } => { + walk(then_branch, out); + if let Some(else_branch) = else_branch { + walk(else_branch, out); + } + } + Stmt::While { body, .. } | Stmt::DoWhile { body, .. } => walk(body, out), + Stmt::For { init, body, .. } => { + if let Some(init) = init { + walk(std::slice::from_ref(init.as_ref()), out); + } + walk(body, out); + } + Stmt::Try { + body, + catch, + finally, + } => { + walk(body, out); + if let Some(catch) = catch { + walk(&catch.body, out); + } + if let Some(finally) = finally { + walk(finally, out); + } + } + Stmt::Switch { cases, .. } => { + for case in cases { + walk(&case.body, out); + } + } + Stmt::Labeled { body, .. } => walk(std::slice::from_ref(body.as_ref()), out), + Stmt::Let { .. } + | Stmt::Expr(_) + | Stmt::Return(_) + | Stmt::Break + | Stmt::Continue + | Stmt::LabeledBreak(_) + | Stmt::LabeledContinue(_) + | Stmt::Throw(_) + | Stmt::PreallocateBoxes(_) + | Stmt::PreallocateTdzBoxes(_) => {} + } } - Stmt::Switch { cases, .. } => cases - .iter() - .any(|case| is_plain_async_step_body(&case.body)), - Stmt::Labeled { body, .. } => is_plain_async_step_body(std::slice::from_ref(body.as_ref())), - Stmt::Let { .. } - | Stmt::Expr(_) - | Stmt::Return(_) - | Stmt::Break - | Stmt::Continue - | Stmt::LabeledBreak(_) - | Stmt::LabeledContinue(_) - | Stmt::Throw(_) - | Stmt::PreallocateBoxes(_) - | Stmt::PreallocateTdzBoxes(_) => false, - }) + } + let mut out = None; + walk(stmts, &mut out); + out } pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { @@ -130,6 +149,12 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // closure body can deref it via js_box_get/set. Without // this, each closure would get a snapshot of the box's // current value. + let plain_async_step_cells = plain_async_step_release_ids(body); + let uncounted_box_capture = |cap_id: &u32| { + plain_async_step_cells + .as_ref() + .is_some_and(|cells| cells.contains(cap_id)) + }; let mut captured_value_bits: Vec = Vec::with_capacity(auto_captures.len()); for cap_id in &auto_captures { if ctx.boxed_vars.contains(cap_id) { @@ -156,6 +181,11 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { } else if let Some(slot) = ctx.locals.get(cap_id).cloned() { // Enclosing function owns the box: slot holds // the raw box pointer as i64. + if uncounted_box_capture(cap_id) { + // #10464: the activation, not this frame, owns + // the cell's lifetime from here on. + ctx.func.forget_pre_return_box_release(&slot); + } let box_ptr = ctx.block().load(I64, &slot); captured_value_bits.push(box_ptr); } else if let Some(global_name) = ctx.module_globals.get(cap_id).cloned() { @@ -297,13 +327,13 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // // The singleton caches therefore only serve closures the // COMPILER synthesized: the async-activation step closures - // recognized by `is_plain_async_step_body` (their terminal + // recognized by `plain_async_step_release_ids` (their terminal // `ReleaseBoxes` arms cannot appear in user code, and their // identity never escapes the runtime's promise machinery). // Those are the closures the caches were built for — re-created // per resume with the same per-activation box captures. User // arrows and function expressions always mint fresh objects. - let is_plain_async_step = is_plain_async_step_body(body); + let is_plain_async_step = plain_async_step_cells.is_some(); let singleton_identity_safe = is_plain_async_step && (*is_arrow || captures_all_boxed); let no_capture_singleton = is_plain_async_step && *is_arrow && total_caps == 0; let captured_singleton = @@ -347,9 +377,9 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { && !captured_singleton && total_caps > 0 && !captured_value_bits.is_empty() - && auto_captures - .iter() - .all(|cap_id| is_plain_async_step || !ctx.boxed_vars.contains(cap_id)); + && auto_captures.iter().all(|cap_id| { + !ctx.boxed_vars.contains(cap_id) || uncounted_box_capture(cap_id) + }); let closure_handle = if no_capture_singleton { let blk = ctx.block(); blk.call(I64, "js_closure_alloc_singleton", &[(PTR, &func_ref)]) @@ -435,17 +465,19 @@ pub(crate) fn lower(ctx: &mut FnCtx<'_>, expr: &Expr) -> Result { // their lifetime edges are declared; fresh closures need every // slot initialized here. The compiler-private plain-async step // closure is different: its activation refcount already covers - // every queued/running instance, so declaring its whole boxed - // frame as escaped would delay every terminal cell until a full - // GC. User closures nested inside it still take the dedicated - // setter and therefore preserve #8213's escaped-cell lifetime. - let boxed_capture_slots = auto_captures + // every queued/running instance of its OWN cells, so declaring its + // whole boxed frame as escaped would delay every terminal cell + // until a full GC. User closures nested inside it still take the + // dedicated setter and therefore preserve #8213's escaped-cell + // lifetime, and so does a step closure's capture of an enclosing + // scope's cell (#10464). + let tracked_box_capture_slots = auto_captures .iter() - .map(|cap_id| ctx.boxed_vars.contains(cap_id)) + .map(|cap_id| ctx.boxed_vars.contains(cap_id) && !uncounted_box_capture(cap_id)) .collect::>(); let blk = ctx.block(); for (idx, val_bits) in captured_value_bits.iter().enumerate() { - let track_box_capture = boxed_capture_slots[idx] && !is_plain_async_step; + let track_box_capture = tracked_box_capture_slots[idx]; if bulk_fresh_init { // Every slot was written by `js_closure_alloc_init`. continue; diff --git a/crates/perry-codegen/src/function.rs b/crates/perry-codegen/src/function.rs index dff3fb48e2..641036385a 100644 --- a/crates/perry-codegen/src/function.rs +++ b/crates/perry-codegen/src/function.rs @@ -178,6 +178,14 @@ pub struct LlFunction { /// Entry/module-init functions use this for process-level diagnostics /// that must run regardless of which block reaches the normal epilogue. pre_return_void_calls: Vec, + /// #10464: entry-alloca slots holding a variable-box cell this frame + /// minted, paired with the kind's `js_*box_scope_release`. Each `ret` + /// hands the slot's current cell back to the runtime (a no-op for a slot + /// still holding its TAG_UNDEFINED entry sentinel). + pre_return_box_releases: Vec<(String, &'static str)>, + /// Slots withdrawn by [`Self::forget_pre_return_box_release`]; a later + /// registration of the same slot stays withdrawn. + withheld_box_release_slots: Vec, } /// Render the frame-push instruction. Kept in one place so the eager @@ -282,6 +290,8 @@ impl LlFunction { stack_map_slot_count: 0, force_shadow_frame: false, pre_return_void_calls: Vec::new(), + pre_return_box_releases: Vec::new(), + withheld_box_release_slots: Vec::new(), } } @@ -497,6 +507,29 @@ impl LlFunction { self.pre_return_void_calls.push(func_name.into()); } + /// #10464: release the variable-box cell held by `slot` before every + /// `ret`. `slot` must be an entry-block alloca whose value is a box + /// pointer or TAG_UNDEFINED on every path. Idempotent per slot. + pub fn add_pre_return_box_release(&mut self, slot: &str, release_fn: &'static str) { + if !self.pre_return_box_releases.iter().any(|(s, _)| s == slot) + && !self.withheld_box_release_slots.iter().any(|s| s == slot) + { + self.pre_return_box_releases + .push((slot.to_string(), release_fn)); + } + } + + /// Withdraw a slot registered by [`Self::add_pre_return_box_release`] + /// because a holder the runtime does not count (a mapped `arguments` + /// object, a plain-async step closure) received its cell. Sticky: the + /// slot is never released by this frame afterwards. + pub fn forget_pre_return_box_release(&mut self, slot: &str) { + self.pre_return_box_releases.retain(|(s, _)| s != slot); + if !self.withheld_box_release_slots.iter().any(|s| s == slot) { + self.withheld_box_release_slots.push(slot.to_string()); + } + } + /// Invoke-EH (#7302): enter/leave a handler scope. While a scope is /// active, every potentially-throwing call any block of this function /// emits carries an unwind edge to the scope's landing-pad label. @@ -1092,8 +1125,9 @@ impl LlFunction { &self, sink: &mut dyn FnMut(FinalItem<'_>) -> Result<(), E>, ) -> Result<(), E> { - let rewrite_rets = - self.shadow_frame_slot.is_some() || !self.pre_return_void_calls.is_empty(); + let rewrite_rets = self.shadow_frame_slot.is_some() + || !self.pre_return_void_calls.is_empty() + || !self.pre_return_box_releases.is_empty(); let mut seq: u32 = 0; for (i, blk) in self.blocks.iter().enumerate() { if i > 0 { @@ -1203,6 +1237,18 @@ impl LlFunction { for func_name in &self.pre_return_void_calls { sink(FinalItem::Text(&format!(" call void @{}()", func_name)))?; } + for (slot, release_fn) in &self.pre_return_box_releases { + let load_reg = format!("%box_release_l_{}", seq); + *seq += 1; + sink(FinalItem::Text(&format!( + " {} = load i64, ptr {}", + load_reg, slot + )))?; + sink(FinalItem::Text(&format!( + " call void @{}(i64 {})", + release_fn, load_reg + )))?; + } if let Some(handle_slot) = &self.shadow_frame_slot { let load_reg = format!("%shadow_pop_l_{}", seq); *seq += 1; diff --git a/crates/perry-codegen/src/gc_call_effects.rs b/crates/perry-codegen/src/gc_call_effects.rs index 265da914e7..6be9a6379a 100644 --- a/crates/perry-codegen/src/gc_call_effects.rs +++ b/crates/perry-codegen/src/gc_call_effects.rs @@ -258,7 +258,13 @@ pub(crate) fn classify_direct_callee(name: &str) -> GcCallEffect { // collection trigger — the same audit as the accessors above. | "js_box_release" | "js_i32_box_release" - | "js_bool_box_release" => GcCallEffect::CannotCollect, + | "js_bool_box_release" + // #10464 scope-exit release: a registry probe, a capture-count + // lookup, then either the same publish (registry remove, cache evict, + // raw clear, TLS free-list push) or a TLS pending-map insert. + | "js_box_scope_release" + | "js_i32_box_scope_release" + | "js_bool_box_scope_release" => GcCallEffect::CannotCollect, // Audited allocate-but-never-reenter helpers (2026-07-31): each body // was checked for closure invocation, coercion (valueOf/toString), // and accessor dispatch — none present, and none takes a receiver @@ -835,6 +841,9 @@ mod tests { "js_box_release", "js_i32_box_release", "js_bool_box_release", + "js_box_scope_release", + "js_i32_box_scope_release", + "js_bool_box_scope_release", ] { assert_eq!( classify_direct_callee(name), diff --git a/crates/perry-codegen/src/lower_call/new_ctor_args.rs b/crates/perry-codegen/src/lower_call/new_ctor_args.rs index de9bd00491..b4948fc297 100644 --- a/crates/perry-codegen/src/lower_call/new_ctor_args.rs +++ b/crates/perry-codegen/src/lower_call/new_ctor_args.rs @@ -75,6 +75,7 @@ pub(crate) fn bind_inline_constructor_params( .collect(); crate::codegen::arguments::add_arguments_mapped_boxes(params, &mut ctx.boxed_vars); + let mapped_param_ids = crate::codegen::arguments::mapped_parameter_ids(params); let values = inline_constructor_param_values_with_class(ctx, params, lowered_args, capture_fill); for ((param, arg_val), proof) in params @@ -86,7 +87,21 @@ pub(crate) fn bind_inline_constructor_params( let slot = ctx .func .alloca_entry(if boxed_param { I64 } else { DOUBLE }); - if boxed_param { + if boxed_param && !mapped_param_ids.contains(¶m.id) { + // #10464: this frame mints the cell (again per iteration when the + // `new` sits in a loop), so it also releases it. + let arg_bits = ctx.block().bitcast_double_to_i64(arg_val); + ctx.func + .entry_allocas_push_store(I64, crate::nanbox::TAG_UNDEFINED_I64, &slot); + use crate::stmt::boxed_frame_release as frame_release; + frame_release::mint_frame_cell( + ctx, + &slot, + "js_box_alloc_bits", + &[(I64, &arg_bits)], + frame_release::JS_BOX_SCOPE_RELEASE, + ); + } else if boxed_param { let arg_bits = ctx.block().bitcast_double_to_i64(arg_val); let box_ptr = ctx .block() diff --git a/crates/perry-codegen/src/runtime_decls/strings.rs b/crates/perry-codegen/src/runtime_decls/strings.rs index a73cd55079..3988b944c4 100644 --- a/crates/perry-codegen/src/runtime_decls/strings.rs +++ b/crates/perry-codegen/src/runtime_decls/strings.rs @@ -1117,6 +1117,10 @@ pub fn declare_phase_b_strings(module: &mut LlModule) { module.declare_function("js_box_release", VOID, &[I64]); module.declare_function("js_i32_box_release", VOID, &[I64]); module.declare_function("js_bool_box_release", VOID, &[I64]); + // #10464: frame-exit release of cells an ordinary frame minted. + module.declare_function("js_box_scope_release", VOID, &[I64]); + module.declare_function("js_i32_box_scope_release", VOID, &[I64]); + module.declare_function("js_bool_box_scope_release", VOID, &[I64]); module.declare_function("js_bool_box_alloc", I64, &[I32]); module.declare_function("js_bool_box_get", I32, &[I64]); module.declare_function("js_bool_box_set", VOID, &[I64, I32]); diff --git a/crates/perry-codegen/src/stmt/boxed_frame_release.rs b/crates/perry-codegen/src/stmt/boxed_frame_release.rs new file mode 100644 index 0000000000..f01aef30a0 --- /dev/null +++ b/crates/perry-codegen/src/stmt/boxed_frame_release.rs @@ -0,0 +1,65 @@ +//! #10464: hand a frame's variable-box cells back to the runtime when the +//! frame can no longer name them. +//! +//! A boxed local's entry alloca holds the cell this frame minted (or its +//! TAG_UNDEFINED entry sentinel). Two points end the frame's hold on that +//! cell: every `ret` (registered here, emitted by the `LlFunction` return-site +//! rewrite so no return path can be missed), and a declaration inside a loop +//! minting the next iteration's cell into the same slot. The runtime +//! (`perry_runtime::r#box::scope_release`) publishes a cell nothing else +//! captured and leaves a closure-captured cell to its closures' GC death. +//! +//! Only holders the runtime counts may keep a released cell alive. The two +//! compiler-emitted holders it does not count withdraw the slot instead: +//! a mapped sloppy-mode `arguments` object (`codegen/arguments.rs`) and a +//! plain-async step closure's own activation cells (`expr/closure.rs`). + +use crate::expr::FnCtx; +use crate::types::{LlvmType, I64}; + +pub(crate) const JS_BOX_SCOPE_RELEASE: &str = "js_box_scope_release"; +pub(crate) const I32_BOX_SCOPE_RELEASE: &str = "js_i32_box_scope_release"; +pub(crate) const BOOL_BOX_SCOPE_RELEASE: &str = "js_bool_box_scope_release"; + +/// Release `slot`'s cell before every `ret` of the current function. +pub(crate) fn release_at_frame_exit(ctx: &mut FnCtx<'_>, slot: &str, release_fn: &'static str) { + ctx.func.add_pre_return_box_release(slot, release_fn); +} + +/// A declaration about to mint a fresh cell into `slot` re-executes only in a +/// loop; the previous iteration's cell is then unnameable by this frame. +/// Outside a loop the slot still holds its entry sentinel, so nothing is +/// emitted. `switch` frames push an empty continue label and do not count. +pub(crate) fn release_previous_iteration_cell( + ctx: &mut FnCtx<'_>, + slot: &str, + release_fn: &'static str, +) { + let in_loop = ctx + .loop_targets + .iter() + .any(|(continue_label, _, _)| !continue_label.is_empty()); + if !in_loop { + return; + } + let previous = ctx.block().load(I64, slot); + ctx.block().call_void(release_fn, &[(I64, &previous)]); +} + +/// Store a freshly minted cell (`alloc_fn(args)`) into this frame's entry +/// `slot`: release the previous iteration's cell first (so an uncaptured one +/// is reused by this very allocation), and release the slot at frame exit. +/// Returns the new cell pointer. +pub(crate) fn mint_frame_cell( + ctx: &mut FnCtx<'_>, + slot: &str, + alloc_fn: &str, + args: &[(LlvmType, &str)], + release_fn: &'static str, +) -> String { + release_previous_iteration_cell(ctx, slot, release_fn); + let cell = ctx.block().call(I64, alloc_fn, args); + ctx.block().store(I64, &cell, slot); + release_at_frame_exit(ctx, slot, release_fn); + cell +} diff --git a/crates/perry-codegen/src/stmt/boxed_frame_release_tests.rs b/crates/perry-codegen/src/stmt/boxed_frame_release_tests.rs new file mode 100644 index 0000000000..08f10d66ba --- /dev/null +++ b/crates/perry-codegen/src/stmt/boxed_frame_release_tests.rs @@ -0,0 +1,293 @@ +//! #10464: an ordinary frame releases the box cells it minted. +//! +//! Each fixture pairs the released shape with the holder the runtime cannot +//! count, so the assertions discriminate in both directions: dropping the +//! frame release loses the positive assertions, releasing too much trips the +//! negative ones. + +use perry_hir::types::Type; +use perry_hir::{ArgumentsObjectMeta, Expr, Function, Module, Param, Stmt}; + +const RELEASE: &str = "call void @js_box_scope_release(i64 "; + +fn param(id: u32, name: &str) -> Param { + Param { + id, + name: name.into(), + ty: Type::Any, + default: None, + decorators: Vec::new(), + is_rest: false, + arguments_object: None, + } +} + +fn closure(func_id: u32, body: Vec, captures: Vec) -> Expr { + Expr::Closure { + func_id, + params: Vec::new(), + return_type: Type::Any, + body, + mutable_captures: captures.clone(), + captures, + captures_this: false, + captures_new_target: false, + enclosing_class: None, + is_arrow: true, + is_async: false, + is_generator: false, + is_strict: true, + } +} + +fn let_stmt(id: u32, init: Expr) -> Stmt { + Stmt::Let { + id, + name: format!("v{id}"), + ty: Type::Any, + mutable: true, + init: Some(init), + } +} + +fn function(name: &str, params: Vec, body: Vec) -> Function { + Function { + id: 1, + name: name.into(), + type_params: Vec::new(), + params, + return_type: Type::Any, + body, + is_async: false, + is_generator: false, + is_strict: true, + is_exported: false, + captures: Vec::new(), + decorators: Vec::new(), + was_plain_async: false, + was_unrolled: false, + } +} + +fn function_ir(f: Function) -> String { + let mut module = Module::new("boxed_frame_release.ts"); + let needle = format!("__{}(", f.name); + module.functions.push(f); + let ir = String::from_utf8( + crate::compile_module(&module, super::prealloc_module_global_tests::ir_opts()).unwrap(), + ) + .unwrap(); + ir.split("\ndefine ") + .find(|block| block.lines().next().is_some_and(|l| l.contains(&needle))) + .unwrap_or_else(|| panic!("no define for {needle}:\n{ir}")) + .to_string() +} + +/// Every `ret` of the frame is preceded by one release per slot it names. +fn releases_before_each_ret(ir: &str) -> Vec { + let lines: Vec<&str> = ir.lines().collect(); + let mut counts = Vec::new(); + for (i, line) in lines.iter().enumerate() { + if line.trim_start().starts_with("ret ") { + let mut n = 0; + for prev in lines[..i].iter().rev() { + let t = prev.trim_start(); + if t.starts_with("call void @js_shadow_frame_pop") || t.contains("= load i64, ptr") + { + continue; + } + if t.starts_with(RELEASE) { + n += 1; + continue; + } + break; + } + counts.push(n); + } + } + counts +} + +#[test] +fn captured_reassigned_let_is_released_at_every_return() { + // function counter(flag) { let n = 0; const inc = () => { n = 1 }; + // if (flag) return inc; return n; } + let body = vec![ + let_stmt(10, Expr::Integer(0)), + let_stmt( + 11, + closure( + 2, + vec![Stmt::Expr(Expr::LocalSet(10, Box::new(Expr::Integer(1))))], + vec![10], + ), + ), + Stmt::If { + condition: Expr::LocalGet(1), + then_branch: vec![Stmt::Return(Some(Expr::LocalGet(11)))], + else_branch: None, + }, + Stmt::Return(Some(Expr::LocalGet(10))), + ]; + let ir = function_ir(function("counter", vec![param(1, "flag")], body)); + assert!( + ir.contains("call i64 @js_box_alloc_bits("), + "premise: n is boxed\n{ir}" + ); + assert!( + ir.contains("js_closure_set_box_capture_ptr"), + "premise: counted edge\n{ir}" + ); + let per_ret = releases_before_each_ret(&ir); + assert!(per_ret.len() >= 2, "both returns must be lowered:\n{ir}"); + assert!( + per_ret.iter().all(|n| *n == 1), + "each return releases the single frame-owned cell ({per_ret:?}):\n{ir}" + ); + // Outside a loop the declaration runs once: no previous-iteration release. + let alloc = ir.find("call i64 @js_box_alloc_bits(").unwrap(); + assert!( + !ir[..alloc].contains(RELEASE), + "no release before a one-shot mint:\n{ir}" + ); +} + +#[test] +fn loop_declaration_releases_the_previous_iterations_cell_before_minting() { + // while (flag) { let x = 0; keep = () => { x = 1 }; } + let body = vec![ + Stmt::While { + condition: Expr::LocalGet(1), + body: vec![ + let_stmt(20, Expr::Integer(0)), + let_stmt( + 21, + closure( + 3, + vec![Stmt::Expr(Expr::LocalSet(20, Box::new(Expr::Integer(1))))], + vec![20], + ), + ), + ], + }, + Stmt::Return(Some(Expr::Undefined)), + ]; + let ir = function_ir(function("looped", vec![param(1, "flag")], body)); + let lines: Vec<&str> = ir.lines().map(str::trim_start).collect(); + let alloc = lines + .iter() + .position(|l| l.contains("call i64 @js_box_alloc_bits(")) + .expect("premise: x is boxed"); + let cell = lines[alloc].split(" = ").next().unwrap(); + let slot = lines[alloc..] + .iter() + .find_map(|l| l.strip_prefix(&format!("store i64 {cell}, ptr "))) + .expect("the minted cell is stored in its slot"); + let previous = lines[..alloc] + .iter() + .rev() + .take(4) + .find_map(|l| l.strip_suffix(&format!(" = load i64, ptr {slot}"))) + .unwrap_or_else(|| panic!("the slot's previous cell is loaded before minting:\n{ir}")); + assert!( + lines[..alloc] + .iter() + .rev() + .take(4) + .any(|l| l.starts_with(&format!("{RELEASE}{previous})"))), + "and released before the next iteration's cell is minted:\n{ir}" + ); + assert!( + releases_before_each_ret(&ir).iter().all(|n| *n == 1), + "{ir}" + ); +} + +#[test] +fn mapped_arguments_parameter_is_never_released_by_its_frame() { + // Sloppy `function f(a, b) { const g = () => { a = 1; b = 2 }; return arguments; }` + // with only `a` mapped: the Arguments object aliases a's cell raw. + let mut arguments = param(3, "arguments"); + arguments.arguments_object = Some(ArgumentsObjectMeta { + strict: false, + simple_parameters: true, + mapped_parameter_ids: vec![(0, 1)], + restricted_callee: false, + }); + let body = vec![ + let_stmt( + 30, + closure( + 4, + vec![ + Stmt::Expr(Expr::LocalSet(1, Box::new(Expr::Integer(1)))), + Stmt::Expr(Expr::LocalSet(2, Box::new(Expr::Integer(2)))), + ], + vec![1, 2], + ), + ), + Stmt::Return(Some(Expr::LocalGet(3))), + ]; + let mut f = function( + "sloppy", + vec![param(1, "a"), param(2, "b"), arguments], + body, + ); + f.is_strict = false; + let ir = function_ir(f); + assert!( + ir.contains("js_arguments_object_map_index"), + "premise: a is mapped\n{ir}" + ); + assert_eq!( + ir.matches("call i64 @js_box_alloc_bits(").count(), + 2, + "premise: both parameters are boxed:\n{ir}" + ); + let per_ret = releases_before_each_ret(&ir); + assert!( + !per_ret.is_empty() && per_ret.iter().all(|n| *n == 1), + "only the unmapped parameter is released ({per_ret:?}):\n{ir}" + ); +} + +#[test] +fn plain_async_step_counts_only_enclosing_cells_and_frame_keeps_its_own() { + // An activation frame: OWN is named by the step closure's terminal + // `ReleaseBoxes`, OUTER is an ordinary captured-and-reassigned local. + const OWN: u32 = 40; + const OUTER: u32 = 41; + let step = closure( + 5, + vec![ + Stmt::Expr(Expr::LocalSet(OUTER, Box::new(Expr::Integer(1)))), + Stmt::Expr(Expr::LocalSet(OWN, Box::new(Expr::Integer(2)))), + Stmt::ReleaseBoxes(vec![OWN]), + Stmt::Return(Some(Expr::Undefined)), + ], + vec![OWN, OUTER], + ); + let body = vec![ + Stmt::PreallocateBoxes(vec![OWN]), + let_stmt(OUTER, Expr::Integer(0)), + let_stmt(42, step), + Stmt::Return(Some(Expr::LocalGet(42))), + ]; + let ir = function_ir(function("activation", Vec::new(), body)); + assert_eq!( + ir.matches("call i64 @js_box_alloc_bits(").count(), + 2, + "premise: both cells are minted by this frame:\n{ir}" + ); + assert_eq!( + ir.matches("call void @js_closure_set_box_capture_ptr(") + .count(), + 1, + "the enclosing cell is a counted edge, the activation's own is not:\n{ir}" + ); + let per_ret = releases_before_each_ret(&ir); + assert!( + !per_ret.is_empty() && per_ret.iter().all(|n| *n == 1), + "the frame releases OUTER only; OWN belongs to the activation ({per_ret:?}):\n{ir}" + ); +} diff --git a/crates/perry-codegen/src/stmt/boxed_local_init.rs b/crates/perry-codegen/src/stmt/boxed_local_init.rs index 418f7f2377..9fd2dc5d8d 100644 --- a/crates/perry-codegen/src/stmt/boxed_local_init.rs +++ b/crates/perry-codegen/src/stmt/boxed_local_init.rs @@ -24,18 +24,30 @@ pub(super) fn ensure_reused_box_is_initialized(ctx: &mut FnCtx<'_>, id: u32) { let ready_label = ctx.block_label(ready); ctx.block().cond_br(&missing, &allocate_label, &ready_label); ctx.current_block = allocate; - let cell = if crate::expr::is_compiler_private_async_i32_control_local(ctx, id) { - ctx.block().call(I64, "js_i32_box_alloc", &[(I32, "0")]) + use super::boxed_frame_release as frame_release; + let (cell, release_fn) = if crate::expr::is_compiler_private_async_i32_control_local(ctx, id) { + ( + ctx.block().call(I64, "js_i32_box_alloc", &[(I32, "0")]), + frame_release::I32_BOX_SCOPE_RELEASE, + ) } else if crate::expr::is_compiler_private_async_i1_control_local(ctx, id) { - ctx.block().call(I64, "js_bool_box_alloc", &[(I32, "0")]) + ( + ctx.block().call(I64, "js_bool_box_alloc", &[(I32, "0")]), + frame_release::BOOL_BOX_SCOPE_RELEASE, + ) } else { - ctx.block().call( - I64, - "js_box_alloc_bits", - &[(I64, crate::nanbox::TAG_UNDEFINED_I64)], + ( + ctx.block().call( + I64, + "js_box_alloc_bits", + &[(I64, crate::nanbox::TAG_UNDEFINED_I64)], + ), + frame_release::JS_BOX_SCOPE_RELEASE, ) }; ctx.block().store(I64, &cell, &slot); + // #10464: the cell is this frame's (a withdrawn slot stays withdrawn). + frame_release::release_at_frame_exit(ctx, &slot, release_fn); super::record_boxed_slot_js_value_bits(ctx, id, &cell, "boxed_let.reused_missing_box"); ctx.block().br(&ready_label); ctx.current_block = ready; diff --git a/crates/perry-codegen/src/stmt/let_stmt.rs b/crates/perry-codegen/src/stmt/let_stmt.rs index 251f4bbd8a..f360f0a353 100644 --- a/crates/perry-codegen/src/stmt/let_stmt.rs +++ b/crates/perry-codegen/src/stmt/let_stmt.rs @@ -1242,13 +1242,6 @@ pub(crate) fn lower_let( } return Ok(()); } - // Step 1: allocate box with undefined sentinel bits. - let blk = ctx.block(); - let box_ptr = blk.call( - crate::types::I64, - "js_box_alloc_bits", - &[(I64, crate::nanbox::TAG_UNDEFINED_I64)], - ); // Slot must live in the entry block — closures from sibling // branches may capture this id later, and an alloca placed // here would not dominate those branches' loads. @@ -1267,7 +1260,14 @@ pub(crate) fn lower_let( // deterministically. let undef_bits = crate::nanbox::TAG_UNDEFINED_I64.to_string(); ctx.func.entry_allocas_push_store(I64, &undef_bits, &slot); - ctx.block().store(I64, &box_ptr, &slot); + // Step 1: allocate the box (#10464: released by this frame). + let box_ptr = super::boxed_frame_release::mint_frame_cell( + ctx, + &slot, + "js_box_alloc_bits", + &[(I64, crate::nanbox::TAG_UNDEFINED_I64)], + super::boxed_frame_release::JS_BOX_SCOPE_RELEASE, + ); super::record_boxed_slot_js_value_bits(ctx, id, &box_ptr, "boxed_let.box_ptr_slot"); // Step 2: register BEFORE lowering init. ctx.locals.insert(id, slot); diff --git a/crates/perry-codegen/src/stmt/mod.rs b/crates/perry-codegen/src/stmt/mod.rs index a6ca584e6a..4846931d95 100644 --- a/crates/perry-codegen/src/stmt/mod.rs +++ b/crates/perry-codegen/src/stmt/mod.rs @@ -13,6 +13,9 @@ use crate::types::DOUBLE; #[cfg(test)] mod boxed_continuation_tests; +pub(crate) mod boxed_frame_release; +#[cfg(test)] +mod boxed_frame_release_tests; mod boxed_local_init; #[cfg(test)] mod boxed_slot_no_root_tests; @@ -665,6 +668,14 @@ pub(crate) fn lower_stmt(ctx: &mut FnCtx<'_>, stmt: &Stmt) -> Result<()> { } fn emit_preallocate_boxes(ctx: &mut FnCtx<'_>, ids: &[u32], tdz: bool) -> Result<()> { + // #10464: a generator/async activation frame's list names its + // compiler-private control cells. That list runs once per frame, and a + // plain-async step closure holds its cells without a counted capture edge, + // so it never releases a "previous iteration" cell. + let activation_frame = ids.iter().any(|id| { + ctx.compiler_private_async_i32_control_locals.contains(id) + || ctx.compiler_private_async_i1_control_locals.contains(id) + }); for id in ids { // #7521: a module-level binding promoted to `@perry_global___` // ALREADY has the shared, forward-visible, GC-rooted cell a prealloc box @@ -701,41 +712,35 @@ fn emit_preallocate_boxes(ctx: &mut FnCtx<'_>, ids: &[u32], tdz: bool) -> Result } let is_i32_control = crate::expr::is_compiler_private_async_i32_control_local(ctx, *id); let is_i1_control = crate::expr::is_compiler_private_async_i1_control_local(ctx, *id); - let blk = ctx.block(); - let (box_ptr, cell_note) = if is_i32_control { + // Seed the JSValue box with TAG_TDZ (Temporal Dead Zone) when + // requested -- a read before the declaration runs throws a spec + // ReferenceError via the runtime `js_box_get_bits` choke point. + // Compiler-private i32/i1 control cells are never TDZ. + let seed_bits = if tdz { + crate::nanbox::TAG_TDZ_I64.to_string() + } else { + crate::nanbox::TAG_UNDEFINED_I64.to_string() + }; + use boxed_frame_release as frame_release; + let (alloc_fn, alloc_arg, release_fn, cell_note) = if is_i32_control { ( - blk.call( - crate::types::I64, - "js_i32_box_alloc", - &[(crate::types::I32, "0")], - ), + "js_i32_box_alloc", + (crate::types::I32, "0"), + frame_release::I32_BOX_SCOPE_RELEASE, "primitive_i32_control_cell", ) } else if is_i1_control { ( - blk.call( - crate::types::I64, - "js_bool_box_alloc", - &[(crate::types::I32, "0")], - ), + "js_bool_box_alloc", + (crate::types::I32, "0"), + frame_release::BOOL_BOX_SCOPE_RELEASE, "primitive_i1_control_cell", ) } else { - // Seed the JSValue box with TAG_TDZ (Temporal Dead Zone) when - // requested -- a read before the declaration runs throws a spec - // ReferenceError via the runtime `js_box_get_bits` choke point. - // Compiler-private i32/i1 control cells are never TDZ. - let seed_bits = if tdz { - crate::nanbox::TAG_TDZ_I64.to_string() - } else { - crate::nanbox::TAG_UNDEFINED_I64.to_string() - }; ( - blk.call( - crate::types::I64, - "js_box_alloc_bits", - &[(crate::types::I64, &seed_bits)], - ), + "js_box_alloc_bits", + (crate::types::I64, seed_bits.as_str()), + frame_release::JS_BOX_SCOPE_RELEASE, "jsvalue_box_cell", ) }; @@ -762,7 +767,12 @@ fn emit_preallocate_boxes(ctx: &mut FnCtx<'_>, ids: &[u32], tdz: bool) -> Result .entry_allocas_push_store(crate::types::I64, &undef_bits, &slot); slot }; + if !activation_frame { + frame_release::release_previous_iteration_cell(ctx, &slot, release_fn); + } + let box_ptr = ctx.block().call(crate::types::I64, alloc_fn, &[alloc_arg]); ctx.block().store(crate::types::I64, &box_ptr, &slot); + frame_release::release_at_frame_exit(ctx, &slot, release_fn); record_boxed_slot_js_value_bits(ctx, *id, &box_ptr, "preallocate_boxes.box_ptr_slot"); if cell_note != "jsvalue_box_cell" { let lowered = LoweredValue::js_value_bits(&box_ptr); diff --git a/crates/perry-runtime/src/box.rs b/crates/perry-runtime/src/box.rs index 4eae4b605b..77e7ebf71b 100644 --- a/crates/perry-runtime/src/box.rs +++ b/crates/perry-runtime/src/box.rs @@ -457,27 +457,29 @@ fn push_free_cell(addr: usize, head: &'static crate::tls_hot::HotKey { - BOX_REGISTRY.with(|r| { - r.borrow_mut().remove(&addr); - }); + if !BOX_REGISTRY.with(|r| r.borrow_mut().remove(&addr)) { + return; + } box_ptr_cache_evict(box_ptr_cache(), addr); unsafe { (*(addr as *mut Box)).value = crate::value::TAG_UNDEFINED }; push_free_cell(addr, &BOX_FREE_HEAD); } ASYNC_RELEASE_I32 => { - I32_BOX_REGISTRY.with(|r| { - r.borrow_mut().remove(&addr); - }); + if !I32_BOX_REGISTRY.with(|r| r.borrow_mut().remove(&addr)) { + return; + } box_ptr_cache_evict(i32_box_ptr_cache(), addr); unsafe { (*(addr as *mut I32Box)).value = -1 }; push_free_cell(addr, &I32_BOX_FREE_HEAD); } ASYNC_RELEASE_BOOL => { - BOOL_BOX_REGISTRY.with(|r| { - r.borrow_mut().remove(&addr); - }); + if !BOOL_BOX_REGISTRY.with(|r| r.borrow_mut().remove(&addr)) { + return; + } box_ptr_cache_evict(bool_box_ptr_cache(), addr); unsafe { (*(addr as *mut BoolBox)).value = true }; push_free_cell(addr, &BOOL_BOX_FREE_HEAD); @@ -485,7 +487,10 @@ fn publish_box_cell(addr: usize, tag: usize) { _ => unreachable!("invalid async released-cell tag"), } ASYNC_PENDING_RELEASES.with(|pending| { - pending.borrow_mut().remove(&addr); + let mut pending = pending.borrow_mut(); + if !pending.is_empty() { + pending.remove(&addr); + } }); BOX_FLUSH_PUBLISHED.fetch_add(1, Ordering::Relaxed); } @@ -499,21 +504,22 @@ pub(crate) fn box_capture_count_reached_zero(addr: usize) { } } -/// Expose a drained, closure-owned JS box's payload to the closure tracer. -/// The exact-capture table may also contain i32/bool box addresses; requiring -/// the pending JS tag is the authoritative type discriminator before the -/// pointer is dereferenced as [`Box`]. +/// Expose a released, closure-owned JS box's payload to the closure tracer — +/// a drained async activation cell, or one its ordinary frame released +/// (#10464). The exact-capture table may also contain i32/bool box addresses; +/// requiring the JS tag on the release record is the authoritative type +/// discriminator before the pointer is dereferenced as [`Box`]. pub(crate) fn visit_pending_captured_js_box_payload_slot( addr: usize, visit: &mut dyn FnMut(*mut u64), ) { - let is_pending_js = ASYNC_PENDING_RELEASES.with(|pending| { + let is_released_js = ASYNC_PENDING_RELEASES.with(|pending| { pending .borrow() .get(&addr) .is_some_and(|tag| *tag == (ASYNC_RELEASE_JS | ASYNC_RELEASE_DRAINED)) - }); - if is_pending_js && BOX_REGISTRY.with(|registry| registry.borrow().contains(&addr)) { + }) || crate::closure::frame_released_js_cell(addr, ASYNC_RELEASE_JS); + if is_released_js && BOX_REGISTRY.with(|registry| registry.borrow().contains(&addr)) { let ptr = addr as *mut Box; unsafe { visit(&raw mut (*ptr).value) }; } @@ -965,19 +971,6 @@ pub fn scan_box_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { BOX_REGISTRY.with(|r| { let r = r.borrow(); for &addr in r.iter() { - // A drained box is retained only by exact closure-capture - // metadata. During a full trace its payload is reached from - // each live closure instead. Rooting it here as well would - // make `box -> closure -> same box` an uncollectable native - // cycle. Minors retain the old strong-root rule because they - // cannot adjudicate old-closure liveness. - if full_trace - && pending - .get(&addr) - .is_some_and(|tag| *tag == (ASYNC_RELEASE_JS | ASYNC_RELEASE_DRAINED)) - { - continue; - } let ptr = addr as *mut Box; // Defensive: the registry should only contain valid live // pointers, but if a stale entry slipped through we'd @@ -985,15 +978,40 @@ pub fn scan_box_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { // address (alloc gives 8-aligned pointers in user space) // matches `is_plausible_box_ptr` to keep this a no-op for // any pathological entry. - if addr >= 0x1000 && (addr as u64) < 0x0001_0000_0000_0000 && addr % 8 == 0 { + if !(addr >= 0x1000 && (addr as u64) < 0x0001_0000_0000_0000 && addr % 8 == 0) { + continue; + } + // A released box (a drained async cell, or a cell its + // ordinary frame released — #10464) is retained only by exact + // closure-capture metadata. During a full trace its payload is + // reached from each live closure instead. Rooting it here as + // well would make `box -> closure -> same box` an uncollectable + // native cycle. Minors retain the old strong-root rule because + // they cannot adjudicate old-closure liveness — and a minor + // walks ONLY the log below, so a young payload must stay logged + // even when this trace does not visit it. Dropping it here left + // the next minor with no root for a live payload + // (`gc-fromspace-protect` catches the stale use immediately). + if full_trace + && (pending + .get(&addr) + .is_some_and(|tag| *tag == (ASYNC_RELEASE_JS | ASYNC_RELEASE_DRAINED)) + || crate::closure::frame_released_js_cell(addr, ASYNC_RELEASE_JS)) + { unsafe { - visitor.visit_nanbox_u64_raw_slot(&raw mut (*ptr).value); if crate::gc::young_log::bits_are_minor_relevant((*ptr).value) { kept.push(addr); } } - visited += 1; + continue; } + unsafe { + visitor.visit_nanbox_u64_raw_slot(&raw mut (*ptr).value); + if crate::gc::young_log::bits_are_minor_relevant((*ptr).value) { + kept.push(addr); + } + } + visited += 1; } }); }); @@ -1905,3 +1923,11 @@ mod tests { #[cfg(test)] #[path = "box/release_tests.rs"] mod release_tests; + +// #10464: scope-exit release for frame-owned cells. +#[path = "box/scope_release.rs"] +mod scope_release; +pub(crate) use scope_release::publish_frame_released_cell; +pub use scope_release::{ + js_bool_box_scope_release, js_box_scope_release, js_i32_box_scope_release, +}; diff --git a/crates/perry-runtime/src/box/scope_release.rs b/crates/perry-runtime/src/box/scope_release.rs new file mode 100644 index 0000000000..b9274fc5ef --- /dev/null +++ b/crates/perry-runtime/src/box/scope_release.rs @@ -0,0 +1,346 @@ +//! #10464: release the box cells an ordinary frame minted once that frame can +//! no longer name them. +//! +//! A variable box is minted per execution of the declaration of a captured and +//! reassigned binding, and every registered cell is a strong GC root +//! (`scan_box_roots_mut`). Before #10464 only a lowered plain-async activation +//! ever released its cells (#7933/#8208), so a synchronous function, method, +//! arrow, generator, or an `async` function without `await` kept one registered +//! root per boxed binding per call alive for the life of the thread, together +//! with everything that binding last pointed at. +//! +//! Codegen now names each frame-owned cell before every `ret` of the frame and +//! before a declaration inside a loop mints the next iteration's cell. The +//! frame is the holder that disappears at those points; every other holder is +//! a compiler-declared closure capture edge (`js_closure_set_box_capture_ptr`), +//! which is exactly the edge set #8303 counts. So: +//! +//! - a cell with no capture edge publishes immediately: de-registered, +//! positive-cache-evicted, cleared, and pushed on its kind's free list; +//! - a captured cell stays registered and readable, and its capture-edge record +//! is marked frame-released. It publishes when authoritative GC death +//! pruning removes its last capture edge. Like a drained async terminal cell, +//! a full trace reaches its payload only through live capturing closures (the +//! ephemeron half), so a payload that references its own closure does not +//! keep either alive. +//! +//! A running closure keeps its own edges: every capturing closure body spills +//! `%this_closure` into a frame-long root slot (#7055), so a cell a closure body +//! cached at entry cannot be published while that body is still executing. +//! +//! These entries are deliberately separate from `js_*box_release`, which parks +//! into the AMBIENT async activation: a synchronous callee running inside an +//! async step must not append its own cells to the caller activation's +//! contiguous terminal release range. + +use super::*; + +#[inline] +fn release_scope_cell(addr: usize, tag: usize, registered: bool) { + // Not registered: a TAG_UNDEFINED slot whose declaration never ran, an + // already-published cell, or a foreign address. All are no-ops, which is + // what makes a repeated release unable to push one cell twice. + if !registered { + return; + } + // An async activation's terminal cell: its activation already decided. + let async_pending = ASYNC_PENDING_RELEASES.with(|pending| { + let pending = pending.borrow(); + !pending.is_empty() && pending.contains_key(&addr) + }); + if async_pending { + return; + } + match crate::closure::note_frame_released_cell(addr, tag) { + crate::closure::FrameRelease::Uncaptured => { + BOX_RELEASE_COUNT.fetch_add(1, Ordering::Relaxed); + publish_box_cell(addr, tag); + } + crate::closure::FrameRelease::Deferred => { + BOX_RELEASE_COUNT.fetch_add(1, Ordering::Relaxed); + } + crate::closure::FrameRelease::AlreadyReleased => {} + } +} + +/// The last capture edge of a frame-released cell disappeared (GC death +/// pruning): publish it for reuse. +pub(crate) fn publish_frame_released_cell(addr: usize, tag: usize) { + publish_box_cell(addr, tag); +} + +/// Release a JSValue box cell owned by the exiting (or re-entering) frame. +#[no_mangle] +pub extern "C" fn js_box_scope_release(ptr: *mut Box) { + release_scope_cell(ptr as usize, ASYNC_RELEASE_JS, is_registered_box_ptr(ptr)); +} + +/// [`js_box_scope_release`] for the compiler-private i32 control cells of a +/// generator frame. +#[no_mangle] +pub extern "C" fn js_i32_box_scope_release(ptr: *mut I32Box) { + release_scope_cell( + ptr as usize, + ASYNC_RELEASE_I32, + is_registered_i32_box_ptr(ptr), + ); +} + +/// [`js_box_scope_release`] for the compiler-private boolean control cells of +/// a generator frame. +#[no_mangle] +pub extern "C" fn js_bool_box_scope_release(ptr: *mut BoolBox) { + release_scope_cell( + ptr as usize, + ASYNC_RELEASE_BOOL, + is_registered_bool_box_ptr(ptr), + ); +} + +#[cfg(feature = "keepalive-anchors")] +#[used(compiler)] +static KEEP_JS_BOX_SCOPE_RELEASE: extern "C" fn(*mut Box) = js_box_scope_release; +#[cfg(feature = "keepalive-anchors")] +#[used(compiler)] +static KEEP_JS_I32_BOX_SCOPE_RELEASE: extern "C" fn(*mut I32Box) = js_i32_box_scope_release; +#[cfg(feature = "keepalive-anchors")] +#[used(compiler)] +static KEEP_JS_BOOL_BOX_SCOPE_RELEASE: extern "C" fn(*mut BoolBox) = js_bool_box_scope_release; + +#[cfg(test)] +mod tests { + use super::*; + + fn int_bits(value: i32) -> i64 { + crate::value::JSValue::int32(value).bits() as i64 + } + + fn free_list_contains(addr: usize) -> bool { + let mut cursor = BOX_FREE_HEAD.with(std::cell::Cell::get); + let mut steps = 0; + while cursor != 0 && steps < 1 << 20 { + if cursor == addr { + return true; + } + cursor = unsafe { (cursor as *const usize).read() }; + steps += 1; + } + false + } + + /// The common case: the frame was the cell's only holder. The cell is + /// inert at once and the next allocation reuses it; repeating the release + /// (a second `ret` path, a loop that exits right after re-entering) must + /// not push it onto the free list a second time. + #[test] + fn uncaptured_cell_publishes_at_scope_exit_exactly_once() { + test_clear_box_registry(); + let cell = js_box_alloc_bits(int_bits(7)); + js_box_scope_release(cell); + assert!(!is_registered_box_ptr(cell), "published cell de-registers"); + assert_eq!(js_box_get_bits(cell) as u64, crate::value::TAG_UNDEFINED); + js_box_scope_release(cell); + js_box_scope_release(cell); + + let first = js_box_alloc_bits(int_bits(1)); + let second = js_box_alloc_bits(int_bits(2)); + assert_eq!(first, cell, "the published cell is reused immediately"); + assert_ne!( + first, second, + "a repeated release must not alias two live bindings onto one cell" + ); + assert_eq!(js_box_get_bits(first), int_bits(1)); + assert_eq!(js_box_get_bits(second), int_bits(2)); + } + + /// A slot whose declaration never ran still holds TAG_UNDEFINED; codegen + /// releases it unconditionally at `ret`. Foreign pointers are rejected + /// by the same registry gate as every other box entry point. + #[test] + fn unminted_slot_values_and_foreign_pointers_are_no_ops() { + test_clear_box_registry(); + let live = js_box_alloc_bits(int_bits(3)); + js_box_scope_release(crate::value::TAG_UNDEFINED as usize as *mut Box); + js_box_scope_release(std::ptr::null_mut()); + static RODATA: [u64; 1] = [0xDEAD_BEEF]; + js_box_scope_release((&RODATA[0] as *const u64) as *mut Box); + assert_eq!(RODATA[0], 0xDEAD_BEEF); + assert!(is_registered_box_ptr(live)); + assert_eq!(js_box_get_bits(live), int_bits(3)); + assert!(!free_list_contains(live as usize)); + } + + /// `function counter() { let n = 0; return () => ++n; }`: the returned + /// closure outlives the frame. Its cell stays readable and writable after + /// scope exit, a closure created later from that closure adds its own + /// edge, and only the last closure's death publishes the cell. + #[test] + fn escaped_closure_keeps_its_cell_until_gc_death() { + test_clear_box_registry(); + let cell = js_box_alloc_bits(int_bits(0)); + let counter = crate::closure::js_closure_alloc(std::ptr::null(), 1); + crate::closure::js_closure_set_box_capture_ptr(counter, 0, cell as i64); + js_box_scope_release(cell); + + assert!(is_registered_box_ptr(cell), "an escaped closure owns it"); + js_box_set_bits(cell, int_bits(1)); + assert_eq!(js_box_get_bits(cell), int_bits(1)); + assert!(!free_list_contains(cell as usize)); + js_box_scope_release(cell); + assert!( + crate::closure::frame_released_js_cell(cell as usize, ASYNC_RELEASE_JS), + "a repeated scope release leaves the released state unchanged" + ); + assert_eq!(crate::closure::box_capture_count(cell as usize), 1); + + let child = crate::closure::js_closure_alloc(std::ptr::null(), 1); + crate::closure::js_closure_set_box_capture_ptr(child, 0, cell as i64); + crate::closure::prune_dead_closure_box_capture_owners(&|owner| owner == counter as usize); + assert!(is_registered_box_ptr(cell), "the child closure is live"); + assert_eq!(js_box_get_bits(cell), int_bits(1)); + + crate::closure::prune_dead_closure_box_capture_owners(&|owner| owner == child as usize); + assert!(!is_registered_box_ptr(cell)); + assert!(free_list_contains(cell as usize)); + let reused = js_box_alloc_bits(int_bits(9)); + assert_eq!(reused, cell, "last closure death publishes the cell"); + } + + /// A scope-released captured cell must follow the #8303 full-trace rule: + /// not a global root (else a payload that references its own closure is + /// immortal), but traced through a closure the mark set proved live. + #[test] + fn scope_released_captured_cell_is_an_ephemeron_edge_in_a_full_trace() { + test_clear_box_registry(); + let cell = js_box_alloc_bits(crate::value::TAG_UNDEFINED as i64); + let closure = crate::closure::js_closure_alloc(std::ptr::null(), 1); + crate::closure::js_closure_set_box_capture_ptr(closure, 0, cell as i64); + let closure_bits = crate::value::js_nanbox_pointer(closure as i64).to_bits(); + js_box_set_bits(cell, closure_bits as i64); + + let mut minor_roots = Vec::new(); + scan_box_roots(&mut |value| minor_roots.push(value.to_bits())); + assert!( + minor_roots.contains(&closure_bits), + "sabotage check: before release the registry roots the payload" + ); + js_box_scope_release(cell); + + crate::gc::begin_full_trace(); + let mut rooted = Vec::new(); + scan_box_roots(&mut |value| rooted.push(value.to_bits())); + assert!( + !rooted.contains(&closure_bits), + "a scope-released cell must not root its own closure in a full trace" + ); + let header = unsafe { + (closure as *mut u8).sub(crate::gc::GC_HEADER_SIZE) as *mut crate::gc::GcHeader + }; + let saved_flags = unsafe { (*header).gc_flags }; + unsafe { (*header).gc_flags |= crate::gc::GC_FLAG_MARKED }; + let slots = crate::gc::test_gc_rewrite_slot_addresses(closure as usize) + .expect("closure rewrite descriptor"); + unsafe { (*header).gc_flags = saved_flags }; + crate::gc::finish_full_trace(); + assert!( + slots.contains(&(cell as usize)), + "a marked closure must trace the scope-released cell's payload" + ); + + crate::closure::prune_dead_closure_box_capture_owners(&|owner| owner == closure as usize); + assert!(!is_registered_box_ptr(cell)); + } + + /// A minor walks ONLY the box young log. A full trace that stops rooting a + /// released cell must therefore still LOG it while its payload is young, + /// or the next minor has no root for a payload a live closure still reads. + /// `PERRY_GC_PROTECT_FROMSPACE` caught exactly this as a stale from-space + /// closure call; this test is its cheap, deterministic twin. + #[test] + fn a_full_trace_keeps_a_released_cell_in_the_minor_remembered_set() { + test_clear_box_registry(); + let cell = js_box_alloc_bits(crate::value::TAG_UNDEFINED as i64); + let closure = crate::closure::js_closure_alloc(std::ptr::null(), 1); + crate::closure::js_closure_set_box_capture_ptr(closure, 0, cell as i64); + let payload = crate::value::js_nanbox_pointer(closure as i64).to_bits(); + js_box_set_bits(cell, payload as i64); + assert!( + crate::gc::young_log::bits_are_minor_relevant(payload), + "premise: the payload is a young object a minor must rewrite" + ); + js_box_scope_release(cell); + + crate::gc::begin_full_trace(); + scan_box_roots(&mut |_| {}); + crate::gc::finish_full_trace(); + + BOX_YOUNG_ROOTS.with(|log| { + log.borrow() + .debug_assert_logged(BOX_YOUNG_LOG_NAME, &relevant_box_roots()) + }); + crate::closure::prune_dead_closure_box_capture_owners(&|owner| owner == closure as usize); + } + + /// A synchronous callee running inside an async step must not park its + /// cells into the caller's activation range (that is what + /// `js_box_release` would do): the scope release is independent of the + /// ambient activation, and the activation's own terminal release still + /// works afterwards. + #[test] + fn scope_release_ignores_the_ambient_async_activation() { + test_clear_box_registry(); + let activation = new_async_box_activation(); + retain_async_box_activation(activation); + let previous = crate::promise::INLINE_TRAP.with(|trap| { + trap.replace(crate::promise::InlineTrap { + trap_next: std::ptr::null_mut(), + current_step: 0, + box_activation: activation, + }) + }); + let callee_cell = js_box_alloc_bits(int_bits(4)); + js_box_scope_release(callee_cell); + assert!(!is_registered_box_ptr(callee_cell)); + assert_eq!( + unsafe { (*activation).release_start.get() }, + NO_RELEASE_RANGE, + "the callee's cell must not enter the caller's release range" + ); + + let frame_cell = js_box_alloc_bits(int_bits(5)); + js_box_release(frame_cell); + assert!(is_registered_box_ptr(frame_cell), "a step still owns it"); + release_async_box_activation(activation); + assert!(!is_registered_box_ptr(frame_cell)); + crate::promise::INLINE_TRAP.with(|trap| trap.set(previous)); + } + + /// Generator frames also own compiler-private i32/bool control cells. + /// Each kind publishes through its own registry, and a cell of one kind + /// is never accepted by another kind's release entry. + #[test] + fn primitive_control_cells_release_through_their_own_registries() { + test_clear_box_registry(); + let state = js_i32_box_alloc(3); + let done = js_bool_box_alloc(1); + let ordinary = js_box_alloc_bits(int_bits(6)); + + js_i32_box_scope_release(ordinary.cast::()); + js_bool_box_scope_release(ordinary.cast::()); + js_box_scope_release(state.cast::()); + assert!(is_registered_box_ptr(ordinary)); + assert!(is_registered_i32_box_ptr(state)); + + let closure = crate::closure::js_closure_alloc(std::ptr::null(), 1); + crate::closure::js_closure_set_box_capture_ptr(closure, 0, done as i64); + js_i32_box_scope_release(state); + js_bool_box_scope_release(done); + assert!(!is_registered_i32_box_ptr(state)); + assert!(is_registered_bool_box_ptr(done), "a live closure reads it"); + assert_eq!(js_bool_box_get(done), 1); + crate::closure::prune_dead_closure_box_capture_owners(&|owner| owner == closure as usize); + assert!(!is_registered_bool_box_ptr(done)); + assert_eq!(js_i32_box_alloc(8), state, "i32 free list reuses it"); + assert_eq!(js_bool_box_alloc(0), done, "bool free list reuses it"); + } +} diff --git a/crates/perry-runtime/src/closure/box_captures.rs b/crates/perry-runtime/src/closure/box_captures.rs index 2cc7fd7b4a..bdc7b3fda4 100644 --- a/crates/perry-runtime/src/closure/box_captures.rs +++ b/crates/perry-runtime/src/closure/box_captures.rs @@ -6,56 +6,162 @@ //! pointer-shaped bits. Once its async activation drains, the box runtime //! publishes an unobserved cell immediately and leaves only a captured cell //! pending. Closure moves rekey the per-closure index; authoritative GC death -//! pruning drops the corresponding per-cell counts. +//! pruning drops the corresponding per-cell counts. A cell released by the +//! ordinary frame that minted it (#10464) follows the same edges. use super::ClosureHeader; use std::cell::RefCell; -type BoxCaptureSlots = Vec<(u32, usize)>; +/// One closure's `(capture index, box address)` edges. Nearly every closure +/// declares one or two, and one is stored inline: a closure capturing a +/// reassigned local then costs no heap allocation for its lifetime record +/// (#10464 made those records a per-call cost of ordinary frames). +#[derive(Clone, Default)] +struct BoxCaptureSlots { + first: Option<(u32, usize)>, + rest: Vec<(u32, usize)>, +} + +impl BoxCaptureSlots { + fn take(&mut self, index: u32) -> Option { + if self.first.is_some_and(|(slot, _)| slot == index) { + let cell = self.first.take().map(|(_, cell)| cell); + self.first = self.rest.pop(); + return cell; + } + let pos = self.rest.iter().position(|(slot, _)| *slot == index)?; + Some(self.rest.swap_remove(pos).1) + } + + fn push(&mut self, index: u32, cell: usize) { + if self.first.is_none() { + self.first = Some((index, cell)); + } else { + self.rest.push((index, cell)); + } + } + + /// `rest` is non-empty only while `first` is occupied. + fn is_empty(&self) -> bool { + self.first.is_none() + } + + fn cells(&self) -> impl Iterator + '_ { + self.first + .iter() + .chain(self.rest.iter()) + .map(|(_, cell)| *cell) + } +} crate::perry_thread_local! { /// Closure address -> compiler-declared `(capture index, box address)` edges. static CLOSURE_BOX_CELLS: RefCell> = RefCell::new(crate::fast_hash::new_ptr_hash_map()); - /// Box address -> total number of capture slots naming it. + /// Box address -> packed edge record: the number of capture slots naming + /// it (`EDGE_COUNT_MASK`) plus, once the frame that minted the cell has + /// released it (#10464), `FRAME_RELEASED` and the cell-kind tag. Keeping + /// the release state in the record the capture edges already maintain + /// makes a frame exit one probe, and lets the final edge's removal publish + /// the cell without a second table. static BOX_CAPTURE_COUNTS: RefCell> = RefCell::new(crate::fast_hash::new_ptr_hash_map()); } +const FRAME_RELEASED: usize = 1 << 63; +const FRAME_RELEASE_TAG_SHIFT: u32 = 60; +const FRAME_RELEASE_TAG_MASK: usize = 0b11 << FRAME_RELEASE_TAG_SHIFT; +const EDGE_COUNT_MASK: usize = (1 << FRAME_RELEASE_TAG_SHIFT) - 1; + fn increment_cell_capture_count(cell: usize, amount: usize) { BOX_CAPTURE_COUNTS.with(|counts| { let mut counts = counts.borrow_mut(); - let count = counts.entry(cell).or_default(); - *count = count + let record = counts.entry(cell).or_default(); + let count = (*record & EDGE_COUNT_MASK) .checked_add(amount) + .filter(|count| *count <= EDGE_COUNT_MASK) .expect("box capture count overflow"); + *record = (*record & !EDGE_COUNT_MASK) | count; }); } fn decrement_cell_capture_count(cell: usize, amount: usize) { + // `Some(record)` when the final edge disappeared. let reached_zero = BOX_CAPTURE_COUNTS.with(|counts| { let mut counts = counts.borrow_mut(); - let Some(count) = counts.get_mut(&cell) else { - return false; - }; - debug_assert!(*count >= amount); - *count -= amount; - if *count == 0 { - counts.remove(&cell); - true + let record = counts.get_mut(&cell)?; + debug_assert!(*record & EDGE_COUNT_MASK >= amount); + *record -= amount; + if *record & EDGE_COUNT_MASK == 0 { + counts.remove(&cell) } else { - false + None } }); - if reached_zero { - crate::r#box::box_capture_count_reached_zero(cell); + match reached_zero { + Some(record) if record & FRAME_RELEASED != 0 => crate::r#box::publish_frame_released_cell( + cell, + (record & FRAME_RELEASE_TAG_MASK) >> FRAME_RELEASE_TAG_SHIFT, + ), + Some(_) => crate::r#box::box_capture_count_reached_zero(cell), + None => {} } } pub(crate) fn box_capture_count(cell: usize) -> usize { BOX_CAPTURE_COUNTS - .with(|counts| counts.borrow().get(&cell).copied()) - .unwrap_or(0) + .with(|counts| { + let counts = counts.borrow(); + if counts.is_empty() { + None + } else { + counts.get(&cell).copied() + } + }) + .map_or(0, |record| record & EDGE_COUNT_MASK) +} + +/// Outcome of [`note_frame_released_cell`]. +pub(crate) enum FrameRelease { + /// No closure edge names the cell: the frame was its only holder. + Uncaptured, + /// Captured; the final edge's removal now publishes it. + Deferred, + /// Captured and already released by its frame. + AlreadyReleased, +} + +/// #10464: the frame that minted `cell` (of kind `tag`, 1..=3) can no longer +/// name it. A captured cell is marked so its last capture edge publishes it. +pub(crate) fn note_frame_released_cell(cell: usize, tag: usize) -> FrameRelease { + debug_assert!((1..=3).contains(&tag)); + BOX_CAPTURE_COUNTS.with(|counts| { + let mut counts = counts.borrow_mut(); + if counts.is_empty() { + return FrameRelease::Uncaptured; + } + match counts.get_mut(&cell) { + None => FrameRelease::Uncaptured, + Some(record) if *record & FRAME_RELEASED != 0 => FrameRelease::AlreadyReleased, + Some(record) => { + *record |= FRAME_RELEASED | (tag << FRAME_RELEASE_TAG_SHIFT); + FrameRelease::Deferred + } + } + }) +} + +/// Whether `cell` is a frame-released, still-captured JSValue box: during a +/// full trace such a cell is reached only through its live closures. +pub(crate) fn frame_released_js_cell(cell: usize, js_tag: usize) -> bool { + BOX_CAPTURE_COUNTS.with(|counts| { + let counts = counts.borrow(); + !counts.is_empty() + && counts.get(&cell).is_some_and(|record| { + *record & FRAME_RELEASED != 0 + && (*record & FRAME_RELEASE_TAG_MASK) >> FRAME_RELEASE_TAG_SHIFT == js_tag + }) + }) } /// Visit the JSValue payload slots reached through one live closure's exact @@ -72,7 +178,7 @@ pub(crate) fn visit_closure_box_payload_slots_mut(closure: usize, mut visit: imp let Some(cells) = captures.get(&closure) else { return; }; - for &(_, cell) in cells { + for cell in cells.cells() { crate::r#box::visit_pending_captured_js_box_payload_slot(cell, &mut visit); } }); @@ -91,12 +197,9 @@ pub(super) fn set_closure_box_capture( let previous = CLOSURE_BOX_CELLS.with(|captures| { let mut captures = captures.borrow_mut(); let slots = captures.entry(closure).or_default(); - let previous = slots - .iter() - .position(|(slot, _)| *slot == index) - .map(|pos| slots.swap_remove(pos).1); + let previous = slots.take(index); if let Some(cell) = cell { - slots.push((index, cell)); + slots.push(index, cell); } if slots.is_empty() { captures.remove(&closure); @@ -133,7 +236,7 @@ pub(crate) fn clone_closure_box_captures( } copied }); - for (_, cell) in copied { + for cell in copied.cells() { increment_cell_capture_count(cell, 1); } } @@ -152,21 +255,22 @@ pub(crate) fn closure_box_captures_owner_moved(old_owner: usize, new_owner: usiz } pub(crate) fn prune_dead_closure_box_capture_owners(is_dead_closure: &dyn Fn(usize) -> bool) { - let dead_keys = CLOSURE_BOX_CELLS.with(|captures| { - captures - .borrow() - .keys() - .copied() - .filter(|owner| is_dead_closure(*owner)) - .collect::>() + // One pass: dropping a dead owner and collecting its edges together avoids + // a second probe per dead closure. Counts (and any publication they + // trigger) are settled after the table borrow ends. + let mut dead_cells = Vec::new(); + CLOSURE_BOX_CELLS.with(|captures| { + captures.borrow_mut().retain(|owner, slots| { + if is_dead_closure(*owner) { + dead_cells.extend(slots.cells()); + false + } else { + true + } + }); }); - for closure in dead_keys { - let cells = CLOSURE_BOX_CELLS - .with(|captures| captures.borrow_mut().remove(&closure)) - .unwrap_or_default(); - for (_, cell) in cells { - decrement_cell_capture_count(cell, 1); - } + for cell in dead_cells { + decrement_cell_capture_count(cell, 1); } } diff --git a/crates/perry-runtime/src/closure/mod.rs b/crates/perry-runtime/src/closure/mod.rs index a7e4210220..a7f4cb499f 100644 --- a/crates/perry-runtime/src/closure/mod.rs +++ b/crates/perry-runtime/src/closure/mod.rs @@ -73,7 +73,8 @@ pub use unbox::{js_closure_unbox_callee_checked, js_closure_unbox_callee_checked pub(crate) use box_captures::test_clear_closure_box_capture_indexes; pub(crate) use box_captures::{ box_capture_count, clone_closure_box_captures, closure_box_captures_owner_moved, - prune_dead_closure_box_capture_owners, visit_closure_box_payload_slots_mut, + frame_released_js_cell, note_frame_released_cell, prune_dead_closure_box_capture_owners, + visit_closure_box_payload_slots_mut, FrameRelease, }; #[cfg(feature = "wasm-host")] pub(crate) use dynamic_props::register_wasm_funcref_external; diff --git a/scripts/gc_root_dominance_check.py b/scripts/gc_root_dominance_check.py index 015a5f9590..c7d6e82707 100755 --- a/scripts/gc_root_dominance_check.py +++ b/scripts/gc_root_dominance_check.py @@ -522,6 +522,10 @@ def build_cfg(f): # (#7510: letting these two lists drift one-sided printed 358 spurious # violations once the corpus widened). "js_box_release", "js_i32_box_release", "js_bool_box_release", + # #10464 scope-exit release (box/scope_release.rs): the same publish, or a + # TLS pending-map insert for a closure-captured cell. + "js_box_scope_release", "js_i32_box_scope_release", + "js_bool_box_scope_release", "js_write_barrier", # gc/barrier.rs:930 "js_tdz_suppress_begin", "js_tdz_suppress_end", # box.rs:242/248 counter "js_array_note_numeric_write", # array/header.rs:1443 @@ -2742,6 +2746,11 @@ def _probe_boxes_outside_the_gc_heap(): still resume, which is a use-after-release aliasing hazard that this exemption would otherwise silently suppress. The old global quarantine is retained only as a conservative fallback for untracked callers. + + #10464 adds the scope-exit release of an ordinary frame's cells + (`box/scope_release.rs`). It may publish directly only because it is gated + on the cell's closure capture count; a captured cell must take the same + drained-pending path closure death pruning publishes from. """ try: with open("crates/perry-runtime/src/box.rs", @@ -2785,6 +2794,22 @@ def _probe_boxes_outside_the_gc_heap(): "park until its activation reaches zero references. " "Publishing overwrites the terminal value a stray " "resume still writes through.") + scope_path = "crates/perry-runtime/src/box/scope_release.rs" + scope = rust_fn_body(scope_path, "release_scope_cell") + if scope is None: + return (False, "release_scope_cell not found in box/scope_release.rs; " + "the #10464 scope-exit release changed shape") + if "note_frame_released_cell" not in scope \ + or "FrameRelease::Deferred" not in scope: + return (False, "scope-exit release no longer defers a closure-captured " + "cell to closure death") + try: + with open(scope_path, encoding="utf-8", errors="replace") as fh: + scope_src = fh.read() + except OSError: + return (False, f"{scope_path} not readable") + if re.search(r"\bdealloc\s*\(|arena_alloc\w*\s*\(", scope_src): + return (False, "box/scope_release.rs frees or arena-allocates cells") release_ref = rust_fn_body("crates/perry-runtime/src/box.rs", "release_async_box_activation") if release_ref is None or "if new == 0" not in release_ref \ diff --git a/test-files/test_gap_10464_box_cell_release.ts b/test-files/test_gap_10464_box_cell_release.ts new file mode 100644 index 0000000000..58b0f24268 --- /dev/null +++ b/test-files/test_gap_10464_box_cell_release.ts @@ -0,0 +1,305 @@ +// #10464: a `let`/`var` captured by a closure and reassigned lives in a box +// cell. Outside a lowered async state machine those cells were never released, +// so each call leaked a registered GC root plus everything the binding last +// referenced. This test checks both halves of the fix: memory stays bounded +// relative to an equal-allocation control, and cells that escape through +// closures (returned, stored, nested, generators, async) keep their values +// across many collections. + +const maybeGc = (globalThis as any).gc as (() => void) | undefined; + +function churn(rounds: number): number { + let total = 0; + for (let r = 0; r < rounds; r++) { + const junk: Array<{ i: number; s: string; a: number[] }> = []; + for (let i = 0; i < 2000; i++) junk.push({ i, s: "x" + i, a: [i, i + 1] }); + total += junk.length; + } + if (typeof maybeGc === "function") maybeGc(); + return total; +} + +// ---- memory: the issue's repro, compared in-process against its control ---- + +function payload(i: number): number { + let buf: number[] = new Array(512).fill(i); + const swap = () => { + buf = new Array(512).fill(i + 1); + }; + swap(); + return buf.length; +} + +function noBox(i: number): number { + const holder = { buf: new Array(512).fill(i) }; + const swap = () => { + holder.buf = new Array(512).fill(i + 1); + }; + swap(); + return holder.buf.length; +} + +function counterCell(i: number): number { + let x = i; + const bump = () => { + x += 1; + }; + bump(); + return x; +} + +const rssMb = () => process.memoryUsage().rss / 1048576; + +function growthMb(run: (i: number) => number, calls: number): number { + const before = rssMb(); + let acc = 0; + for (let i = 1; i <= calls; i++) acc += run(i); + if (acc <= 0) throw new Error("no work done"); + return rssMb() - before; +} + +// Warm both shapes so allocator and heap high-water marks settle first; the +// control runs before the boxed shape so RSS reuse can only favor the control. +growthMb(noBox, 5000); +growthMb(payload, 5000); +const CALLS = 60000; +const controlGrowth = growthMb(noBox, CALLS); +const payloadGrowth = growthMb(payload, CALLS); +// Leaking every call's 512-element array costs ~5 KB * 60k = ~300 MB. +console.log("payload growth bounded by control:", payloadGrowth < Math.max(controlGrowth, 0) + 96); + +let cellAcc = 0; +for (let i = 1; i <= 200000; i++) cellAcc += counterCell(i); +console.log("counter cells:", cellAcc); + +// ---- escaping closures keep their cells ---- + +function makeCounter(start: number) { + let n = start; + return { + inc: () => ++n, + add: (k: number) => { + n += k; + return n; + }, + get: () => n, + }; +} + +const counters: Array> = []; +for (let i = 0; i < 3000; i++) counters.push(makeCounter(i)); +churn(20); +let counterSum = 0; +for (let i = 0; i < counters.length; i++) { + counters[i].inc(); + counterSum += counters[i].add(i); +} +churn(20); +for (let i = 0; i < counters.length; i += 7) counterSum += counters[i].get(); +console.log("returned counters:", counterSum); + +function makeAdders(): Map number> { + const map = new Map number>(); + for (let i = 0; i < 500; i++) { + let base = i; + map.set("k" + i, (v: number) => (base += v)); + } + return map; +} +const adders = makeAdders(); +churn(15); +let adderSum = 0; +for (const [, fn] of adders) adderSum += fn(1); +churn(15); +for (const [, fn] of adders) adderSum += fn(2); +console.log("per-iteration map closures:", adderSum); + +// A loop whose iterations sometimes capture and sometimes do not: released +// uncaptured cells are reused by the next iteration, captured ones must not be. +function mixedLoop(): number[] { + const kept: Array<() => number> = []; + for (let i = 0; i < 400; i++) { + let value = i * 3; + const touch = () => (value += 1); + if (i % 3 === 0) kept.push(touch); + else touch(); + value += 0; + } + churn(10); + return kept.map((f) => f()).slice(0, 8); +} +console.log("mixed loop:", mixedLoop().join(",")); + +// Nested closure created after the outer frame returned. +function outerFactory() { + let state = "a"; + const middle = () => { + state += "b"; + return () => { + state += "c"; + return state; + }; + }; + return middle; +} +const middles: Array<() => () => string> = []; +for (let i = 0; i < 200; i++) middles.push(outerFactory()); +churn(10); +const inners = middles.map((m) => m()); +churn(10); +console.log("nested after return:", inners[0](), inners[199](), middles[5]()()); + +// Self-referencing payload: the cell's value references its own closure. +function selfCycle(i: number) { + let self: any = null; + const getSelf = () => self; + self = { i, getSelf }; + return getSelf; +} +const cycles: Array<() => any> = []; +for (let i = 0; i < 20000; i++) { + const g = selfCycle(i); + if (i % 1000 === 0) cycles.push(g); +} +churn(15); +console.log("self cycles:", cycles.map((g) => g().getSelf().i).join(",")); + +// Generators capture boxed state across suspensions; abandoned ones are freed. +function* running(limit: number) { + let total = 0; + const add = (v: number) => { + total += v; + }; + for (let i = 1; i <= limit; i++) { + add(i); + yield total; + } + return total; +} +const gens: Array> = []; +for (let i = 0; i < 50; i++) gens.push(running(5)); +let genSum = 0; +for (let step = 0; step < 6; step++) { + churn(3); + for (const g of gens) { + const r = g.next(); + genSum += r.value ?? 0; + } +} +for (let i = 0; i < 5000; i++) running(3).next(); +console.log("generators:", genSum); + +// Class method frames and closures stored on the instance. +class Account { + report: () => string = () => ""; + owner: string; + constructor(owner: string) { + this.owner = owner; + } + open(deposit: number) { + let balance = deposit; + let history = [deposit]; + this.report = () => `${this.owner}:${balance}:${history.length}`; + return (amount: number) => { + balance += amount; + history = history.concat([amount]); + return balance; + }; + } +} +const accounts: Account[] = []; +const deposits: Array<(n: number) => number> = []; +for (let i = 0; i < 300; i++) { + const acct = new Account("u" + i); + accounts.push(acct); + deposits.push(acct.open(i)); +} +churn(10); +for (let i = 0; i < deposits.length; i++) deposits[i](10); +churn(10); +console.log("class frames:", accounts[0].report(), accounts[299].report()); + +// Early returns, throws and finally on frames that own cells. +function exits(mode: number): number { + let seen = mode; + const mark = () => (seen += 100); + try { + if (mode === 0) return mark(); + if (mode === 1) throw new Error("boom" + seen); + mark(); + } finally { + seen += 1; + } + return seen; +} +let exitSum = 0; +for (let i = 0; i < 3000; i++) { + try { + exitSum += exits(i % 3); + } catch (e) { + exitSum += (e as Error).message.length; + } +} +console.log("exits:", exitSum); + +// Recursion: every frame owns its own cell. +function depth(n: number): number { + let local = n; + const bump = () => (local += 1); + if (n > 0) local += depth(n - 1); + bump(); + return local; +} +console.log("recursion:", depth(200)); + +// Parameters captured and reassigned are boxed too. +function paramCell(a: number, b: string) { + const again = () => { + a += 1; + b = b + a; + }; + again(); + return () => b + ":" + a; +} +const paramFns: Array<() => string> = []; +for (let i = 0; i < 1000; i++) paramFns.push(paramCell(i, "p")); +churn(10); +console.log("params:", paramFns[0](), paramFns[999]()); + +// async without await, and an await-ing async closure that captures a cell +// owned by an enclosing synchronous frame which returns before it resumes. +async function noAwait(i: number) { + let y = i; + const f = () => { + y++; + }; + f(); + return y; +} + +function syncOwner(seed: number) { + let shared = seed; + const read = () => shared; + void (async () => { + await null; + churn(2); + shared += 1000; + })(); + shared += 1; + return read; +} + +async function main() { + let asyncSum = 0; + for (let i = 0; i < 2000; i++) asyncSum += await noAwait(i); + console.log("async no await:", asyncSum); + + const readers: Array<() => number> = []; + for (let i = 0; i < 20; i++) readers.push(syncOwner(i)); + churn(10); + await new Promise((resolve) => setTimeout(resolve, 10)); + churn(10); + console.log("async capture of outer cell:", readers.map((r) => r()).join(",")); +} + +main().then(() => console.log("done"));