fix: use stack optimization for replacer

packages/core/Cargo.toml

@@ -41,6 +41,7 @@ log = "0.4.17"
 [dev-dependencies]
 tokio = { version = "*", features = ["full"] }
 dioxus = { path = "../dioxus" }
+pretty_assertions = "1.3.0"
 
 [features]
 default = []

packages/core/src/arena.rs

@@ -115,10 +115,14 @@ impl VirtualDom {
                 nodes.iter().for_each(|node| self.drop_scope_inner(node))
             }
             DynamicNode::Placeholder(t) => {
-                self.try_reclaim(t.id.get().unwrap());
+                if let Some(id) = t.id.get() {
+                    self.try_reclaim(id);
+                }
             }
             DynamicNode::Text(t) => {
-                self.try_reclaim(t.id.get().unwrap());
+                if let Some(id) = t.id.get() {
+                    self.try_reclaim(id);
+                }
             }
         });
 

packages/core/src/diff.rs

@@ -155,22 +155,7 @@ impl<'b> VirtualDom {
 
         // Replace components that have different render fns
         if left.render_fn != right.render_fn {
-            let created = self.create_component_node(right_template, right, idx);
+            return self.replace_vcomponent(right_template, right, idx, left);
-            let head = unsafe {
-                self.scopes[left.scope.get().unwrap().0]
-                    .root_node()
-                    .extend_lifetime_ref()
-            };
-            let last = match head {
-                RenderReturn::Sync(Ok(node)) => self.find_last_element(node),
-                _ => todo!(),
-            };
-            self.mutations.push(Mutation::InsertAfter {
-                id: last,
-                m: created,
-            });
-            self.remove_component_node(left, true);
-            return;
         }
 
         // Make sure the new vcomponent has the right scopeid associated to it
@@ -203,6 +188,26 @@ impl<'b> VirtualDom {
         });
     }
 
+    fn replace_vcomponent(
+        &mut self,
+        right_template: &'b VNode<'b>,
+        right: &'b VComponent<'b>,
+        idx: usize,
+        left: &'b VComponent<'b>,
+    ) {
+        let m = self.create_component_node(right_template, right, idx);
+
+        self.remove_component_node(left, true);
+
+        // We want to optimize the replace case to use one less mutation if possible
+        // Since mutations are done in reverse, the last node removed will be the first in the stack
+        // Instead of *just* removing it, we can use the replace mutation
+        match self.mutations.edits.pop().unwrap() {
+            Mutation::Remove { id } => self.mutations.push(Mutation::ReplaceWith { id, m }),
+            at => panic!("Expected remove mutation from remove_node {:#?}", at),
+        };
+    }
+
     /// Lightly diff the two templates, checking only their roots.
     ///
     /// The goal here is to preserve any existing component state that might exist. This is to preserve some React-like
@@ -712,11 +717,21 @@ impl<'b> VirtualDom {
     fn replace(&mut self, left: &'b VNode<'b>, right: impl IntoIterator<Item = &'b VNode<'b>>) {
         let m = self.create_children(right);
 
-        let id = self.find_last_element(left);
+        let pre_edits = self.mutations.edits.len();
-
-        self.mutations.push(Mutation::InsertAfter { id, m });
 
         self.remove_node(left, true);
+
+        // We should always have a remove mutation
+        // Eventually we don't want to generate placeholders, so this might not be true. But it's true today
+        assert!(self.mutations.edits.len() > pre_edits);
+
+        // We want to optimize the replace case to use one less mutation if possible
+        // Since mutations are done in reverse, the last node removed will be the first in the stack
+        // Instead of *just* removing it, we can use the replace mutation
+        match self.mutations.edits.pop().unwrap() {
+            Mutation::Remove { id } => self.mutations.push(Mutation::ReplaceWith { id, m }),
+            _ => panic!("Expected remove mutation from remove_node"),
+        };
     }
 
     fn node_to_placeholder(&mut self, l: &'b [VNode<'b>], r: &'b VPlaceholder) {
@@ -725,24 +740,45 @@ impl<'b> VirtualDom {
 
         r.id.set(Some(placeholder));
 
-        let id = self.find_last_element(&l[0]);
-
         self.mutations
             .push(Mutation::CreatePlaceholder { id: placeholder });
 
-        self.mutations.push(Mutation::InsertAfter { id, m: 1 });
-
         self.remove_nodes(l);
+
+        // We want to optimize the replace case to use one less mutation if possible
+        // Since mutations are done in reverse, the last node removed will be the first in the stack
+        // Instead of *just* removing it, we can use the replace mutation
+        match self.mutations.edits.pop().unwrap() {
+            Mutation::Remove { id } => self.mutations.push(Mutation::ReplaceWith { id, m: 1 }),
+            _ => panic!("Expected remove mutation from remove_node"),
+        };
     }
 
     /// Remove these nodes from the dom
     /// Wont generate mutations for the inner nodes
     fn remove_nodes(&mut self, nodes: &'b [VNode<'b>]) {
-        nodes.iter().for_each(|node| self.remove_node(node, true));
+        nodes
+            .iter()
+            .rev()
+            .for_each(|node| self.remove_node(node, true));
     }
 
     fn remove_node(&mut self, node: &'b VNode<'b>, gen_muts: bool) {
+        // Clean up any attributes that have claimed a static node as dynamic for mount/unmounta
+        // Will not generate mutations!
+        self.reclaim_attributes(node);
+
+        // Remove the nested dynamic nodes
+        // We don't generate mutations for these, as they will be removed by the parent (in the next line)
+        // But we still need to make sure to reclaim them from the arena and drop their hooks, etc
+        self.remove_nested_dyn_nodes(node);
+
         // Clean up the roots, assuming we need to generate mutations for these
+        // This is done last in order to preserve Node ID reclaim order (reclaim in reverse order of claim)
+        self.reclaim_roots(node, gen_muts);
+    }
+
+    fn reclaim_roots(&mut self, node: &VNode, gen_muts: bool) {
         for (idx, _) in node.template.roots.iter().enumerate() {
             if let Some(dy) = node.dynamic_root(idx) {
                 self.remove_dynamic_node(dy, gen_muts);
@@ -754,17 +790,9 @@ impl<'b> VirtualDom {
                 self.reclaim(id);
             }
         }
+    }
 
-        for (idx, dyn_node) in node.dynamic_nodes.iter().enumerate() {
+    fn reclaim_attributes(&mut self, node: &VNode) {
-            // Roots are cleaned up automatically above
-            if node.template.node_paths[idx].len() == 1 {
-                continue;
-            }
-
-            self.remove_dynamic_node(dyn_node, false);
-        }
-
-        // we clean up nodes with dynamic attributes, provided the node is unique and not a root node
         let mut id = None;
         for (idx, attr) in node.dynamic_attrs.iter().enumerate() {
             // We'll clean up the root nodes either way, so don't worry
@@ -784,49 +812,69 @@ impl<'b> VirtualDom {
         }
     }
 
+    fn remove_nested_dyn_nodes(&mut self, node: &VNode) {
+        for (idx, dyn_node) in node.dynamic_nodes.iter().enumerate() {
+            // Roots are cleaned up automatically above
+            if node.template.node_paths[idx].len() == 1 {
+                continue;
+            }
+
+            self.remove_dynamic_node(dyn_node, false);
+        }
+    }
+
     fn remove_dynamic_node(&mut self, node: &DynamicNode, gen_muts: bool) {
         match node {
             Component(comp) => self.remove_component_node(comp, gen_muts),
-            Text(t) => self.remove_text_node(t),
+            Text(t) => self.remove_text_node(t, gen_muts),
-            Placeholder(t) => self.remove_placeholder(t),
+            Placeholder(t) => self.remove_placeholder(t, gen_muts),
             Fragment(nodes) => nodes
                 .iter()
                 .for_each(|node| self.remove_node(node, gen_muts)),
         };
     }
 
-    fn remove_placeholder(&mut self, t: &VPlaceholder) {
+    fn remove_placeholder(&mut self, t: &VPlaceholder, gen_muts: bool) {
         if let Some(id) = t.id.take() {
+            if gen_muts {
+                self.mutations.push(Mutation::Remove { id });
+            }
             self.reclaim(id)
         }
     }
 
-    fn remove_text_node(&mut self, t: &VText) {
+    fn remove_text_node(&mut self, t: &VText, gen_muts: bool) {
         if let Some(id) = t.id.take() {
+            if gen_muts {
+                self.mutations.push(Mutation::Remove { id });
+            }
             self.reclaim(id)
         }
     }
 
     fn remove_component_node(&mut self, comp: &VComponent, gen_muts: bool) {
-        if let Some(scope) = comp.scope.take() {
+        let scope = comp.scope.take().unwrap();
-            match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } {
-                RenderReturn::Sync(Ok(t)) => self.remove_node(t, gen_muts),
-                _ => todo!("cannot handle nonstandard nodes"),
-            };
 
-            let props = self.scopes[scope.0].props.take();
+        match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } {
+            RenderReturn::Sync(Ok(t)) => {
+                println!("Removing component node sync {:?}", gen_muts);
+                self.remove_node(t, gen_muts)
+            }
+            _ => todo!("cannot handle nonstandard nodes"),
+        };
 
-            self.dirty_scopes.remove(&DirtyScope {
+        let props = self.scopes[scope.0].props.take();
-                height: self.scopes[scope.0].height,
-                id: scope,
-            });
 
-            *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) };
+        self.dirty_scopes.remove(&DirtyScope {
+            height: self.scopes[scope.0].height,
+            id: scope,
+        });
 
-            // make sure to wipe any of its props and listeners
+        *comp.props.borrow_mut() = unsafe { std::mem::transmute(props) };
-            self.ensure_drop_safety(scope);
+
-            self.scopes.remove(scope.0);
+        // make sure to wipe any of its props and listeners
-        }
+        self.ensure_drop_safety(scope);
+        self.scopes.remove(scope.0);
     }
 
     fn find_first_element(&self, node: &'b VNode<'b>) -> ElementId {

packages/core/tests/attr_cleanup.rs

@@ -46,7 +46,7 @@ fn attrs_cycle() {
     assert_eq!(
         dom.render_immediate().santize().edits,
         [
-            LoadTemplate { name: "template", index: 0, id: ElementId(3) },
+            LoadTemplate { name: "template", index: 0, id: ElementId(1) },
             ReplaceWith { id: ElementId(2), m: 1 }
         ]
     );
@@ -56,10 +56,10 @@ fn attrs_cycle() {
         dom.render_immediate().santize().edits,
         [
             LoadTemplate { name: "template", index: 0, id: ElementId(2) },
-            AssignId { path: &[0], id: ElementId(1) },
+            AssignId { path: &[0], id: ElementId(3) },
-            SetAttribute { name: "class", value: "3", id: ElementId(1), ns: None },
+            SetAttribute { name: "class", value: "3", id: ElementId(3), ns: None },
-            SetAttribute { name: "id", value: "3", id: ElementId(1), ns: None },
+            SetAttribute { name: "id", value: "3", id: ElementId(3), ns: None },
-            ReplaceWith { id: ElementId(3), m: 1 }
+            ReplaceWith { id: ElementId(1), m: 1 }
         ]
     );
 

packages/core/tests/diff_unkeyed_list.rs

@@ -1,5 +1,6 @@
 use dioxus::core::{ElementId, Mutation::*};
 use dioxus::prelude::*;
+use pretty_assertions::assert_eq;
 
 #[test]
 fn list_creates_one_by_one() {
@@ -125,7 +126,7 @@ fn removes_one_by_one() {
     assert_eq!(
         dom.render_immediate().santize().edits,
         [
-            CreatePlaceholder { id: ElementId(3) },
+            CreatePlaceholder { id: ElementId(4) },
             ReplaceWith { id: ElementId(2), m: 1 }
         ]
     );
@@ -137,12 +138,12 @@ fn removes_one_by_one() {
         dom.render_immediate().santize().edits,
         [
             LoadTemplate { name: "template", index: 0, id: ElementId(2) },
-            HydrateText { path: &[0], value: "0", id: ElementId(4) },
+            HydrateText { path: &[0], value: "0", id: ElementId(3) },
             LoadTemplate { name: "template", index: 0, id: ElementId(5) },
             HydrateText { path: &[0], value: "1", id: ElementId(6) },
             LoadTemplate { name: "template", index: 0, id: ElementId(7) },
             HydrateText { path: &[0], value: "2", id: ElementId(8) },
-            ReplaceWith { id: ElementId(3), m: 3 }
+            ReplaceWith { id: ElementId(4), m: 3 }
         ]
     );
 }
@@ -264,9 +265,9 @@ fn removes_one_by_one_multiroot() {
     assert_eq!(
         dom.render_immediate().santize().edits,
         [
-            Remove { id: ElementId(4) },
+            CreatePlaceholder { id: ElementId(8) },
-            CreatePlaceholder { id: ElementId(5) },
+            Remove { id: ElementId(2) },
-            ReplaceWith { id: ElementId(2), m: 1 }
+            ReplaceWith { id: ElementId(4), m: 1 }
         ]
     );
 }
@@ -357,11 +358,11 @@ fn remove_many() {
     assert_eq!(
         edits.edits,
         [
+            CreatePlaceholder { id: ElementId(11,) },
             Remove { id: ElementId(9,) },
             Remove { id: ElementId(7,) },
             Remove { id: ElementId(5,) },
             Remove { id: ElementId(1,) },
-            CreatePlaceholder { id: ElementId(3,) },
             ReplaceWith { id: ElementId(2,), m: 1 },
         ]
     );
@@ -372,8 +373,8 @@ fn remove_many() {
         edits.edits,
         [
             LoadTemplate { name: "template", index: 0, id: ElementId(2,) },
-            HydrateText { path: &[0,], value: "hello 0", id: ElementId(1,) },
+            HydrateText { path: &[0,], value: "hello 0", id: ElementId(3,) },
-            ReplaceWith { id: ElementId(3,), m: 1 },
+            ReplaceWith { id: ElementId(11,), m: 1 },
         ]
     )
 }