From 62601479a5f8ab895607b8b4f942162540824fb5 Mon Sep 17 00:00:00 2001 From: Brent Anderson Date: Wed, 25 Mar 2026 21:47:04 -0500 Subject: [PATCH] fix: memory cycle in diverts There is an Rc cycle between a Container, a Divert, back to a Container. This reliably produces a memory leak when running multiple stories in parallel (memory_leak_test covers this case). By caching potential targets and using Weak to break the cycle, we avoid this memory leak case. This shouldn't change the performance profile of a running story, as target references are already lazily loaded. We are just moving from the Divert struct to a shared cache with weak references. --- conformance-tests/tests/memory_leak_test.rs | 86 +++++++++++++++++++++ lib/src/divert.rs | 71 ++++++++++------- 2 files changed, 130 insertions(+), 27 deletions(-) create mode 100644 conformance-tests/tests/memory_leak_test.rs diff --git a/conformance-tests/tests/memory_leak_test.rs b/conformance-tests/tests/memory_leak_test.rs new file mode 100644 index 0000000..16d9183 --- /dev/null +++ b/conformance-tests/tests/memory_leak_test.rs @@ -0,0 +1,86 @@ +use bladeink::story::Story; +use std::alloc::{GlobalAlloc, Layout, System}; +use std::sync::atomic::{AtomicI64, Ordering}; + +mod common; + +static ALLOCATED: AtomicI64 = AtomicI64::new(0); + +struct TrackingAllocator; + +unsafe impl GlobalAlloc for TrackingAllocator { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + let ptr = unsafe { System.alloc(layout) }; + if !ptr.is_null() { + ALLOCATED.fetch_add(layout.size() as i64, Ordering::Relaxed); + } + ptr + } + + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { System.dealloc(ptr, layout) }; + ALLOCATED.fetch_sub(layout.size() as i64, Ordering::Relaxed); + } + + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + let new_ptr = unsafe { System.realloc(ptr, layout, new_size) }; + if !new_ptr.is_null() { + ALLOCATED.fetch_sub(layout.size() as i64, Ordering::Relaxed); + ALLOCATED.fetch_add(new_size as i64, Ordering::Relaxed); + } + new_ptr + } +} + +#[global_allocator] +static GLOBAL: TrackingAllocator = TrackingAllocator; + +fn run_story_to_end(json_string: &str) { + let mut story = Story::new(json_string).expect("failed to create story"); + while story.can_continue() || !story.get_current_choices().is_empty() { + while story.can_continue() { + story.cont().expect("cont failed"); + } + if !story.get_current_choices().is_empty() { + story.choose_choice_index(0).expect("choose failed"); + } + } +} + +#[test] +fn the_intercept_on_loop_test() { + const THREADS: usize = 10; + const STORIES_PER_THREAD: usize = 100; + + let json_string = common::get_json_string("inkfiles/TheIntercept.ink.json").unwrap(); + + let before = ALLOCATED.load(Ordering::SeqCst); + + std::thread::scope(|s| { + let json = &json_string; + let handles: Vec<_> = (0..THREADS) + .map(|_| { + s.spawn(move || { + for _ in 0..STORIES_PER_THREAD { + run_story_to_end(json); + } + }) + }) + .collect(); + + for handle in handles { + handle.join().expect("thread panicked"); + } + }); + + let after = ALLOCATED.load(Ordering::SeqCst); + let leaked = after - before; + + assert_eq!( + leaked, + 0, + "Memory leak detected: {} bytes still allocated after dropping {} stories", + leaked, + THREADS * STORIES_PER_THREAD + ); +} diff --git a/lib/src/divert.rs b/lib/src/divert.rs index e74b9e1..8fd629c 100644 --- a/lib/src/divert.rs +++ b/lib/src/divert.rs @@ -1,16 +1,20 @@ -use std::{cell::RefCell, fmt, rc::Rc}; +use std::{ + cell::RefCell, + fmt, + rc::{Rc, Weak}, +}; use crate::{ container::Container, object::{Object, RTObject}, path::{Component, Path}, - pointer::{self, Pointer}, + pointer::Pointer, push_pop::PushPopType, }; pub struct Divert { obj: Object, - target_pointer: RefCell, + target_cache: RefCell, i32)>>, target_path: RefCell>, pub external_args: usize, pub is_conditional: bool, @@ -37,7 +41,7 @@ impl Divert { stack_push_type, is_external, external_args, - target_pointer: RefCell::new(pointer::NULL.clone()), + target_cache: RefCell::new(None), target_path: RefCell::new(Self::target_path_string(target_path)), variable_divert_name: var_divert_name, } @@ -80,24 +84,32 @@ impl Divert { } pub fn get_target_pointer(self: &Rc) -> Pointer { - let target_pointer_null = self.target_pointer.borrow().is_null(); - if target_pointer_null { - let target_obj = - Object::resolve_path(self.clone(), self.target_path.borrow().as_ref().unwrap()) - .obj - .clone(); - - if self - .target_path - .borrow() - .as_ref() - .unwrap() - .get_last_component() - .unwrap() - .is_index() - { - self.target_pointer.borrow_mut().container = target_obj.get_object().get_parent(); - self.target_pointer.borrow_mut().index = self + if let Some((weak, index)) = self.target_cache.borrow().as_ref() { + if let Some(container) = weak.upgrade() { + return Pointer { + container: Some(container), + index: *index, + }; + } + } + + let target_obj = + Object::resolve_path(self.clone(), self.target_path.borrow().as_ref().unwrap()) + .obj + .clone(); + + let pointer = if self + .target_path + .borrow() + .as_ref() + .unwrap() + .get_last_component() + .unwrap() + .is_index() + { + Pointer { + container: target_obj.get_object().get_parent(), + index: self .target_path .borrow() .as_ref() @@ -105,14 +117,19 @@ impl Divert { .get_last_component() .unwrap() .index - .unwrap() as i32; - } else { - let c = target_obj.into_any().downcast::(); - self.target_pointer.replace(Pointer::start_of(c.unwrap())); + .unwrap() as i32, } + } else { + let c = target_obj.into_any().downcast::().unwrap(); + Pointer::start_of(c) + }; + + if let Some(container) = &pointer.container { + self.target_cache + .replace(Some((Rc::downgrade(container), pointer.index))); } - self.target_pointer.borrow().clone() + pointer } pub fn get_target_path(self: &Rc) -> Option {