From 63cf0151e2957bd4f46507fd0e76566837303213 Mon Sep 17 00:00:00 2001 From: Dennis Kobert Date: Sat, 8 Aug 2026 07:33:33 +0000 Subject: [PATCH] Change stack return to standart sret semantics --- .../src/dynamic_executor.rs | 61 +++++++++++- node-graph/libraries/core-types/src/record.rs | 96 ++++++++++++++----- .../libraries/core-types/src/registry.rs | 15 ++- node-graph/node-macro/src/codegen.rs | 52 +++++++--- node-graph/nodes/repeat/src/repeat_nodes.rs | 12 +++ 5 files changed, 191 insertions(+), 45 deletions(-) diff --git a/node-graph/interpreted-executor/src/dynamic_executor.rs b/node-graph/interpreted-executor/src/dynamic_executor.rs index 43a91450e3..6d2260c550 100644 --- a/node-graph/interpreted-executor/src/dynamic_executor.rs +++ b/node-graph/interpreted-executor/src/dynamic_executor.rs @@ -271,23 +271,36 @@ pub struct BorrowTree { nodes: HashMap, /// A hashmap from the document path to the proto node ID. source_map: HashMap, + /// The record-stack reserve, folded from the graph at construction. + stack_need: usize, } impl BorrowTree { pub fn new(proto_network: ProtoNetwork, typing_context: &TypingContext) -> Result { let mut nodes = BorrowTree::default(); + let output = proto_network.output; + let mut deps = HashMap::new(); for (id, node) in proto_network.nodes { + if let ConstructionArgs::Nodes(ids) = &node.construction_args { + deps.insert(id, ids.clone()); + } nodes.push_node(id, node, typing_context)? } + nodes.stack_need = stack_peak(output, &deps, &|id| nodes.frame_bytes(id)); Ok(nodes) } /// Pushes new nodes into the tree and return orphaned nodes pub fn update(&mut self, proto_network: ProtoNetwork, typing_context: &TypingContext) -> Result<(Vec, HashSet), GraphErrors> { + let output = proto_network.output; let mut old_nodes: HashSet<_> = self.nodes.keys().copied().collect(); let mut new_nodes: Vec<_> = Vec::new(); + let mut deps = HashMap::new(); // TODO: Problem: When a passthrough node is connected directly to an export the first input to the passthrough node is not added to the proto network, while the second input is. This means the primary input does not have a type. for (id, node) in proto_network.nodes { + if let ConstructionArgs::Nodes(ids) = &node.construction_args { + deps.insert(id, ids.clone()); + } if !self.nodes.contains_key(&id) { new_nodes.push(node.original_location.path.clone().unwrap_or_default().into()); self.push_node(id, node, typing_context)?; @@ -296,6 +309,7 @@ impl BorrowTree { } old_nodes.remove(&id); } + self.stack_need = stack_peak(output, &deps, &|id| self.frame_bytes(id)); Ok((new_nodes, old_nodes)) } @@ -480,13 +494,37 @@ impl BorrowTree { &self.source_map } - /// The record-stack bound of an evaluation: the sum over all node frames. + /// The record-stack reserve of an evaluation, folded from the graph at + /// construction (see [`stack_peak`]). pub fn stack_need(&self) -> usize { - self.nodes - .values() - .map(|(handle, _)| handle.layout().map_or(0, |layout| layout.frame_bytes())) - .sum() + self.stack_need } + + fn frame_bytes(&self, id: NodeId) -> usize { + self.nodes.get(&id).and_then(|(handle, _)| handle.layout()).map_or(0, |layout| layout.frame_bytes()) + } +} + +/// Peak record-stack bytes for evaluating `output`'s cone. A node holds its +/// inputs' frames until it returns, so its need is its own frame plus every +/// input's frame plus the deepest input's peak. Memoized over shared cones. +fn stack_peak(output: NodeId, deps: &HashMap>, frame_bytes: &dyn Fn(NodeId) -> usize) -> usize { + fn peak(id: NodeId, deps: &HashMap>, frame_bytes: &dyn Fn(NodeId) -> usize, memo: &mut HashMap) -> usize { + if let Some(&cached) = memo.get(&id) { + return cached; + } + let mut held = 0; + let mut deepest = 0; + for &child in deps.get(&id).map_or(&[][..], Vec::as_slice) { + let child_frame = frame_bytes(child); + held += child_frame; + deepest = deepest.max(peak(child, deps, frame_bytes, memo).saturating_sub(child_frame)); + } + let need = frame_bytes(id) + held + deepest; + memo.insert(id, need); + need + } + peak(output, deps, frame_bytes, &mut HashMap::new()) } #[cfg(test)] @@ -505,6 +543,19 @@ mod test { } } + #[test] + fn stack_peak_folds_a_diamond_chain() { + // S3 <- S2 <- S1 <- S0, each consuming the node below on both inputs. + let deps = HashMap::from([ + (NodeId(1), vec![NodeId(0), NodeId(0)]), + (NodeId(2), vec![NodeId(1), NodeId(1)]), + (NodeId(3), vec![NodeId(2), NodeId(2)]), + ]); + let frame = |_: NodeId| 1; + assert_eq!(stack_peak(NodeId(0), &deps, &frame), 1); + assert_eq!(stack_peak(NodeId(3), &deps, &frame), 7); + } + #[test] fn eval_root_builds_the_bare_root_with_the_call_argument_as_vararg_0() { let mut arena = Arena::new(64).unwrap(); diff --git a/node-graph/libraries/core-types/src/record.rs b/node-graph/libraries/core-types/src/record.rs index 4ff57d06d4..6ae3fc693b 100644 --- a/node-graph/libraries/core-types/src/record.rs +++ b/node-graph/libraries/core-types/src/record.rs @@ -361,7 +361,7 @@ pub fn lift_poll<'e, T: Send + Sync + 'static>(poll: GPoll, layout: &Layout, } else { let dst = stack::push(layout.frame_bytes()); let written = unsafe { write_element(dst, element, arena) }; - stack::pop(dst); + stack::truncate_above(dst, layout.frame_bytes()); written.map(|()| RecordValue::spilled(unsafe { Rec::new(dst.cast_const()) })) } }; @@ -612,7 +612,8 @@ pub mod stack { } /// Returns the stack pointer to `frame`, a pointer earlier returned by - /// [`push`] on this thread, releasing everything above it. + /// [`push`] on this thread, releasing it and everything above it. Resets to + /// a checkpoint between repeated evaluations. pub fn pop(frame: *mut u8) { STACK.with(|stack| { let offset = frame as usize - stack.base.get() as usize; @@ -620,6 +621,66 @@ pub mod stack { stack.sp.set(offset); }); } + + /// Releases everything above `frame`'s `bytes`-sized region, keeping the + /// region itself. A node reclaims its inputs' frames on return but leaves + /// its own output readable for its consumer. + pub fn truncate_above(frame: *mut u8, bytes: usize) { + STACK.with(|stack| { + let top = frame as usize - stack.base.get() as usize + bytes.next_multiple_of(8); + debug_assert!(top <= stack.sp.get(), "truncate target must lie within the claimed stack"); + stack.sp.set(top); + }); + } + + /// The current stack pointer, a checkpoint to [`rewind`] to. + pub fn sp() -> usize { + STACK.with(|stack| stack.sp.get()) + } + + /// Resets the stack pointer to an earlier [`sp`] checkpoint, so a loop that + /// evaluates a subtree per iteration reuses the same slots each time. + /// + /// # Safety + /// No `Rec` or `RecordValue` into the region above `mark` may be used after + /// this call. The caller must have copied out everything it still needs. + pub unsafe fn rewind(mark: usize) { + STACK.with(|stack| { + debug_assert!(mark <= stack.sp.get(), "rewind target above the stack pointer"); + stack.sp.set(mark); + }); + } +} + +/// Reclaims the frames an inline node's inputs push. An inline node returns +/// its output by value rather than on the stack, so it has no frame whose +/// `truncate_above` would release its inputs; this guard captures the entry +/// pointer and rewinds to it on drop instead. Inactive (a no-op) for spilled +/// nodes, which release their inputs through their own frame. +pub struct ReclaimGuard { + target: usize, +} + +impl ReclaimGuard { + /// # Safety + /// When `active`, the node must return its output by value, so that no + /// record into the region above the entry pointer is live once its eval + /// returns and the guard rewinds. + pub unsafe fn new(active: bool) -> Self { + Self { + target: if active { stack::sp() } else { usize::MAX }, + } + } +} + +impl Drop for ReclaimGuard { + fn drop(&mut self) { + if self.target != usize::MAX { + // SAFETY: an inline node returns its output by value, so no record into + // the reclaimed region is live once its eval returns. + unsafe { stack::rewind(self.target) }; + } + } } /// Field-by-field carry from `from`'s layout into `to`'s, computed at @@ -664,7 +725,7 @@ pub unsafe fn write_field(dst: *mut u8, offset: usize, value: T) { pub unsafe fn lift_poll_into<'e, T: Send + Sync + 'static>(poll: GPoll, dst: *mut u8, frame_bytes: usize, arena: &'e crate::arena::Arena) -> GPoll> { let release = || { if frame_bytes != 0 { - stack::pop(dst); + stack::truncate_above(dst, frame_bytes); } }; let build = |element: T| { @@ -750,27 +811,6 @@ pub unsafe fn read_element(rec: Rec) -> T { unsafe { borrow_element::(rec) }.clone() } -/// Borrows a record's element for the rest of the evaluation. Parked elements -/// borrow the arena, inline elements borrow their record value in place, and -/// a byte-carried spilled element copies into the arena first: its stack -/// region dies with the next push, and a borrow taken before the consumer's -/// own frame push always has one coming. `None` reports arena exhaustion. -/// -/// # Safety -/// The record's element must be a `T` in the form [`element_parked`] picks, -/// and for inline layouts the record value must outlive the borrow. -pub unsafe fn borrow_or_park<'e, T: Send + Sync + 'static>(rec: Rec, layout: &Layout, arena: &'e crate::arena::Arena) -> Option<&'e T> { - if element_parked::() { - return Some(unsafe { rec.element::<&T>() }); - } - if layout.is_inline() { - return Some(unsafe { &*rec.ptr().cast::() }); - } - // A bitwise read duplicates soundly: byte-carried elements have no drop - // glue. - let value = unsafe { rec.ptr().cast::().read() }; - arena.alloc(value).map(|(parked, _)| parked) -} /// # Safety /// `dst` must be fresh element storage of a record whose element is `T`. @@ -867,6 +907,7 @@ impl SourcePlan { pub struct RecordSource { edge: N, plan: Option, + union: Layout, } impl RecordSource { @@ -874,6 +915,7 @@ impl RecordSource { Self { edge, plan: SourcePlan::new(source, union), + union: union.clone(), } } } @@ -985,7 +1027,7 @@ impl OwnedRecord { }; let written = self.write_into(layout, dst, arena); if layout.frame_bytes() != 0 { - stack::pop(dst); + stack::truncate_above(dst, layout.frame_bytes()); value = RecordValue::spilled(unsafe { Rec::new(dst.cast_const()) }); } written.map(|()| value) @@ -1094,6 +1136,10 @@ where } } } + + fn layout(&self) -> Option<&Layout> { + Some(&self.union) + } } #[cfg(test)] diff --git a/node-graph/libraries/core-types/src/registry.rs b/node-graph/libraries/core-types/src/registry.rs index 35418b4238..07f5ef2e58 100644 --- a/node-graph/libraries/core-types/src/registry.rs +++ b/node-graph/libraries/core-types/src/registry.rs @@ -147,9 +147,22 @@ where type Output = N::Output; fn eval(&self, input: &Input) -> crate::gpoll::GPoll { + // Every node advances the record stack by exactly its own frame: it keeps + // its output and reclaims its inputs. A mismatch is a leaked or + // over-released frame. + #[cfg(debug_assertions)] + let sp_before = crate::record::stack::sp(); // SAFETY: `own` keeps the payload alive for `self`'s lifetime and Arc // payloads are address stable. - unsafe { self.ptr.as_ref() }.eval(input) + let result = unsafe { self.ptr.as_ref() }.eval(input); + #[cfg(debug_assertions)] + debug_assert_eq!( + crate::record::stack::sp(), + sp_before + self.layout().map_or(0, |layout| layout.frame_bytes()), + "{} left the record stack misaligned", + std::any::type_name::(), + ); + result } fn extent(&self, input: &Input) -> crate::gpoll::GPoll { diff --git a/node-graph/node-macro/src/codegen.rs b/node-graph/node-macro/src/codegen.rs index 5af9852911..51e8793829 100644 --- a/node-graph/node-macro/src/codegen.rs +++ b/node-graph/node-macro/src/codegen.rs @@ -1208,9 +1208,8 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn let #name: #ty = unsafe { #core_types::record::read_element(#rec_local) }; } } - // A borrow taken before the node's own frame push parks a - // byte-carried spilled element into the arena; parked and inline - // elements borrow directly. + // The lend input's frame survives on the record stack until this + // node's frame is reclaimed, so the borrow stays valid in place. ParsedFieldType::Regular(RegularParsedField { ty, lend: Some(_), .. }) if flip => { let slot = format_ident!("__in_{index}"); let record_local = format_ident!("__record_{index}"); @@ -1219,14 +1218,7 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn Ok(value) => value, Err(interrupt) => return interrupt.into(), }; - let Some(#name) = (unsafe { - #core_types::record::borrow_or_park::<#ty>(self.#slot.rec(&#record_local), &self.#slot, #core_types::context::ExtractArena::arena(__input)) - }) else { - return #core_types::gpoll::GPoll::Error(::std::boxed::Box::new(#core_types::gpoll::GraphError { - kind: #core_types::gpoll::ErrorKind::ArenaExhausted, - trace: ::std::vec::Vec::new(), - })); - }; + let #name = unsafe { #core_types::record::borrow_element::<#ty>(self.#slot.rec(&#record_local)) }; } } ParsedFieldType::Regular(RegularParsedField { ty, .. }) if flip => { @@ -1650,7 +1642,7 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn #element_store #(#attr_stores)* if self.__frame_bytes != 0 { - #core_types::record::stack::pop(__dst); + #core_types::record::stack::truncate_above(__dst, self.__frame_bytes); __value = #core_types::record::RecordValue::spilled(unsafe { #core_types::record::Rec::new(__dst.cast_const()) }); } __cell.finish(__value) @@ -1664,7 +1656,12 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn __cell.merge(unsafe { #core_types::record::lift_poll_into(#kernel_call, __dst, self.__frame_bytes, #core_types::context::ExtractArena::arena(__input)) }) }, None => quote! { - __cell.merge(#core_types::record::lift_poll(#kernel_call, &self.__layout, #core_types::context::ExtractArena::arena(__input))) + let mut __scratch = #core_types::record::RecordValue::zeroed(); + let __dst = match self.__frame_bytes { + 0 => __scratch.as_mut_ptr(), + __bytes => #core_types::record::stack::push(__bytes), + }; + __cell.merge(unsafe { #core_types::record::lift_poll_into(#kernel_call, __dst, self.__frame_bytes, #core_types::context::ExtractArena::arena(__input)) }) }, }; } @@ -1688,8 +1685,13 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn }) }, None => quote! { + let mut __scratch = #core_types::record::RecordValue::zeroed(); + let __dst = match self.__frame_bytes { + 0 => __scratch.as_mut_ptr(), + __bytes => #core_types::record::stack::push(__bytes), + }; let __kernel_value = #kernel_value; - __cell.merge(#core_types::record::lift_poll(#core_types::gpoll::GPoll::Final(__kernel_value), &self.__layout, #core_types::context::ExtractArena::arena(__input))) + __cell.merge(unsafe { #core_types::record::lift_poll_into(#core_types::gpoll::GPoll::Final(__kernel_value), __dst, self.__frame_bytes, #core_types::context::ExtractArena::arena(__input)) }) }, } }); @@ -1915,6 +1917,27 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn } }); + // An inline node only leaks if an input spilled a frame. Flip nodes store + // each input's layout, so gate precisely; inline record-io nodes are rare + // (attributes usually spill the output) and their value-input layouts are + // not all stored, so guard them whenever inline. + let reclaim_active = match flip { + true => { + let spilled_inputs = (0..regular_fields.len()).map(|index| { + let slot = format_ident!("__in_{index}"); + quote!(self.#slot.frame_bytes() != 0) + }); + quote!(self.__frame_bytes == 0 && (false #(|| #spilled_inputs)*)) + } + false => quote!(self.__frame_bytes == 0), + }; + let reclaim_guard = (flip || record.is_some()).then(|| { + quote! { + // SAFETY: an inline node returns its output by value, so nothing above the entry pointer is live when the guard rewinds. + let __reclaim_guard = unsafe { #core_types::record::ReclaimGuard::new(#reclaim_active) }; + } + }); + let top_level = quote! { #cfg #[automatically_derived] @@ -1932,6 +1955,7 @@ pub(crate) fn generate_node_impl(crate_ident: &CrateIdent, parsed: &ParsedNodeFn fn eval(&self, __input: &#ctx_ident) -> #core_types::gpoll::GPoll { let __cell = #cell_constructor; + #reclaim_guard #(#eval_values)* #(#clamps)* #eval_tail diff --git a/node-graph/nodes/repeat/src/repeat_nodes.rs b/node-graph/nodes/repeat/src/repeat_nodes.rs index a277dc53d7..35ca3835c0 100644 --- a/node-graph/nodes/repeat/src/repeat_nodes.rs +++ b/node-graph/nodes/repeat/src/repeat_nodes.rs @@ -35,11 +35,14 @@ fn repeat + Default + Send + Clone + 'static>( for index in 0..count { let index = if reverse { count - index - 1 } else { index }; + let mark = core_types::record::stack::sp(); let generated_content = content.eval(&ctx.promoted(&spilled, index))?; for generated_row in generated_content.into_iter() { result_list.push(generated_row); } + // SAFETY: generated_content is an owned list fully moved into result_list, so no record borrow into this iteration's frames remains. + unsafe { core_types::record::stack::rewind(mark) }; } Ok(result_list) @@ -76,6 +79,7 @@ pub fn repeat_array + Default + Send + Clone + 'static>( let translation = index as f64 * direction / total; let transform = DAffine2::from_angle(angle) * DAffine2::from_translation(translation); + let mark = core_types::record::stack::sp(); let generated_content = content.eval(&ctx.promoted(&spilled, index as u64))?; for row_index in 0..generated_content.len() { @@ -88,6 +92,8 @@ pub fn repeat_array + Default + Send + Clone + 'static>( result_list.push(row); } + // SAFETY: rows are cloned into result_list and generated_content is owned, so no record borrow into this iteration's frames remains. + unsafe { core_types::record::stack::rewind(mark) }; } Ok(result_list) @@ -120,6 +126,7 @@ fn repeat_radial + Default + Send + Clone + 'static>( let translation = DAffine2::from_translation(radius * DVec2::Y); let transform = angle * translation; + let mark = core_types::record::stack::sp(); let generated_content = content.eval(&ctx.promoted(&spilled, index as u64))?; for row_index in 0..generated_content.len() { @@ -132,6 +139,8 @@ fn repeat_radial + Default + Send + Clone + 'static>( result_list.push(row); } + // SAFETY: rows are cloned into result_list and generated_content is owned, so no record borrow into this iteration's frames remains. + unsafe { core_types::record::stack::rewind(mark) }; } Ok(result_list) @@ -168,12 +177,15 @@ fn repeat_on_points + Default + Send + Clone + 'static>( let transformed_point = transform.transform_point2(point); let scoped = ctx.push_position(transformed_point); + let mark = core_types::record::stack::sp(); let generated_content = content.eval(&scoped.ctx().promoted(&spilled, index as u64))?; for mut generated_row in generated_content.into_iter() { generated_row.attribute_mut_or_insert_default::(ATTR_TRANSFORM).translation = transformed_point; result_list.push(generated_row); } + // SAFETY: generated_content is an owned list fully moved into result_list, so no record borrow into this iteration's frames remains. + unsafe { core_types::record::stack::rewind(mark) }; } }