Use bumpalo instead of hand-rolled arena implementation (#407)

* Use bumpalo instead of hand-rolled arena implementation

* allow using arena to allocate arbitrary boxes

* Fix compile errors

* Appease clippy

* Allocate the EffectState in the arena

* Fix UI test

* Remove unused field from ScopeInner

packages/sycamore-reactive/Cargo.toml

@@ -11,6 +11,7 @@ repository = "https://github.com/sycamore-rs/sycamore"
 version = "0.8.0-beta.4"
 
 [dependencies]
+bumpalo = { version = "3.9.1", features = ["boxed"] }
 indexmap = "1.8.0"
 serde = { version = "1.0.136", optional = true }
 slotmap = "1.0.6"

packages/sycamore-reactive/src/arena.rs

@@ -2,6 +2,7 @@
 
 use std::cell::UnsafeCell;
 
+use bumpalo::Bump;
 use smallvec::SmallVec;
 
 /// The size of the [`SmallVec`] inline data.
@@ -13,14 +14,18 @@ impl<T> ReallyAny for T {}
 
 #[derive(Default)]
 pub(crate) struct ScopeArena<'a> {
+    pub bump: Bump,
+    // We need to store the raw pointers because otherwise the values won't be dropped.
     inner: UnsafeCell<SmallVec<[*mut (dyn ReallyAny + 'a); SCOPE_ARENA_STACK_SIZE]>>,
 }
 
 impl<'a> ScopeArena<'a> {
-    /// Allocate a value onto the arena. Returns a reference that lasts as long as the arena itself.
-    pub fn alloc<T: 'a>(&'a self, value: T) -> &'a T {
-        let boxed = Box::new(value);
-        let ptr = Box::into_raw(boxed);
+    /// Allocate a value onto the arena. Returns a mutable reference that lasts as long as the arena
+    /// itself.
+    #[allow(clippy::mut_from_ref)] // We return a new reference each time so this is a false-positive.
+    pub fn alloc<T: 'a>(&'a self, value: T) -> &'a mut T {
+        let boxed = bumpalo::boxed::Box::new_in(value, &self.bump);
+        let ptr = bumpalo::boxed::Box::into_raw(boxed);
         unsafe {
             // SAFETY: The only place where self.inner.get() is mutably borrowed is right here.
             // It is impossible to have two alloc() calls on the same ScopeArena at the same time so
@@ -35,7 +40,7 @@ impl<'a> ScopeArena<'a> {
         //   dropped.
         // - The drop code for Scope never reads the allocated value and therefore does not create a
         //   stacked-borrows violation.
-        unsafe { &*ptr }
+        unsafe { &mut *ptr }
     }
 
     /// Cleanup the resources owned by the [`ScopeArena`]. This is automatically called in [`Drop`].
@@ -45,8 +50,8 @@ impl<'a> ScopeArena<'a> {
     /// If a [`ScopeArena`] has already been disposed, calling it again does nothing.
     pub unsafe fn dispose(&self) {
         for &ptr in &*self.inner.get() {
-            // SAFETY: the ptr was allocated in Self::alloc using Box::into_raw.
-            let boxed: Box<dyn ReallyAny> = Box::from_raw(ptr);
+            // SAFETY: the ptr was allocated in Self::alloc using bumpalo::boxed::Box::into_raw.
+            let boxed: bumpalo::boxed::Box<dyn ReallyAny> = bumpalo::boxed::Box::from_raw(ptr);
             // Call the drop code for the allocated value.
             drop(boxed);
         }

packages/sycamore-reactive/src/effect.rs

@@ -69,54 +69,50 @@ impl<'a> EffectState<'a> {
 /// # });
 /// ```
 pub fn create_effect<'a>(cx: Scope<'a>, f: impl FnMut() + 'a) {
-    _create_effect(cx, Box::new(f))
+    let f = cx.alloc(f);
+    _create_effect(cx, f)
 }
 
 /// Internal implementation for `create_effect`. Use dynamic dispatch to reduce code-bloat.
-fn _create_effect<'a>(cx: Scope<'a>, mut f: Box<dyn FnMut() + 'a>) {
-    let effect = Rc::new(RefCell::new(None::<EffectState<'a>>));
+fn _create_effect<'a>(cx: Scope<'a>, f: &'a mut (dyn FnMut() + 'a)) {
+    let effect = &*cx.alloc(RefCell::new(None::<EffectState<'a>>));
     let cb = Rc::new(RefCell::new({
-        let effect = Rc::downgrade(&effect);
         move || {
             EFFECTS.with(|effects| {
                 // Record initial effect stack length to verify that it is the same after.
                 let initial_effect_stack_len = effects.borrow().len();
-                // Upgrade the effect to an Rc now so that it is valid for the rest of the
-                // callback.
-                let effect_ref = effect.upgrade().unwrap();
 
                 // Take effect out.
-                let mut effect = effect_ref.take().unwrap();
-                effect.clear_dependencies();
+                let mut tmp_effect = effect.take().unwrap();
+                tmp_effect.clear_dependencies();
 
                 // Push the effect onto the effect stack so that it is visible by signals.
                 effects
                     .borrow_mut()
-                    .push(unsafe { std::mem::transmute(&mut effect as *mut EffectState<'a>) });
+                    .push(unsafe { std::mem::transmute(&mut tmp_effect as *mut EffectState<'a>) });
                 // Now we can call the user-provided function.
                 f();
                 // Pop the effect from the effect stack.
                 effects.borrow_mut().pop().unwrap();
                 // The raw pointer pushed onto `effects` is dead and can no longer be accessed.
                 // We can now access `effect` directly again.
 
-                // For all the signals collected by the EffectState,
-                // we need to add backlinks from the signal to the effect, so that
-                // updating the signal will trigger the effect.
-                for emitter in &effect.dependencies {
+                // For all the signals collected by the EffectState, we need to add backlinks from
+                // the signal to the effect, so that updating the signal will trigger the effect.
+                for emitter in &tmp_effect.dependencies {
                     // The SignalEmitter might have been destroyed between when the signal was
                     // accessed and now.
                     if let Some(emitter) = emitter.0.upgrade() {
                         // SAFETY: When the effect is destroyed or when the emitter is dropped,
-                        // this link will be destroyed to prevent
-                        // dangling references.
-                        emitter
-                            .subscribe(Rc::downgrade(unsafe { std::mem::transmute(&effect.cb) }));
+                        // this link will be destroyed to prevent dangling references.
+                        emitter.subscribe(Rc::downgrade(unsafe {
+                            std::mem::transmute(&tmp_effect.cb)
+                        }));
                     }
                 }
 
                 // Get the effect state back into the Rc
-                *effect_ref.borrow_mut() = Some(effect);
+                *effect.borrow_mut() = Some(tmp_effect);
 
                 debug_assert_eq!(effects.borrow().len(), initial_effect_stack_len);
             });
@@ -131,9 +127,6 @@ fn _create_effect<'a>(cx: Scope<'a>, mut f: Box<dyn FnMut() + 'a>) {
 
     // Initial callback call to get everything started.
     cb.borrow_mut()();
-
-    // Push Rc to self.effects so that it is not dropped immediately.
-    cx.raw.inner.borrow_mut().effects.push(effect);
 }
 
 /// Creates an effect on signals used inside the effect closure.
@@ -167,9 +160,7 @@ where
         // use-after-free during the clear dependencies phase.
         if let Some(disposer) = disposer.take() {
             // SAFETY: we are not accessing the scope after the effect has been dropped.
-            unsafe {
-                disposer.dispose();
-            }
+            unsafe { disposer.dispose() };
         }
         // Create a new nested scope and save the disposer.
         let new_disposer: Option<Box<ScopeDisposer<'a>>> =
@@ -178,9 +169,6 @@ where
                 // self.create_child_scope(_).
                 f(unsafe { std::mem::transmute(cx) });
             })));
-        // SAFETY: transmute the lifetime. This is safe because disposer is only used within the
-        // effect which is necessarily within the lifetime of self (the Scope).
-        // disposer = unsafe { std::mem::transmute(new_disposer) };
         disposer = new_disposer;
     });
 }

packages/sycamore-reactive/src/lib.rs

@@ -34,8 +34,6 @@ struct InvariantLifetime<'id>(PhantomData<&'id mut &'id ()>);
 /// instead of a [`RefCell`] for every field.
 #[derive(Default)]
 struct ScopeInner<'a> {
-    /// Effect functions created on the [`Scope`].
-    effects: Vec<Rc<RefCell<Option<EffectState<'a>>>>>,
     /// Cleanup functions.
     cleanups: Vec<Box<dyn FnOnce() + 'a>>,
     /// Child scopes.
@@ -96,6 +94,11 @@ impl<'a, 'b: 'a> BoundedScope<'a, 'b> {
             _phantom: PhantomData,
         }
     }
+
+    /// Alias for `self.raw.arena.alloc`.
+    fn alloc<T>(&self, value: T) -> &'a mut T {
+        self.raw.arena.alloc(value)
+    }
 }
 
 /// A type-alias for [`BoundedScope`] where both lifetimes are the same.
@@ -112,7 +115,6 @@ impl<'a> ScopeRaw<'a> {
         // Self::new() is intentionally pub(crate) only to prevent end-users from creating a Scope.
         Self {
             inner: RefCell::new(ScopeInner {
-                effects: Default::default(),
                 cleanups: Default::default(),
                 child_scopes: Default::default(),
                 contexts: None,
@@ -371,8 +373,6 @@ impl<'a> ScopeRaw<'a> {
             // Dispose of cx if it has not already been disposed.
             cx.dispose()
         }
-        // Drop effects.
-        drop(mem::take(&mut inner.effects));
         // Call cleanup functions in an untracked scope.
         untrack(|| {
             for cb in mem::take(&mut inner.cleanups) {