From bffb2644a377d5365b524cd1b676cb79478c94f1 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Fri, 25 Nov 2022 02:12:29 -0500 Subject: [PATCH] fix: node reclaimation --- packages/core/src/arena.rs | 2 + packages/core/src/diff.rs | 177 +++++++++++------------ packages/core/src/garbage.rs | 1 + packages/core/src/nodes.rs | 9 ++ packages/core/tests/diff_element.rs | 10 +- packages/core/tests/diff_unkeyed_list.rs | 55 ++++--- 6 files changed, 129 insertions(+), 125 deletions(-) diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index c4fbe2b23..98fe478e6 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -31,6 +31,8 @@ impl VirtualDom { path, }); + println!("Claiming {}", id); + ElementId(id) } diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 541167486..21091f961 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -227,17 +227,16 @@ impl<'b: 'static> VirtualDom { } fn replace_placeholder_with_nodes(&mut self, l: &'b Cell, r: &'b [VNode<'b>]) { - let created = self.create_children(r); - self.mutations.push(Mutation::ReplaceWith { - id: l.get(), - m: created, - }) + let m = self.create_children(r); + let id = l.get(); + self.mutations.push(Mutation::ReplaceWith { id, m }); + self.reclaim(id); } fn replace_nodes_with_placeholder(&mut self, l: &'b [VNode<'b>], r: &'b Cell) { // Remove the old nodes, except for onea - self.remove_nodes(&l[1..]); let first = self.replace_inner(&l[0]); + self.remove_nodes(&l[1..]); let placeholder = self.next_element(&l[0], &[]); r.set(placeholder); @@ -252,24 +251,20 @@ impl<'b: 'static> VirtualDom { // Remove all the top-level nodes, returning the firstmost root ElementId fn replace_inner(&mut self, node: &'b VNode<'b>) -> ElementId { - let id = match &node.template.roots[0] { - TemplateNode::Text(_) | TemplateNode::Element { .. } => node.root_ids[0].get(), - TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { - match &node.dynamic_nodes[*id] { - Text(t) => t.id.get(), - Fragment(VFragment::Empty(e)) => e.get(), - Fragment(VFragment::NonEmpty(nodes)) => { - let id = self.replace_inner(&nodes[0]); - self.remove_nodes(&nodes[1..]); - id - } - Component(comp) => { - let scope = comp.scope.get().unwrap(); - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.replace_inner(t), - _ => todo!("cannot handle nonstandard nodes"), - } - } + let id = match node.dynamic_root(0) { + None => node.root_ids[0].get(), + Some(Text(t)) => t.id.get(), + Some(Fragment(VFragment::Empty(e))) => e.get(), + Some(Fragment(VFragment::NonEmpty(nodes))) => { + let id = self.replace_inner(&nodes[0]); + self.remove_nodes(&nodes[1..]); + id + } + Some(Component(comp)) => { + let scope = comp.scope.get().unwrap(); + match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { + RenderReturn::Sync(Ok(t)) => self.replace_inner(t), + _ => todo!("cannot handle nonstandard nodes"), } } }; @@ -305,40 +300,43 @@ impl<'b: 'static> VirtualDom { } }; } + + // we also need to clean up dynamic attribute roots + // let last_node = None; + // for attr in node.dynamic_attrs { + // match last_node { + // Some(node) => todo!(), + // None => todo!(), + // } + // } } fn remove_root_node(&mut self, node: &'b VNode<'b>, idx: usize) { - let root = node.template.roots[idx]; - match root { - TemplateNode::Element { .. } | TemplateNode::Text(_) => { + match node.dynamic_root(idx) { + Some(Text(i)) => { + let id = i.id.get(); + self.mutations.push(Mutation::Remove { id }); + self.reclaim(id); + } + Some(Fragment(VFragment::Empty(e))) => { + let id = e.get(); + self.mutations.push(Mutation::Remove { id }); + self.reclaim(id); + } + Some(Fragment(VFragment::NonEmpty(nodes))) => self.remove_nodes(nodes), + Some(Component(comp)) => { + let scope = comp.scope.get().unwrap(); + match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { + RenderReturn::Sync(Ok(t)) => self.remove_node(t), + _ => todo!("cannot handle nonstandard nodes"), + }; + } + None => { let id = node.root_ids[idx].get(); self.mutations.push(Mutation::Remove { id }); self.reclaim(id); } - - TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { - match &node.dynamic_nodes[id] { - Text(i) => { - let id = i.id.get(); - self.mutations.push(Mutation::Remove { id }); - self.reclaim(id); - } - Fragment(VFragment::Empty(e)) => { - let id = e.get(); - self.mutations.push(Mutation::Remove { id }); - self.reclaim(id); - } - Fragment(VFragment::NonEmpty(nodes)) => self.remove_nodes(nodes), - Component(comp) => { - let scope = comp.scope.get().unwrap(); - match unsafe { self.scopes[scope.0].root_node().extend_lifetime_ref() } { - RenderReturn::Sync(Ok(t)) => self.remove_node(t), - _ => todo!("cannot handle nonstandard nodes"), - }; - } - }; - } - } + }; } fn diff_non_empty_fragment(&mut self, new: &'b [VNode<'b>], old: &'b [VNode<'b>]) { @@ -729,21 +727,26 @@ impl<'b: 'static> VirtualDom { } fn remove_node(&mut self, node: &'b VNode<'b>) { - for (idx, root) in node.template.roots.iter().enumerate() { - let id = match root { - TemplateNode::Text(_) | TemplateNode::Element { .. } => node.root_ids[idx].get(), - TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { - match &node.dynamic_nodes[*id] { - Text(t) => t.id.get(), - Fragment(VFragment::Empty(t)) => t.get(), - Fragment(VFragment::NonEmpty(t)) => return self.remove_nodes(t), - Component(comp) => return self.remove_component(comp.scope.get().unwrap()), - } - } + for (idx, _) in node.template.roots.iter().enumerate() { + let id = match node.dynamic_root(idx) { + Some(Text(t)) => t.id.get(), + Some(Fragment(VFragment::Empty(t))) => t.get(), + Some(Fragment(VFragment::NonEmpty(t))) => return self.remove_nodes(t), + Some(Component(comp)) => return self.remove_component(comp.scope.get().unwrap()), + None => node.root_ids[idx].get(), }; self.mutations.push(Mutation::Remove { id }) } + + self.clean_up_node(node); + + for root in node.root_ids { + let id = root.get(); + if id.0 != 0 { + self.reclaim(id); + } + } } fn remove_component(&mut self, scope_id: ScopeId) { @@ -782,42 +785,32 @@ impl<'b: 'static> VirtualDom { } fn find_first_element(&self, node: &VNode) -> ElementId { - match &node.template.roots[0] { - TemplateNode::Element { .. } | TemplateNode::Text(_) => node.root_ids[0].get(), - TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { - match &node.dynamic_nodes[*id] { - Text(t) => t.id.get(), - Fragment(VFragment::NonEmpty(t)) => self.find_first_element(&t[0]), - Fragment(VFragment::Empty(t)) => t.get(), - Component(comp) => { - let scope = comp.scope.get().unwrap(); - match self.scopes[scope.0].root_node() { - RenderReturn::Sync(Ok(t)) => self.find_first_element(t), - _ => todo!("cannot handle nonstandard nodes"), - } - } + match node.dynamic_root(0) { + None => node.root_ids[0].get(), + Some(Text(t)) => t.id.get(), + Some(Fragment(VFragment::NonEmpty(t))) => self.find_first_element(&t[0]), + Some(Fragment(VFragment::Empty(t))) => t.get(), + Some(Component(comp)) => { + let scope = comp.scope.get().unwrap(); + match self.scopes[scope.0].root_node() { + RenderReturn::Sync(Ok(t)) => self.find_first_element(t), + _ => todo!("cannot handle nonstandard nodes"), } } } } fn find_last_element(&self, node: &VNode) -> ElementId { - match node.template.roots.last().unwrap() { - TemplateNode::Element { .. } | TemplateNode::Text(_) => { - node.root_ids.last().unwrap().get() - } - TemplateNode::DynamicText(id) | TemplateNode::Dynamic(id) => { - match &node.dynamic_nodes[*id] { - Text(t) => t.id.get(), - Fragment(VFragment::NonEmpty(t)) => self.find_last_element(t.last().unwrap()), - Fragment(VFragment::Empty(t)) => t.get(), - Component(comp) => { - let scope = comp.scope.get().unwrap(); - match self.scopes[scope.0].root_node() { - RenderReturn::Sync(Ok(t)) => self.find_last_element(t), - _ => todo!("cannot handle nonstandard nodes"), - } - } + match node.dynamic_root(node.template.roots.len() - 1) { + None => node.root_ids.last().unwrap().get(), + Some(Text(t)) => t.id.get(), + Some(Fragment(VFragment::NonEmpty(t))) => self.find_last_element(t.last().unwrap()), + Some(Fragment(VFragment::Empty(t))) => t.get(), + Some(Component(comp)) => { + let scope = comp.scope.get().unwrap(); + match self.scopes[scope.0].root_node() { + RenderReturn::Sync(Ok(t)) => self.find_last_element(t), + _ => todo!("cannot handle nonstandard nodes"), } } } diff --git a/packages/core/src/garbage.rs b/packages/core/src/garbage.rs index 2d9e1d6d7..66a54c2d9 100644 --- a/packages/core/src/garbage.rs +++ b/packages/core/src/garbage.rs @@ -15,6 +15,7 @@ impl<'b> VirtualDom { pub fn reclaim(&mut self, el: ElementId) { assert_ne!(el, ElementId(0)); + println!("reclaiming {}", el.0); self.elements.remove(el.0); } diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 1719388ad..1750d4276 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -54,6 +54,15 @@ impl<'a> VNode<'a> { }, }) } + + pub fn dynamic_root(&self, idx: usize) -> Option<&'a DynamicNode<'a>> { + match &self.template.roots[idx] { + TemplateNode::Element { .. } | TemplateNode::Text(_) => None, + TemplateNode::Dynamic(id) | TemplateNode::DynamicText(id) => { + Some(&self.dynamic_nodes[*id]) + } + } + } } #[derive(Debug, Clone, Copy)] diff --git a/packages/core/tests/diff_element.rs b/packages/core/tests/diff_element.rs index f4e5f108d..5dff0fbc8 100644 --- a/packages/core/tests/diff_element.rs +++ b/packages/core/tests/diff_element.rs @@ -59,7 +59,7 @@ fn element_swap() { assert_eq!( vdom.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 }, ] ); @@ -68,8 +68,8 @@ fn element_swap() { assert_eq!( vdom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(4,) }, - ReplaceWith { id: ElementId(3,), m: 1 }, + LoadTemplate { name: "template", index: 0, id: ElementId(2,) }, + ReplaceWith { id: ElementId(1,), m: 1 }, ] ); @@ -77,8 +77,8 @@ fn element_swap() { assert_eq!( vdom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(5,) }, - ReplaceWith { id: ElementId(4,), m: 1 }, + LoadTemplate { name: "template", index: 0, id: ElementId(1,) }, + ReplaceWith { id: ElementId(2,), m: 1 }, ] ); } diff --git a/packages/core/tests/diff_unkeyed_list.rs b/packages/core/tests/diff_unkeyed_list.rs index de7252f4e..6d8f0a236 100644 --- a/packages/core/tests/diff_unkeyed_list.rs +++ b/packages/core/tests/diff_unkeyed_list.rs @@ -41,8 +41,8 @@ fn list_creates_one_by_one() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(5,) }, - HydrateText { path: &[0], value: "1", id: ElementId(6,) }, + LoadTemplate { name: "template", index: 0, id: ElementId(2,) }, + HydrateText { path: &[0], value: "1", id: ElementId(5,) }, InsertAfter { id: ElementId(3,), m: 1 }, ] ); @@ -52,9 +52,9 @@ fn list_creates_one_by_one() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(7,) }, - HydrateText { path: &[0], value: "2", id: ElementId(8,) }, - InsertAfter { id: ElementId(5,), m: 1 }, + LoadTemplate { name: "template", index: 0, id: ElementId(6,) }, + HydrateText { path: &[0], value: "2", id: ElementId(7,) }, + InsertAfter { id: ElementId(2,), m: 1 }, ] ); @@ -63,9 +63,9 @@ fn list_creates_one_by_one() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(9,) }, - HydrateText { path: &[0], value: "3", id: ElementId(10,) }, - InsertAfter { id: ElementId(7,), m: 1 }, + LoadTemplate { name: "template", index: 0, id: ElementId(8,) }, + HydrateText { path: &[0], value: "3", id: ElementId(9,) }, + InsertAfter { id: ElementId(6,), m: 1 }, ] ); } @@ -125,7 +125,7 @@ fn removes_one_by_one() { assert_eq!( dom.render_immediate().santize().edits, [ - CreatePlaceholder { id: ElementId(8) }, + CreatePlaceholder { id: ElementId(3) }, ReplaceWith { id: ElementId(2), m: 1 } ] ); @@ -136,13 +136,13 @@ fn removes_one_by_one() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(9) }, - HydrateText { path: &[0], value: "0", id: ElementId(10) }, - LoadTemplate { name: "template", index: 0, id: ElementId(11) }, - HydrateText { path: &[0], value: "1", id: ElementId(12) }, - LoadTemplate { name: "template", index: 0, id: ElementId(13) }, - HydrateText { path: &[0], value: "2", id: ElementId(14) }, - ReplaceWith { id: ElementId(8), m: 3 } + LoadTemplate { name: "template", index: 0, id: ElementId(2) }, + HydrateText { path: &[0], value: "0", id: ElementId(4) }, + 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 } ] ); } @@ -150,10 +150,9 @@ fn removes_one_by_one() { #[test] fn list_shrink_multiroot() { let mut dom = VirtualDom::new(|cx| { - let gen = cx.generation(); cx.render(rsx! { div { - (0..gen).map(|i| rsx! { + (0..cx.generation()).map(|i| rsx! { div { "{i}" } div { "{i}" } }) @@ -186,10 +185,10 @@ fn list_shrink_multiroot() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(7) }, - HydrateText { path: &[0], value: "1", id: ElementId(8) }, - LoadTemplate { name: "template", index: 1, id: ElementId(9) }, - HydrateText { path: &[0], value: "1", id: ElementId(10) }, + LoadTemplate { name: "template", index: 0, id: ElementId(2) }, + HydrateText { path: &[0], value: "1", id: ElementId(7) }, + LoadTemplate { name: "template", index: 1, id: ElementId(8) }, + HydrateText { path: &[0], value: "1", id: ElementId(9) }, InsertAfter { id: ElementId(5), m: 2 } ] ); @@ -198,11 +197,11 @@ fn list_shrink_multiroot() { assert_eq!( dom.render_immediate().santize().edits, [ - LoadTemplate { name: "template", index: 0, id: ElementId(11) }, - HydrateText { path: &[0], value: "2", id: ElementId(12) }, - LoadTemplate { name: "template", index: 1, id: ElementId(13) }, - HydrateText { path: &[0], value: "2", id: ElementId(14) }, - InsertAfter { id: ElementId(9), m: 2 } + LoadTemplate { name: "template", index: 0, id: ElementId(10) }, + HydrateText { path: &[0], value: "2", id: ElementId(11) }, + LoadTemplate { name: "template", index: 1, id: ElementId(12) }, + HydrateText { path: &[0], value: "2", id: ElementId(13) }, + InsertAfter { id: ElementId(8), m: 2 } ] ); } @@ -266,7 +265,7 @@ fn removes_one_by_one_multiroot() { dom.render_immediate().santize().edits, [ Remove { id: ElementId(4) }, - CreatePlaceholder { id: ElementId(14) }, + CreatePlaceholder { id: ElementId(5) }, ReplaceWith { id: ElementId(2), m: 1 } ] );