From 3edf3e367f8ac7fb813dc3826e780e2a1125b1d8 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Sun, 30 Jan 2022 14:08:03 -0500 Subject: [PATCH 01/14] wip: debugging --- packages/core/src/diff.rs | 59 ++++++++++-- packages/core/src/nodes.rs | 18 +++- packages/core/src/scopes.rs | 113 ++++++++++++---------- packages/core/src/virtual_dom.rs | 4 +- packages/core/tests/miri_stress.rs | 117 +++++++++++++++++++---- packages/core/tests/sharedstate.rs | 96 ++++++++++++++++++- packages/router/src/components/link.rs | 2 +- packages/router/src/components/route.rs | 4 +- packages/router/src/components/router.rs | 1 + packages/router/src/platform/mod.rs | 7 +- packages/router/src/service.rs | 76 ++++++++------- 11 files changed, 368 insertions(+), 129 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 499d9543ea..c8e2b29a7a 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -443,6 +443,13 @@ impl<'bump> DiffState<'bump> { new_idx }; + log::info!( + "created component {:?} with parent {:?} and originator {:?}", + new_idx, + parent_idx, + vcomponent.originator + ); + // Actually initialize the caller's slot with the right address vcomponent.scope.set(Some(new_idx)); @@ -476,6 +483,11 @@ impl<'bump> DiffState<'bump> { // Check the most common cases first // these are *actual* elements, not wrappers around lists (Text(old), Text(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - text are the sames"); + return; + } + if let Some(root) = old.id.get() { if old.text != new.text { self.mutations.set_text(new.text, root.as_u64()); @@ -487,24 +499,46 @@ impl<'bump> DiffState<'bump> { } (Placeholder(old), Placeholder(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - placeholder are the sames"); + return; + } + if let Some(root) = old.id.get() { self.scopes.update_node(new_node, root); new.id.set(Some(root)) } } - (Element(old), Element(new)) => self.diff_element_nodes(old, new, old_node, new_node), + (Element(old), Element(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - element are the sames"); + return; + } + self.diff_element_nodes(old, new, old_node, new_node) + } // These two sets are pointers to nodes but are not actually nodes themselves (Component(old), Component(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - placeholder are the sames"); + return; + } self.diff_component_nodes(old_node, new_node, *old, *new) } - (Fragment(old), Fragment(new)) => self.diff_fragment_nodes(old, new), + (Fragment(old), Fragment(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - fragment are the sames"); + return; + } + self.diff_fragment_nodes(old, new) + } // The normal pathway still works, but generates slightly weird instructions // This pathway ensures uses the ReplaceAll, not the InsertAfter and remove (Placeholder(_), Fragment(new)) => { + log::debug!("replacing placeholder with fragment {:?}", new_node); self.stack .create_children(new.children, MountType::Replace { old: old_node }); } @@ -686,7 +720,11 @@ impl<'bump> DiffState<'bump> { new: &'bump VComponent<'bump>, ) { let scope_addr = old.scope.get().unwrap(); - log::trace!("diff_component_nodes: {:?}", scope_addr); + log::trace!( + "diff_component_nodes. old: {:#?} new: {:#?}", + old_node, + new_node + ); if std::ptr::eq(old, new) { log::trace!("skipping component diff - component is the sames"); @@ -747,6 +785,8 @@ impl<'bump> DiffState<'bump> { self.stack.scope_stack.pop(); } else { + // + log::debug!("scope stack is {:#?}", self.stack.scope_stack); self.stack .create_node(new_node, MountType::Replace { old: old_node }); } @@ -790,7 +830,10 @@ impl<'bump> DiffState<'bump> { match (old, new) { ([], []) => {} ([], _) => self.stack.create_children(new, MountType::Append), - (_, []) => self.remove_nodes(old, true), + (_, []) => { + log::debug!("removing nodes {:?}", old); + self.remove_nodes(old, true) + } _ => { let new_is_keyed = new[0].key().is_some(); let old_is_keyed = old[0].key().is_some(); @@ -1216,6 +1259,7 @@ impl<'bump> DiffState<'bump> { } fn replace_node(&mut self, old: &'bump VNode<'bump>, nodes_created: usize) { + log::debug!("Replacing node {:?}", old); match old { VNode::Element(el) => { let id = old @@ -1262,11 +1306,12 @@ impl<'bump> DiffState<'bump> { ) { // or cache the vec on the diff machine for node in nodes { + log::debug!("removing {:?}", node); match node { VNode::Text(t) => { // this check exists because our null node will be removed but does not have an ID if let Some(id) = t.id.get() { - self.scopes.collect_garbage(id); + // self.scopes.collect_garbage(id); if gen_muts { self.mutations.remove(id.as_u64()); @@ -1275,7 +1320,7 @@ impl<'bump> DiffState<'bump> { } VNode::Placeholder(a) => { let id = a.id.get().unwrap(); - self.scopes.collect_garbage(id); + // self.scopes.collect_garbage(id); if gen_muts { self.mutations.remove(id.as_u64()); @@ -1289,6 +1334,8 @@ impl<'bump> DiffState<'bump> { } self.remove_nodes(e.children, false); + + // self.scopes.collect_garbage(id); } VNode::Fragment(f) => { diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index b18aeaf2db..64da46279d 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -4,7 +4,7 @@ //! cheap and *very* fast to construct - building a full tree should be quick. use crate::{ - innerlude::{Element, Properties, Scope, ScopeId, ScopeState}, + innerlude::{Element, FcSlot, Properties, Scope, ScopeId, ScopeState}, lazynodes::LazyNodes, AnyEvent, Component, }; @@ -177,11 +177,19 @@ impl Debug for VNode<'_> { .field("children", &el.children) .finish(), VNode::Text(t) => write!(s, "VNode::VText {{ text: {} }}", t.text), - VNode::Placeholder(_) => write!(s, "VNode::VPlaceholder"), + VNode::Placeholder(t) => write!(s, "VNode::VPlaceholder {{ id: {:?} }}", t.id), VNode::Fragment(frag) => { write!(s, "VNode::VFragment {{ children: {:?} }}", frag.children) } - VNode::Component(comp) => write!(s, "VNode::VComponent {{ fc: {:?}}}", comp.user_fc), + VNode::Component(comp) => { + s.debug_struct("VNode::VComponent") + .field("fnptr", &comp.user_fc) + .field("key", &comp.key) + .field("scope", &comp.scope) + .field("originator", &comp.originator) + .finish() + //write!(s, "VNode::VComponent {{ fc: {:?}}}", comp.user_fc) + } } } } @@ -384,7 +392,7 @@ pub struct VComponent<'src> { pub originator: ScopeId, pub scope: Cell>, pub can_memoize: bool, - pub user_fc: *const (), + pub user_fc: FcSlot, pub props: RefCell>>, } @@ -557,7 +565,7 @@ impl<'a> NodeFactory<'a> { key: key.map(|f| self.raw_text(f).0), scope: Default::default(), can_memoize: P::IS_STATIC, - user_fc: component as *const (), + user_fc: component as *mut std::os::raw::c_void, originator: self.scope.scope_id(), props: RefCell::new(Some(Box::new(VComponentProps { // local_props: RefCell::new(Some(props)), diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 77c648d180..d8cfd6505a 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -13,7 +13,9 @@ use std::{ rc::Rc, }; -pub(crate) type FcSlot = *const (); +/// for traceability, we use the raw fn pointer to identify the function +/// we can use this with the traceback crate to resolve funciton names +pub(crate) type FcSlot = *mut std::os::raw::c_void; pub(crate) struct Heuristic { hook_arena_size: usize, @@ -82,7 +84,7 @@ impl ScopeArena { pub(crate) fn new_with_key( &self, - fc_ptr: *const (), + fc_ptr: FcSlot, vcomp: Box, parent_scope: Option, container: ElementId, @@ -116,25 +118,47 @@ impl ScopeArena { scope.subtree.set(subtree); scope.our_arena_idx = new_scope_id; scope.container = container; + scope.fnptr = fc_ptr; let any_item = self.scopes.borrow_mut().insert(new_scope_id, scope); debug_assert!(any_item.is_none()); } else { // else create a new scope + let (node_capacity, hook_capacity) = self + .heuristics + .borrow() + .get(&fc_ptr) + .map(|h| (h.node_arena_size, h.hook_arena_size)) + .unwrap_or_default(); + self.scopes.borrow_mut().insert( new_scope_id, - self.bump.alloc(ScopeState::new( - height, + self.bump.alloc(ScopeState { container, - new_scope_id, + our_arena_idx: new_scope_id, parent_scope, - vcomp, - self.tasks.clone(), - self.heuristics - .borrow() - .get(&fc_ptr) - .map(|h| (h.node_arena_size, h.hook_arena_size)) - .unwrap_or_default(), - )), + height, + fnptr: fc_ptr, + props: RefCell::new(Some(vcomp)), + frames: [BumpFrame::new(node_capacity), BumpFrame::new(node_capacity)], + + // todo: subtrees + subtree: Cell::new(0), + is_subtree_root: Cell::new(false), + + generation: 0.into(), + + tasks: self.tasks.clone(), + shared_contexts: Default::default(), + + items: RefCell::new(SelfReferentialItems { + listeners: Default::default(), + borrowed_props: Default::default(), + }), + + hook_arena: Bump::new(), + hook_vals: RefCell::new(Vec::with_capacity(hook_capacity)), + hook_idx: Default::default(), + }), ); } @@ -169,10 +193,17 @@ impl ScopeArena { pub fn update_node<'a>(&self, node: &'a VNode<'a>, id: ElementId) { let node = unsafe { extend_vnode(node) }; - *self.nodes.borrow_mut().get_mut(id.0).unwrap() = node; + + let mut nodes = self.nodes.borrow_mut(); + let entry = nodes.get_mut(id.0); + match entry { + Some(_node) => *_node = node, + None => panic!("cannot update node {}", id), + } } pub fn collect_garbage(&self, id: ElementId) { + log::debug!("collecting garbage for {:?}", id); self.nodes.borrow_mut().remove(id.0); } @@ -189,7 +220,7 @@ impl ScopeArena { /// This also makes sure that drop order is consistent and predictable. All resources that rely on being dropped will /// be dropped. pub(crate) fn ensure_drop_safety(&self, scope_id: ScopeId) { - log::trace!("Ensuring drop safety for scope {:?}", scope_id); + // log::trace!("Ensuring drop safety for scope {:?}", scope_id); if let Some(scope) = self.get_scope(scope_id) { let mut items = scope.items.borrow_mut(); @@ -217,12 +248,23 @@ impl ScopeArena { // Cycle to the next frame and then reset it // This breaks any latent references, invalidating every pointer referencing into it. // Remove all the outdated listeners - log::trace!("Running scope {:?}", id); self.ensure_drop_safety(id); // todo: we *know* that this is aliased by the contents of the scope itself let scope = unsafe { &mut *self.get_scope_raw(id).expect("could not find scope") }; + // if cfg!(debug_assertions) { + log::debug!("running scope {:?} symbol: {:?}", id, scope.fnptr); + + // todo: resolve frames properly + backtrace::resolve(scope.fnptr, |symbol| { + // backtrace::resolve(scope.fnptr as *mut std::os::raw::c_void, |symbol| { + // panic!("asd"); + // log::trace!("Running scope {:?}, ptr {:?}", id, scope.fnptr); + log::debug!("running scope {:?} symbol: {:?}", id, symbol.name()); + }); + // } + // Safety: // - We dropped the listeners, so no more &mut T can be used while these are held // - All children nodes that rely on &mut T are replaced with a new reference @@ -421,6 +463,7 @@ pub struct ScopeState { pub(crate) container: ElementId, pub(crate) our_arena_idx: ScopeId, pub(crate) height: u32, + pub(crate) fnptr: FcSlot, // todo: subtrees pub(crate) is_subtree_root: Cell, @@ -449,43 +492,6 @@ pub struct SelfReferentialItems<'a> { // Public methods exposed to libraries and components impl ScopeState { - fn new( - height: u32, - container: ElementId, - our_arena_idx: ScopeId, - parent_scope: Option<*mut ScopeState>, - vcomp: Box, - tasks: Rc, - (node_capacity, hook_capacity): (usize, usize), - ) -> Self { - ScopeState { - container, - our_arena_idx, - parent_scope, - height, - props: RefCell::new(Some(vcomp)), - frames: [BumpFrame::new(node_capacity), BumpFrame::new(node_capacity)], - - // todo: subtrees - subtree: Cell::new(0), - is_subtree_root: Cell::new(false), - - generation: 0.into(), - - tasks, - shared_contexts: Default::default(), - - items: RefCell::new(SelfReferentialItems { - listeners: Default::default(), - borrowed_props: Default::default(), - }), - - hook_arena: Bump::new(), - hook_vals: RefCell::new(Vec::with_capacity(hook_capacity)), - hook_idx: Default::default(), - } - } - /// Get the subtree ID that this scope belongs to. /// /// Each component has its own subtree ID - the root subtree has an ID of 0. This ID is used by the renderer to route @@ -731,6 +737,7 @@ impl ScopeState { while let Some(parent_ptr) = search_parent { // safety: all parent pointers are valid thanks to the bump arena let parent = unsafe { &*parent_ptr }; + log::trace!("Searching parent scope {:?}", parent.scope_id()); if let Some(shared) = parent.shared_contexts.borrow().get(&TypeId::of::()) { return Some(shared.clone().downcast::().unwrap()); } diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 4e426caadd..1f443db864 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -212,7 +212,7 @@ impl VirtualDom { let scopes = ScopeArena::new(channel.0.clone()); scopes.new_with_key( - root as *const _, + root as *mut std::os::raw::c_void, Box::new(VComponentProps { props: root_props, memo: |_a, _b| unreachable!("memo on root will neve be run"), @@ -475,6 +475,8 @@ impl VirtualDom { let (old, new) = (self.scopes.wip_head(scopeid), self.scopes.fin_head(scopeid)); diff_state.stack.push(DiffInstruction::Diff { new, old }); + + log::debug!("pushing scope {:?} onto scope stack", scopeid); diff_state.stack.scope_stack.push(scopeid); let scope = scopes.get_scope(scopeid).unwrap(); diff --git a/packages/core/tests/miri_stress.rs b/packages/core/tests/miri_stress.rs index 6b78052618..0f4f64546d 100644 --- a/packages/core/tests/miri_stress.rs +++ b/packages/core/tests/miri_stress.rs @@ -319,36 +319,117 @@ fn test_pass_thru() { #[inline_props] fn Router<'a>(cx: Scope, children: Element<'a>) -> Element { cx.render(rsx! { - &cx.props.children + div { + &cx.props.children + } }) } - fn MemoizedThing(cx: Scope) -> Element { - rsx!(cx, div { "memoized" }) + #[inline_props] + fn NavContainer<'a>(cx: Scope, children: Element<'a>) -> Element { + cx.render(rsx! { + header { + nav { + &cx.props.children + } + } + }) } - fn app(cx: Scope) -> Element { - let thing = cx.use_hook(|_| "asd"); + fn NavMenu(cx: Scope) -> Element { rsx!(cx, - Router { - MemoizedThing { - } + NavBrand {} + div { + NavStart {} + NavEnd {} } ) } - let mut dom = new_dom(app, ()); - let _ = dom.rebuild(); + fn NavBrand(cx: Scope) -> Element { + rsx!(cx, div {}) + } - dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); - dom.work_with_deadline(|| false); + fn NavStart(cx: Scope) -> Element { + rsx!(cx, div {}) + } - dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); - dom.work_with_deadline(|| false); + fn NavEnd(cx: Scope) -> Element { + rsx!(cx, div {}) + } - dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); - dom.work_with_deadline(|| false); + #[inline_props] + fn MainContainer<'a>( + cx: Scope, + nav: Element<'a>, + body: Element<'a>, + footer: Element<'a>, + ) -> Element { + cx.render(rsx! { + div { + class: "columns is-mobile", + div { + class: "column is-full", + &cx.props.nav, + &cx.props.body, + &cx.props.footer, + } + } + }) + } - dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); - dom.work_with_deadline(|| false); + fn app(cx: Scope) -> Element { + let nav = cx.render(rsx! { + NavContainer { + NavMenu {} + } + }); + let body = cx.render(rsx! { + div {} + }); + let footer = cx.render(rsx! { + div {} + }); + + cx.render(rsx! { + MainContainer { + nav: nav, + body: body, + footer: footer, + } + }) + } + + let mut dom = new_dom(app, ()); + let _ = dom.rebuild(); + + for x in 0..40 { + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + + dom.handle_message(SchedulerMsg::Immediate(ScopeId(1))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(1))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(1))); + dom.work_with_deadline(|| false); + + dom.handle_message(SchedulerMsg::Immediate(ScopeId(2))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(2))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(2))); + dom.work_with_deadline(|| false); + + dom.handle_message(SchedulerMsg::Immediate(ScopeId(3))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(3))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(3))); + dom.work_with_deadline(|| false); + } } diff --git a/packages/core/tests/sharedstate.rs b/packages/core/tests/sharedstate.rs index 3f04aa1540..41b1787938 100644 --- a/packages/core/tests/sharedstate.rs +++ b/packages/core/tests/sharedstate.rs @@ -1,6 +1,6 @@ #![allow(unused, non_upper_case_globals)] -use dioxus::{prelude::*, DomEdit, Mutations}; +use dioxus::{prelude::*, DomEdit, Mutations, SchedulerMsg, ScopeId}; use dioxus_core as dioxus; use dioxus_core_macro::*; use dioxus_html as dioxus_elements; @@ -37,3 +37,97 @@ fn shared_state_test() { ] ); } + +#[test] +fn swap_test() { + struct MySharedState(&'static str); + + fn app(cx: Scope) -> Element { + let val = cx.use_hook(|_| 0); + *val += 1; + + cx.provide_context(MySharedState("world!")); + + let child = match *val % 2 { + 0 => rsx!( + Child1 { + Child1 { } + Child2 { } + } + ), + _ => rsx!( + Child2 { + Child2 { } + Child2 { } + } + ), + }; + + cx.render(rsx!( + Router { + div { child } + } + )) + } + + #[inline_props] + fn Router<'a>(cx: Scope, children: Element<'a>) -> Element<'a> { + cx.render(rsx!(div { children })) + } + + #[inline_props] + fn Child1<'a>(cx: Scope, children: Element<'a>) -> Element { + let shared = cx.consume_context::().unwrap(); + println!("Child1: {}", shared.0); + cx.render(rsx! { + div { + "{shared.0}", + children + } + }) + } + + #[inline_props] + fn Child2<'a>(cx: Scope, children: Element<'a>) -> Element { + let shared = cx.consume_context::().unwrap(); + println!("Child2: {}", shared.0); + cx.render(rsx! { + h1 { + "{shared.0}", + children + } + }) + } + + let mut dom = VirtualDom::new(app); + let Mutations { edits, .. } = dom.rebuild(); + + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + dom.work_with_deadline(|| false); + + // dom.handle_message(SchedulerMsg::Immediate(ScopeId(1))); + // dom.work_with_deadline(|| false); + + // dom.handle_message(SchedulerMsg::Immediate(ScopeId(2))); + // dom.work_with_deadline(|| false); + + // dom.handle_message(SchedulerMsg::Immediate(ScopeId(3))); + // dom.work_with_deadline(|| false); + + // dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + // dom.work_with_deadline(|| false); + // dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + // dom.work_with_deadline(|| false); + // dom.handle_message(SchedulerMsg::Immediate(ScopeId(0))); + // dom.work_with_deadline(|| false); +} diff --git a/packages/router/src/components/link.rs b/packages/router/src/components/link.rs index fd8eea66d6..18e9fd92ee 100644 --- a/packages/router/src/components/link.rs +++ b/packages/router/src/components/link.rs @@ -41,7 +41,7 @@ pub struct LinkProps<'a> { } pub fn Link<'a>(cx: Scope<'a, LinkProps<'a>>) -> Element { - log::trace!("render Link to {}", cx.props.to); + // log::trace!("render Link to {}", cx.props.to); if let Some(service) = cx.consume_context::() { return cx.render(rsx! { a { diff --git a/packages/router/src/components/route.rs b/packages/router/src/components/route.rs index 129953eea4..e4dd2f9c8c 100644 --- a/packages/router/src/components/route.rs +++ b/packages/router/src/components/route.rs @@ -30,7 +30,7 @@ pub fn Route<'a>(cx: Scope<'a, RouteProps<'a>>) -> Element { Some(ctx) => ctx.total_route.to_string(), None => cx.props.to.to_string(), }; - log::trace!("total route for {} is {}", cx.props.to, total_route); + // log::trace!("total route for {} is {}", cx.props.to, total_route); // provide our route context let route_context = cx.provide_context(RouteContext { @@ -48,7 +48,7 @@ pub fn Route<'a>(cx: Scope<'a, RouteProps<'a>>) -> Element { Some(RouteInner {}) }); - log::trace!("Checking route {}", cx.props.to); + // log::trace!("Checking route {}", cx.props.to); if router_root.should_render(cx.scope_id()) { cx.render(rsx!(&cx.props.children)) diff --git a/packages/router/src/components/router.rs b/packages/router/src/components/router.rs index 240c3806ae..bf23645f59 100644 --- a/packages/router/src/components/router.rs +++ b/packages/router/src/components/router.rs @@ -17,6 +17,7 @@ pub struct RouterProps<'a> { #[allow(non_snake_case)] pub fn Router<'a>(cx: Scope<'a, RouterProps<'a>>) -> Element { + log::debug!("running router {:?}", cx.scope_id()); let svc = cx.use_hook(|_| { let update = cx.schedule_update_any(); cx.provide_context(RouterService::new(update, cx.scope_id())) diff --git a/packages/router/src/platform/mod.rs b/packages/router/src/platform/mod.rs index 7afc1316a6..ec59a4703c 100644 --- a/packages/router/src/platform/mod.rs +++ b/packages/router/src/platform/mod.rs @@ -1,9 +1,4 @@ pub trait RouterProvider { fn get_current_route(&self) -> String; - fn subscribe_to_route_changes(&self, callback: Box); -} - -enum RouteChange { - LinkTo(String), - Back(String), + fn listen(&self, callback: Box); } diff --git a/packages/router/src/service.rs b/packages/router/src/service.rs index b62bc78d56..b6d8ba11b7 100644 --- a/packages/router/src/service.rs +++ b/packages/router/src/service.rs @@ -7,14 +7,18 @@ use std::{ use dioxus_core::ScopeId; +use crate::platform::RouterProvider; + pub struct RouterService { pub(crate) regen_route: Rc, pub(crate) pending_events: Rc>>, - history: Rc>, slots: Rc>>, onchange_listeners: Rc>>, root_found: Rc>>, cur_path_params: Rc>>, + + // history: Rc, + history: Rc>, listener: HistoryListener, } @@ -58,12 +62,12 @@ impl RouterService { root_found.set(None); // checking if the route is valid is cheap, so we do it for (slot, root) in slots.borrow_mut().iter().rev() { - log::trace!("regenerating slot {:?} for root '{}'", slot, root); + // log::trace!("regenerating slot {:?} for root '{}'", slot, root); regen_route(*slot); } for listener in onchange_listeners.borrow_mut().iter() { - log::trace!("regenerating listener {:?}", listener); + // log::trace!("regenerating listener {:?}", listener); regen_route(*listener); } @@ -87,31 +91,31 @@ impl RouterService { } pub fn push_route(&self, route: &str) { - log::trace!("Pushing route: {}", route); + // log::trace!("Pushing route: {}", route); self.history.borrow_mut().push(route); } pub fn register_total_route(&self, route: String, scope: ScopeId, fallback: bool) { let clean = clean_route(route); - log::trace!("Registered route '{}' with scope id {:?}", clean, scope); + // log::trace!("Registered route '{}' with scope id {:?}", clean, scope); self.slots.borrow_mut().push((scope, clean)); } pub fn should_render(&self, scope: ScopeId) -> bool { - log::trace!("Should render scope id {:?}?", scope); + // log::trace!("Should render scope id {:?}?", scope); if let Some(root_id) = self.root_found.get() { - log::trace!(" we already found a root with scope id {:?}", root_id); + // log::trace!(" we already found a root with scope id {:?}", root_id); if root_id == scope { - log::trace!(" yes - it's a match"); + // log::trace!(" yes - it's a match"); return true; } - log::trace!(" no - it's not a match"); + // log::trace!(" no - it's not a match"); return false; } let location = self.history.borrow().location(); let path = location.path(); - log::trace!(" current path is '{}'", path); + // log::trace!(" current path is '{}'", path); let roots = self.slots.borrow(); @@ -120,23 +124,23 @@ impl RouterService { // fallback logic match root { Some((id, route)) => { - log::trace!( - " matched given scope id {:?} with route root '{}'", - scope, - route, - ); + // log::trace!( + // " matched given scope id {:?} with route root '{}'", + // scope, + // route, + // ); if let Some(params) = route_matches_path(route, path) { - log::trace!(" and it matches the current path '{}'", path); + // log::trace!(" and it matches the current path '{}'", path); self.root_found.set(Some(*id)); *self.cur_path_params.borrow_mut() = params; true } else { if route == "" { - log::trace!(" and the route is the root, so we will use that without a better match"); + // log::trace!(" and the route is the root, so we will use that without a better match"); self.root_found.set(Some(*id)); true } else { - log::trace!(" and the route '{}' is not the root nor does it match the current path", route); + // log::trace!(" and the route '{}' is not the root nor does it match the current path", route); false } } @@ -154,12 +158,12 @@ impl RouterService { } pub fn subscribe_onchange(&self, id: ScopeId) { - log::trace!("Subscribing onchange for scope id {:?}", id); + // log::trace!("Subscribing onchange for scope id {:?}", id); self.onchange_listeners.borrow_mut().insert(id); } pub fn unsubscribe_onchange(&self, id: ScopeId) { - log::trace!("Subscribing onchange for scope id {:?}", id); + // log::trace!("Subscribing onchange for scope id {:?}", id); self.onchange_listeners.borrow_mut().remove(&id); } } @@ -182,36 +186,36 @@ fn route_matches_path(route: &str, path: &str) -> Option let route_pieces = route.split('/').collect::>(); let path_pieces = clean_path(path).split('/').collect::>(); - log::trace!( - " checking route pieces {:?} vs path pieces {:?}", - route_pieces, - path_pieces, - ); + // log::trace!( + // " checking route pieces {:?} vs path pieces {:?}", + // route_pieces, + // path_pieces, + // ); if route_pieces.len() != path_pieces.len() { - log::trace!(" the routes are different lengths"); + // log::trace!(" the routes are different lengths"); return None; } let mut matches = HashMap::new(); for (i, r) in route_pieces.iter().enumerate() { - log::trace!(" checking route piece '{}' vs path", r); + // log::trace!(" checking route piece '{}' vs path", r); // If this is a parameter then it matches as long as there's // _any_thing in that spot in the path. if r.starts_with(':') { - log::trace!( - " route piece '{}' starts with a colon so it matches anything", - r, - ); + // log::trace!( + // " route piece '{}' starts with a colon so it matches anything", + // r, + // ); let param = &r[1..]; matches.insert(param.to_string(), path_pieces[i].to_string()); continue; } - log::trace!( - " route piece '{}' must be an exact match for path piece '{}'", - r, - path_pieces[i], - ); + // log::trace!( + // " route piece '{}' must be an exact match for path piece '{}'", + // r, + // path_pieces[i], + // ); if path_pieces[i] != *r { return None; } From 3bb5c8142cf63489ffd0ff0c316cc168852d5c4f Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Sun, 30 Jan 2022 17:47:58 -0500 Subject: [PATCH 02/14] fix: switch to future based diffing this commit removes the old manual fiber implementation in favor of a traditional recursion based approach. This should make the diffing algorithm easier to work on and eliminate various stack-based bugs in. --- packages/core/.rustfmt.toml | 1 + packages/core/src/diff.rs | 21 +- packages/core/src/diff_async.rs | 1102 ++++++++++++++++++++++++++++++ packages/core/src/lib.rs | 6 +- packages/core/src/mutations.rs | 8 + packages/core/src/nodes.rs | 59 +- packages/core/src/scopes.rs | 32 +- packages/core/src/virtual_dom.rs | 100 +-- packages/core/tests/diffing.rs | 257 ++----- 9 files changed, 1266 insertions(+), 320 deletions(-) create mode 100644 packages/core/.rustfmt.toml create mode 100644 packages/core/src/diff_async.rs diff --git a/packages/core/.rustfmt.toml b/packages/core/.rustfmt.toml new file mode 100644 index 0000000000..70a9483a88 --- /dev/null +++ b/packages/core/.rustfmt.toml @@ -0,0 +1 @@ +struct_lit_width = 80 diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index c8e2b29a7a..3b9c7692cc 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -222,7 +222,7 @@ impl<'bump> DiffStack<'bump> { self.nodes_created_stack.pop().unwrap() } - fn current_scope(&self) -> Option { + pub fn current_scope(&self) -> Option { self.scope_stack.last().copied() } @@ -314,9 +314,10 @@ impl<'bump> DiffState<'bump> { } MountType::Append => { - self.mutations.edits.push(AppendChildren { - many: nodes_created as u32, - }); + self.mutations.append_children(nodes_created as u32); + // self.mutations.edits.push(AppendChildren { + // many: nodes_created as u32, + // }); } MountType::InsertAfter { other_node } => { @@ -470,7 +471,8 @@ impl<'bump> DiffState<'bump> { self.stack.create_component(new_idx, nextnode); // Finally, insert this scope as a seen node. - self.mutations.dirty_scopes.insert(new_idx); + self.mutations.mark_dirty_scope(new_idx); + // self.mutations.dirty_scopes.insert(new_idx); } // ================================= @@ -639,9 +641,10 @@ impl<'bump> DiffState<'bump> { } if old.children.is_empty() && !new.children.is_empty() { - self.mutations.edits.push(PushRoot { - root: root.as_u64(), - }); + self.mutations.push_root(root); + // self.mutations.edits.push(PushRoot { + // root: root.as_u64(), + // }); self.stack.element_stack.push(root); self.stack.instructions.push(DiffInstruction::PopElement); self.stack.create_children(new.children, MountType::Append); @@ -771,7 +774,7 @@ impl<'bump> DiffState<'bump> { // this should auto drop the previous props self.scopes.run_scope(scope_addr); - self.mutations.dirty_scopes.insert(scope_addr); + self.mutations.mark_dirty_scope(scope_addr); self.diff_node( self.scopes.wip_head(scope_addr), diff --git a/packages/core/src/diff_async.rs b/packages/core/src/diff_async.rs new file mode 100644 index 0000000000..e5d63ffb56 --- /dev/null +++ b/packages/core/src/diff_async.rs @@ -0,0 +1,1102 @@ +use crate::innerlude::*; +use fxhash::{FxHashMap, FxHashSet}; +use smallvec::{smallvec, SmallVec}; + +pub(crate) struct AsyncDiffState<'bump> { + pub(crate) scopes: &'bump ScopeArena, + pub(crate) mutations: Mutations<'bump>, + pub(crate) force_diff: bool, + pub(crate) element_stack: SmallVec<[ElementId; 10]>, + pub(crate) scope_stack: SmallVec<[ScopeId; 5]>, +} + +impl<'b> AsyncDiffState<'b> { + pub fn new(scopes: &'b ScopeArena) -> Self { + Self { + scopes, + mutations: Mutations::new(), + force_diff: false, + element_stack: smallvec![], + scope_stack: smallvec![], + } + } + + pub fn diff_scope(&mut self, scopeid: ScopeId) { + let (old, new) = (self.scopes.wip_head(scopeid), self.scopes.fin_head(scopeid)); + self.scope_stack.push(scopeid); + let scope = self.scopes.get_scope(scopeid).unwrap(); + self.element_stack.push(scope.container); + self.diff_node(old, new); + } + + pub fn diff_node(&mut self, old_node: &'b VNode<'b>, new_node: &'b VNode<'b>) { + use VNode::*; + match (old_node, new_node) { + // Check the most common cases first + // these are *actual* elements, not wrappers around lists + (Text(old), Text(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - text are the sames"); + return; + } + + if let Some(root) = old.id.get() { + if old.text != new.text { + self.mutations.set_text(new.text, root.as_u64()); + } + self.scopes.update_node(new_node, root); + + new.id.set(Some(root)); + } + } + + (Placeholder(old), Placeholder(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - placeholder are the sames"); + return; + } + + if let Some(root) = old.id.get() { + self.scopes.update_node(new_node, root); + new.id.set(Some(root)) + } + } + + (Element(old), Element(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - element are the sames"); + return; + } + self.diff_element_nodes(old, new, old_node, new_node) + } + + // These two sets are pointers to nodes but are not actually nodes themselves + (Component(old), Component(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - placeholder are the sames"); + return; + } + self.diff_component_nodes(old_node, new_node, *old, *new) + } + + (Fragment(old), Fragment(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - fragment are the sames"); + return; + } + self.diff_fragment_nodes(old, new) + } + + // The normal pathway still works, but generates slightly weird instructions + // This pathway ensures uses the ReplaceAll, not the InsertAfter and remove + (Placeholder(_), Fragment(_)) => { + log::debug!("replacing placeholder with fragment {:?}", new_node); + self.replace_node(old_node, new_node); + } + + // Anything else is just a basic replace and create + ( + Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), + Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), + ) => self.replace_node(old_node, new_node), + } + } + + pub fn create_node(&mut self, node: &'b VNode<'b>) -> usize { + match node { + VNode::Text(vtext) => self.create_text_node(vtext, node), + VNode::Placeholder(anchor) => self.create_anchor_node(anchor, node), + VNode::Element(element) => self.create_element_node(element, node), + VNode::Fragment(frag) => self.create_fragment_node(frag), + VNode::Component(component) => self.create_component_node(*component), + } + } + + fn create_text_node(&mut self, vtext: &'b VText<'b>, node: &'b VNode<'b>) -> usize { + let real_id = self.scopes.reserve_node(node); + self.mutations.create_text_node(vtext.text, real_id); + vtext.id.set(Some(real_id)); + + 1 + } + + fn create_anchor_node(&mut self, anchor: &'b VPlaceholder, node: &'b VNode<'b>) -> usize { + let real_id = self.scopes.reserve_node(node); + self.mutations.create_placeholder(real_id); + anchor.id.set(Some(real_id)); + + 1 + } + + fn create_element_node(&mut self, element: &'b VElement<'b>, node: &'b VNode<'b>) -> usize { + let VElement { + tag: tag_name, + listeners, + attributes, + children, + namespace, + id: dom_id, + parent: parent_id, + .. + } = element; + + // set the parent ID for event bubbling + // self.stack.instructions.push(DiffInstruction::PopElement); + + let parent = self.element_stack.last().unwrap(); + parent_id.set(Some(*parent)); + + // set the id of the element + let real_id = self.scopes.reserve_node(node); + self.element_stack.push(real_id); + dom_id.set(Some(real_id)); + + self.mutations.create_element(tag_name, *namespace, real_id); + + if let Some(cur_scope_id) = self.current_scope() { + for listener in *listeners { + listener.mounted_node.set(Some(real_id)); + self.mutations.new_event_listener(listener, cur_scope_id); + } + } else { + log::warn!("create element called with no scope on the stack - this is an error for a live dom"); + } + + for attr in *attributes { + self.mutations.set_attribute(attr, real_id.as_u64()); + } + + if !children.is_empty() { + self.create_and_append_children(children); + } + + 1 + } + + fn create_fragment_node(&mut self, frag: &'b VFragment<'b>) -> usize { + self.create_children(frag.children) + } + + fn create_component_node(&mut self, vcomponent: &'b VComponent<'b>) -> usize { + let parent_idx = self.current_scope().unwrap(); + + // the component might already exist - if it does, we need to reuse it + // this makes figure out when to drop the component more complicated + let new_idx = if let Some(idx) = vcomponent.scope.get() { + assert!(self.scopes.get_scope(idx).is_some()); + idx + } else { + // Insert a new scope into our component list + let props: Box = vcomponent.props.borrow_mut().take().unwrap(); + let props: Box = unsafe { std::mem::transmute(props) }; + let new_idx = self.scopes.new_with_key( + vcomponent.user_fc, + props, + Some(parent_idx), + self.element_stack.last().copied().unwrap(), + 0, + ); + + new_idx + }; + + log::info!( + "created component {:?} with parent {:?} and originator {:?}", + new_idx, + parent_idx, + vcomponent.originator + ); + + // Actually initialize the caller's slot with the right address + vcomponent.scope.set(Some(new_idx)); + + match vcomponent.can_memoize { + true => { + // todo: implement promotion logic. save us from boxing props that we don't need + } + false => { + // track this component internally so we know the right drop order + } + } + + // Run the scope for one iteration to initialize it + self.scopes.run_scope(new_idx); + + self.mutations.mark_dirty_scope(new_idx); + + // self.stack.create_component(new_idx, nextnode); + // Finally, insert this scope as a seen node. + + // Take the node that was just generated from running the component + let nextnode = self.scopes.fin_head(new_idx); + self.create_node(nextnode) + } + + fn diff_element_nodes( + &mut self, + old: &'b VElement<'b>, + new: &'b VElement<'b>, + old_node: &'b VNode<'b>, + new_node: &'b VNode<'b>, + ) { + let root = old.id.get().unwrap(); + + // If the element type is completely different, the element needs to be re-rendered completely + // This is an optimization React makes due to how users structure their code + // + // This case is rather rare (typically only in non-keyed lists) + if new.tag != old.tag || new.namespace != old.namespace { + self.replace_node(old_node, new_node); + return; + } + + self.scopes.update_node(new_node, root); + + new.id.set(Some(root)); + new.parent.set(old.parent.get()); + + // todo: attributes currently rely on the element on top of the stack, but in theory, we only need the id of the + // element to modify its attributes. + // it would result in fewer instructions if we just set the id directly. + // it would also clean up this code some, but that's not very important anyways + + // Diff Attributes + // + // It's extraordinarily rare to have the number/order of attributes change + // In these cases, we just completely erase the old set and make a new set + // + // TODO: take a more efficient path than this + if old.attributes.len() == new.attributes.len() { + for (old_attr, new_attr) in old.attributes.iter().zip(new.attributes.iter()) { + if old_attr.value != new_attr.value || new_attr.is_volatile { + self.mutations.set_attribute(new_attr, root.as_u64()); + } + } + } else { + for attribute in old.attributes { + self.mutations.remove_attribute(attribute, root.as_u64()); + } + for attribute in new.attributes { + self.mutations.set_attribute(attribute, root.as_u64()) + } + } + + // Diff listeners + // + // It's extraordinarily rare to have the number/order of listeners change + // In the cases where the listeners change, we completely wipe the data attributes and add new ones + // + // We also need to make sure that all listeners are properly attached to the parent scope (fix_listener) + // + // TODO: take a more efficient path than this + if let Some(cur_scope_id) = self.current_scope() { + if old.listeners.len() == new.listeners.len() { + for (old_l, new_l) in old.listeners.iter().zip(new.listeners.iter()) { + if old_l.event != new_l.event { + self.mutations + .remove_event_listener(old_l.event, root.as_u64()); + self.mutations.new_event_listener(new_l, cur_scope_id); + } + new_l.mounted_node.set(old_l.mounted_node.get()); + } + } else { + for listener in old.listeners { + self.mutations + .remove_event_listener(listener.event, root.as_u64()); + } + for listener in new.listeners { + listener.mounted_node.set(Some(root)); + self.mutations.new_event_listener(listener, cur_scope_id); + } + } + } + + match (old.children.len(), new.children.len()) { + (0, 0) => {} + (0, _) => { + let created = self.create_children(new.children); + self.mutations.append_children(created as u32); + } + (_, _) => { + self.diff_children(old.children, new.children); + } + }; + } + + fn diff_component_nodes( + &mut self, + old_node: &'b VNode<'b>, + new_node: &'b VNode<'b>, + old: &'b VComponent<'b>, + new: &'b VComponent<'b>, + ) { + let scope_addr = old.scope.get().unwrap(); + log::trace!( + "diff_component_nodes. old: {:#?} new: {:#?}", + old_node, + new_node + ); + + if std::ptr::eq(old, new) { + log::trace!("skipping component diff - component is the sames"); + return; + } + + // Make sure we're dealing with the same component (by function pointer) + if old.user_fc == new.user_fc { + self.enter_scope(scope_addr); + + // Make sure the new component vnode is referencing the right scope id + new.scope.set(Some(scope_addr)); + + // make sure the component's caller function is up to date + let scope = self + .scopes + .get_scope(scope_addr) + .unwrap_or_else(|| panic!("could not find {:?}", scope_addr)); + + // take the new props out regardless + // when memoizing, push to the existing scope if memoization happens + let new_props = new.props.borrow_mut().take().unwrap(); + + let should_run = { + if old.can_memoize { + let props_are_the_same = unsafe { + scope + .props + .borrow() + .as_ref() + .unwrap() + .memoize(new_props.as_ref()) + }; + !props_are_the_same || self.force_diff + } else { + true + } + }; + + if should_run { + let _old_props = scope + .props + .replace(unsafe { std::mem::transmute(Some(new_props)) }); + + // this should auto drop the previous props + self.scopes.run_scope(scope_addr); + self.mutations.mark_dirty_scope(scope_addr); + + self.diff_node( + self.scopes.wip_head(scope_addr), + self.scopes.fin_head(scope_addr), + ); + } else { + log::trace!("memoized"); + // memoization has taken place + drop(new_props); + }; + + self.leave_scope(); + } else { + log::debug!("scope stack is {:#?}", self.scope_stack); + self.replace_node(old_node, new_node); + } + } + + fn diff_fragment_nodes(&mut self, old: &'b VFragment<'b>, new: &'b VFragment<'b>) { + // This is the case where options or direct vnodes might be used. + // In this case, it's faster to just skip ahead to their diff + if old.children.len() == 1 && new.children.len() == 1 { + self.diff_node(&old.children[0], &new.children[0]); + return; + } + + debug_assert!(!old.children.is_empty()); + debug_assert!(!new.children.is_empty()); + + self.diff_children(old.children, new.children); + } + + // ============================================= + // Utilities for creating new diff instructions + // ============================================= + + // Diff the given set of old and new children. + // + // The parent must be on top of the change list stack when this function is + // entered: + // + // [... parent] + // + // the change list stack is in the same state when this function returns. + // + // If old no anchors are provided, then it's assumed that we can freely append to the parent. + // + // Remember, non-empty lists does not mean that there are real elements, just that there are virtual elements. + // + // Fragment nodes cannot generate empty children lists, so we can assume that when a list is empty, it belongs only + // to an element, and appending makes sense. + fn diff_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { + // Remember, fragments can never be empty (they always have a single child) + match (old, new) { + ([], []) => {} + ([], _) => self.create_and_append_children(new), + (_, []) => self.remove_nodes(old, true), + _ => { + let new_is_keyed = new[0].key().is_some(); + let old_is_keyed = old[0].key().is_some(); + + debug_assert!( + new.iter().all(|n| n.key().is_some() == new_is_keyed), + "all siblings must be keyed or all siblings must be non-keyed" + ); + debug_assert!( + old.iter().all(|o| o.key().is_some() == old_is_keyed), + "all siblings must be keyed or all siblings must be non-keyed" + ); + + if new_is_keyed && old_is_keyed { + self.diff_keyed_children(old, new); + } else { + self.diff_non_keyed_children(old, new); + } + } + } + } + + // Diff children that are not keyed. + // + // The parent must be on the top of the change list stack when entering this + // function: + // + // [... parent] + // + // the change list stack is in the same state when this function returns. + fn diff_non_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { + // Handled these cases in `diff_children` before calling this function. + debug_assert!(!new.is_empty()); + debug_assert!(!old.is_empty()); + + use std::cmp::Ordering; + match old.len().cmp(&new.len()) { + Ordering::Greater => self.remove_nodes(&old[new.len()..], true), + Ordering::Less => self.create_and_insert_after(&new[old.len()..], old.last().unwrap()), + Ordering::Equal => {} + } + + for (new, old) in new.iter().zip(old.iter()) { + self.diff_node(old, new); + } + } + + // Diffing "keyed" children. + // + // With keyed children, we care about whether we delete, move, or create nodes + // versus mutate existing nodes in place. Presumably there is some sort of CSS + // transition animation that makes the virtual DOM diffing algorithm + // observable. By specifying keys for nodes, we know which virtual DOM nodes + // must reuse (or not reuse) the same physical DOM nodes. + // + // This is loosely based on Inferno's keyed patching implementation. However, we + // have to modify the algorithm since we are compiling the diff down into change + // list instructions that will be executed later, rather than applying the + // changes to the DOM directly as we compare virtual DOMs. + // + // https://github.com/infernojs/inferno/blob/36fd96/packages/inferno/src/DOM/patching.ts#L530-L739 + // + // The stack is empty upon entry. + fn diff_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { + if cfg!(debug_assertions) { + let mut keys = fxhash::FxHashSet::default(); + let mut assert_unique_keys = |children: &'b [VNode<'b>]| { + keys.clear(); + for child in children { + let key = child.key(); + debug_assert!( + key.is_some(), + "if any sibling is keyed, all siblings must be keyed" + ); + keys.insert(key); + } + debug_assert_eq!( + children.len(), + keys.len(), + "keyed siblings must each have a unique key" + ); + }; + assert_unique_keys(old); + assert_unique_keys(new); + } + + // First up, we diff all the nodes with the same key at the beginning of the + // children. + // + // `shared_prefix_count` is the count of how many nodes at the start of + // `new` and `old` share the same keys. + let (left_offset, right_offset) = match self.diff_keyed_ends(old, new) { + Some(count) => count, + None => return, + }; + + // Ok, we now hopefully have a smaller range of children in the middle + // within which to re-order nodes with the same keys, remove old nodes with + // now-unused keys, and create new nodes with fresh keys. + + let old_middle = &old[left_offset..(old.len() - right_offset)]; + let new_middle = &new[left_offset..(new.len() - right_offset)]; + + debug_assert!( + !((old_middle.len() == new_middle.len()) && old_middle.is_empty()), + "keyed children must have the same number of children" + ); + + if new_middle.is_empty() { + // remove the old elements + self.remove_nodes(old_middle, true); + } else if old_middle.is_empty() { + // there were no old elements, so just create the new elements + // we need to find the right "foothold" though - we shouldn't use the "append" at all + if left_offset == 0 { + // insert at the beginning of the old list + let foothold = &old[old.len() - right_offset]; + self.create_and_insert_before(new_middle, foothold); + } else if right_offset == 0 { + // insert at the end the old list + let foothold = old.last().unwrap(); + self.create_and_insert_after(new_middle, foothold); + } else { + // inserting in the middle + let foothold = &old[left_offset - 1]; + self.create_and_insert_after(new_middle, foothold); + } + } else { + self.diff_keyed_middle(old_middle, new_middle); + } + } + + /// Diff both ends of the children that share keys. + /// + /// Returns a left offset and right offset of that indicates a smaller section to pass onto the middle diffing. + /// + /// If there is no offset, then this function returns None and the diffing is complete. + fn diff_keyed_ends( + &mut self, + old: &'b [VNode<'b>], + new: &'b [VNode<'b>], + ) -> Option<(usize, usize)> { + let mut left_offset = 0; + + for (old, new) in old.iter().zip(new.iter()) { + // abort early if we finally run into nodes with different keys + if old.key() != new.key() { + break; + } + self.diff_node(old, new); + left_offset += 1; + } + + // If that was all of the old children, then create and append the remaining + // new children and we're finished. + if left_offset == old.len() { + self.create_and_insert_after(&new[left_offset..], old.last().unwrap()); + return None; + } + + // And if that was all of the new children, then remove all of the remaining + // old children and we're finished. + if left_offset == new.len() { + self.remove_nodes(&old[left_offset..], true); + return None; + } + + // if the shared prefix is less than either length, then we need to walk backwards + let mut right_offset = 0; + for (old, new) in old.iter().rev().zip(new.iter().rev()) { + // abort early if we finally run into nodes with different keys + if old.key() != new.key() { + break; + } + self.diff_node(old, new); + right_offset += 1; + } + + Some((left_offset, right_offset)) + } + + // The most-general, expensive code path for keyed children diffing. + // + // We find the longest subsequence within `old` of children that are relatively + // ordered the same way in `new` (via finding a longest-increasing-subsequence + // of the old child's index within `new`). The children that are elements of + // this subsequence will remain in place, minimizing the number of DOM moves we + // will have to do. + // + // Upon entry to this function, the change list stack must be empty. + // + // This function will load the appropriate nodes onto the stack and do diffing in place. + // + // Upon exit from this function, it will be restored to that same self. + fn diff_keyed_middle(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { + /* + 1. Map the old keys into a numerical ordering based on indices. + 2. Create a map of old key to its index + 3. Map each new key to the old key, carrying over the old index. + - IE if we have ABCD becomes BACD, our sequence would be 1,0,2,3 + - if we have ABCD to ABDE, our sequence would be 0,1,3,MAX because E doesn't exist + + now, we should have a list of integers that indicates where in the old list the new items map to. + + 4. Compute the LIS of this list + - this indicates the longest list of new children that won't need to be moved. + + 5. Identify which nodes need to be removed + 6. Identify which nodes will need to be diffed + + 7. Going along each item in the new list, create it and insert it before the next closest item in the LIS. + - if the item already existed, just move it to the right place. + + 8. Finally, generate instructions to remove any old children. + 9. Generate instructions to finally diff children that are the same between both + */ + + // 0. Debug sanity checks + // Should have already diffed the shared-key prefixes and suffixes. + debug_assert_ne!(new.first().map(|n| n.key()), old.first().map(|o| o.key())); + debug_assert_ne!(new.last().map(|n| n.key()), old.last().map(|o| o.key())); + + // 1. Map the old keys into a numerical ordering based on indices. + // 2. Create a map of old key to its index + // IE if the keys were A B C, then we would have (A, 1) (B, 2) (C, 3). + let old_key_to_old_index = old + .iter() + .enumerate() + .map(|(i, o)| (o.key().unwrap(), i)) + .collect::>(); + + let mut shared_keys = FxHashSet::default(); + + // 3. Map each new key to the old key, carrying over the old index. + let new_index_to_old_index = new + .iter() + .map(|node| { + let key = node.key().unwrap(); + if let Some(&index) = old_key_to_old_index.get(&key) { + shared_keys.insert(key); + index + } else { + u32::MAX as usize + } + }) + .collect::>(); + + // If none of the old keys are reused by the new children, then we remove all the remaining old children and + // create the new children afresh. + if shared_keys.is_empty() { + if let Some(first_old) = old.get(0) { + self.remove_nodes(&old[1..], true); + let nodes_created = self.create_children(new); + self.replace_inner(first_old, nodes_created); + } else { + // I think this is wrong - why are we appending? + // only valid of the if there are no trailing elements + self.create_and_append_children(new); + } + return; + } + + // 4. Compute the LIS of this list + let mut lis_sequence = Vec::default(); + lis_sequence.reserve(new_index_to_old_index.len()); + + let mut predecessors = vec![0; new_index_to_old_index.len()]; + let mut starts = vec![0; new_index_to_old_index.len()]; + + longest_increasing_subsequence::lis_with( + &new_index_to_old_index, + &mut lis_sequence, + |a, b| a < b, + &mut predecessors, + &mut starts, + ); + + // the lis comes out backwards, I think. can't quite tell. + lis_sequence.sort_unstable(); + + // if a new node gets u32 max and is at the end, then it might be part of our LIS (because u32 max is a valid LIS) + if lis_sequence.last().map(|f| new_index_to_old_index[*f]) == Some(u32::MAX as usize) { + lis_sequence.pop(); + } + + for idx in lis_sequence.iter() { + self.diff_node(&old[new_index_to_old_index[*idx]], &new[*idx]); + } + + let mut nodes_created = 0; + + // add mount instruction for the first items not covered by the lis + let last = *lis_sequence.last().unwrap(); + if last < (new.len() - 1) { + for (idx, new_node) in new[(last + 1)..].iter().enumerate() { + let new_idx = idx + last + 1; + let old_index = new_index_to_old_index[new_idx]; + if old_index == u32::MAX as usize { + nodes_created += self.create_node(new_node); + } else { + self.diff_node(&old[old_index], new_node); + nodes_created += self.push_all_nodes(new_node); + } + } + + self.mutations.insert_after( + self.find_last_element(&new[last]).unwrap(), + nodes_created as u32, + ); + nodes_created = 0; + } + + // for each spacing, generate a mount instruction + let mut lis_iter = lis_sequence.iter().rev(); + let mut last = *lis_iter.next().unwrap(); + for next in lis_iter { + if last - next > 1 { + for (idx, new_node) in new[(next + 1)..last].iter().enumerate() { + let new_idx = idx + next + 1; + + let old_index = new_index_to_old_index[new_idx]; + if old_index == u32::MAX as usize { + nodes_created += self.create_node(new_node); + } else { + self.diff_node(&old[old_index], new_node); + nodes_created += self.push_all_nodes(new_node); + } + } + + self.mutations.insert_before( + self.find_first_element(&new[last]).unwrap(), + nodes_created as u32, + ); + + nodes_created = 0; + } + last = *next; + } + + // add mount instruction for the last items not covered by the lis + let first_lis = *lis_sequence.first().unwrap(); + if first_lis > 0 { + for (idx, new_node) in new[..first_lis].iter().enumerate() { + let old_index = new_index_to_old_index[idx]; + if old_index == u32::MAX as usize { + nodes_created += self.create_node(new_node); + } else { + self.diff_node(&old[old_index], new_node); + nodes_created += self.push_all_nodes(new_node); + } + } + + self.mutations.insert_before( + self.find_first_element(&new[first_lis]).unwrap(), + nodes_created as u32, + ); + } + } + + fn replace_node(&mut self, old: &'b VNode<'b>, new: &'b VNode<'b>) { + // fn replace_node(&mut self, old: &'b VNode<'b>, new: &'b VNode<'b>) { + log::debug!("Replacing node {:?}", old); + let nodes_created = self.create_node(new); + self.replace_inner(old, nodes_created); + } + + fn replace_inner(&mut self, old: &'b VNode<'b>, nodes_created: usize) { + match old { + VNode::Element(el) => { + let id = old + .try_mounted_id() + .unwrap_or_else(|| panic!("broke on {:?}", old)); + + self.mutations.replace_with(id, nodes_created as u32); + self.remove_nodes(el.children, false); + self.scopes.collect_garbage(id); + } + + VNode::Text(_) | VNode::Placeholder(_) => { + let id = old + .try_mounted_id() + .unwrap_or_else(|| panic!("broke on {:?}", old)); + + self.mutations.replace_with(id, nodes_created as u32); + self.scopes.collect_garbage(id); + } + + VNode::Fragment(f) => { + self.replace_inner(&f.children[0], nodes_created); + self.remove_nodes(f.children.iter().skip(1), true); + } + + VNode::Component(c) => { + let node = self.scopes.fin_head(c.scope.get().unwrap()); + self.replace_node(node, node); + + let scope_id = c.scope.get().unwrap(); + + // we can only remove components if they are actively being diffed + if self.scope_stack.contains(&c.originator) { + self.scopes.try_remove(scope_id).unwrap(); + } + } + } + } + + pub fn remove_nodes(&mut self, nodes: impl IntoIterator>, gen_muts: bool) { + for node in nodes { + match node { + VNode::Text(t) => { + // this check exists because our null node will be removed but does not have an ID + if let Some(id) = t.id.get() { + self.scopes.collect_garbage(id); + + if gen_muts { + self.mutations.remove(id.as_u64()); + } + } + } + VNode::Placeholder(a) => { + let id = a.id.get().unwrap(); + self.scopes.collect_garbage(id); + + if gen_muts { + self.mutations.remove(id.as_u64()); + } + } + VNode::Element(e) => { + let id = e.id.get().unwrap(); + + if gen_muts { + self.mutations.remove(id.as_u64()); + } + + self.scopes.collect_garbage(id); + + self.remove_nodes(e.children, false); + } + + VNode::Fragment(f) => { + self.remove_nodes(f.children, gen_muts); + } + + VNode::Component(c) => { + let scope_id = c.scope.get().unwrap(); + let root = self.scopes.root_node(scope_id); + self.remove_nodes([root], gen_muts); + + // we can only remove this node if the originator is actively + if self.scope_stack.contains(&c.originator) { + self.scopes.try_remove(scope_id).unwrap(); + } + } + } + } + } + + fn create_children(&mut self, nodes: &'b [VNode<'b>]) -> usize { + let mut created = 0; + for node in nodes { + created += self.create_node(node); + } + created + } + + fn create_and_append_children(&mut self, nodes: &'b [VNode<'b>]) { + let created = self.create_children(nodes); + self.mutations.append_children(created as u32); + } + + fn create_and_insert_after(&mut self, nodes: &'b [VNode<'b>], after: &'b VNode<'b>) { + let created = self.create_children(nodes); + let last = self.find_last_element(after).unwrap(); + self.mutations.insert_after(last, created as u32); + } + + fn create_and_insert_before(&mut self, nodes: &'b [VNode<'b>], before: &'b VNode<'b>) { + let created = self.create_children(nodes); + let first = self.find_first_element(before).unwrap(); + self.mutations.insert_before(first, created as u32); + } + + fn current_scope(&self) -> Option { + self.scope_stack.last().copied() + } + + fn enter_scope(&mut self, scope: ScopeId) { + self.scope_stack.push(scope); + } + + fn leave_scope(&mut self) { + self.scope_stack.pop(); + } + + fn find_last_element(&self, vnode: &'b VNode<'b>) -> Option { + let mut search_node = Some(vnode); + + loop { + match &search_node.take().unwrap() { + VNode::Text(t) => break t.id.get(), + VNode::Element(t) => break t.id.get(), + VNode::Placeholder(t) => break t.id.get(), + VNode::Fragment(frag) => { + search_node = frag.children.last(); + } + VNode::Component(el) => { + let scope_id = el.scope.get().unwrap(); + search_node = Some(self.scopes.root_node(scope_id)); + } + } + } + } + + fn find_first_element(&self, vnode: &'b VNode<'b>) -> Option { + let mut search_node = Some(vnode); + + loop { + match &search_node.take().unwrap() { + // the ones that have a direct id + VNode::Fragment(frag) => { + search_node = Some(&frag.children[0]); + } + VNode::Component(el) => { + let scope_id = el.scope.get().unwrap(); + search_node = Some(self.scopes.root_node(scope_id)); + } + VNode::Text(t) => break t.id.get(), + VNode::Element(t) => break t.id.get(), + VNode::Placeholder(t) => break t.id.get(), + } + } + } + + // fn replace_node(&mut self, old: &'b VNode<'b>, nodes_created: usize) { + // log::debug!("Replacing node {:?}", old); + // match old { + // VNode::Element(el) => { + // let id = old + // .try_mounted_id() + // .unwrap_or_else(|| panic!("broke on {:?}", old)); + + // self.mutations.replace_with(id, nodes_created as u32); + // self.remove_nodes(el.children, false); + // } + + // VNode::Text(_) | VNode::Placeholder(_) => { + // let id = old + // .try_mounted_id() + // .unwrap_or_else(|| panic!("broke on {:?}", old)); + + // self.mutations.replace_with(id, nodes_created as u32); + // } + + // VNode::Fragment(f) => { + // self.replace_node(&f.children[0], nodes_created); + // self.remove_nodes(f.children.iter().skip(1), true); + // } + + // VNode::Component(c) => { + // let node = self.scopes.fin_head(c.scope.get().unwrap()); + // self.replace_node(node, nodes_created); + + // let scope_id = c.scope.get().unwrap(); + + // // we can only remove components if they are actively being diffed + // if self.stack.scope_stack.contains(&c.originator) { + // self.scopes.try_remove(scope_id).unwrap(); + // } + // } + // } + // } + + // /// schedules nodes for garbage collection and pushes "remove" to the mutation stack + // /// remove can happen whenever + // pub(crate) fn remove_nodes( + // &mut self, + // nodes: impl IntoIterator>, + // gen_muts: bool, + // ) { + // // or cache the vec on the diff machine + // for node in nodes { + // log::debug!("removing {:?}", node); + // match node { + // VNode::Text(t) => { + // // this check exists because our null node will be removed but does not have an ID + // if let Some(id) = t.id.get() { + // // self.scopes.collect_garbage(id); + + // if gen_muts { + // self.mutations.remove(id.as_u64()); + // } + // } + // } + // VNode::Placeholder(a) => { + // let id = a.id.get().unwrap(); + // // self.scopes.collect_garbage(id); + + // if gen_muts { + // self.mutations.remove(id.as_u64()); + // } + // } + // VNode::Element(e) => { + // let id = e.id.get().unwrap(); + + // if gen_muts { + // self.mutations.remove(id.as_u64()); + // } + + // self.remove_nodes(e.children, false); + + // // self.scopes.collect_garbage(id); + // } + + // VNode::Fragment(f) => { + // self.remove_nodes(f.children, gen_muts); + // } + + // VNode::Component(c) => { + // let scope_id = c.scope.get().unwrap(); + // let root = self.scopes.root_node(scope_id); + // self.remove_nodes(Some(root), gen_muts); + + // // we can only remove this node if the originator is actively + // if self.stack.scope_stack.contains(&c.originator) { + // self.scopes.try_remove(scope_id).unwrap(); + // } + // } + // } + // } + // } + + // recursively push all the nodes of a tree onto the stack and return how many are there + fn push_all_nodes(&mut self, node: &'b VNode<'b>) -> usize { + match node { + VNode::Text(_) | VNode::Placeholder(_) => { + self.mutations.push_root(node.mounted_id()); + 1 + } + + VNode::Fragment(_) | VNode::Component(_) => { + // + let mut added = 0; + for child in node.children() { + added += self.push_all_nodes(child); + } + added + } + + VNode::Element(el) => { + let mut num_on_stack = 0; + for child in el.children.iter() { + num_on_stack += self.push_all_nodes(child); + } + self.mutations.push_root(el.id.get().unwrap()); + + num_on_stack + 1 + } + } + } +} diff --git a/packages/core/src/lib.rs b/packages/core/src/lib.rs index dba4effbbc..48a8a35056 100644 --- a/packages/core/src/lib.rs +++ b/packages/core/src/lib.rs @@ -1,7 +1,8 @@ #![allow(non_snake_case)] #![doc = include_str!("../README.md")] -pub(crate) mod diff; +// pub(crate) mod diff; +pub(crate) mod diff_async; pub(crate) mod events; pub(crate) mod lazynodes; pub(crate) mod mutations; @@ -12,7 +13,8 @@ pub(crate) mod util; pub(crate) mod virtual_dom; pub(crate) mod innerlude { - pub(crate) use crate::diff::*; + pub(crate) use crate::diff_async::*; + // pub(crate) use crate::diff::*; pub use crate::events::*; pub use crate::lazynodes::*; pub use crate::mutations::*; diff --git a/packages/core/src/mutations.rs b/packages/core/src/mutations.rs index fa96edd1b7..bd94d0b71c 100644 --- a/packages/core/src/mutations.rs +++ b/packages/core/src/mutations.rs @@ -143,6 +143,10 @@ impl<'a> Mutations<'a> { self.edits.push(InsertBefore { n, root }); } + pub(crate) fn append_children(&mut self, n: u32) { + self.edits.push(AppendChildren { many: n }); + } + // Remove Nodes from the dom pub(crate) fn remove(&mut self, id: u64) { self.edits.push(Remove { root: id }); @@ -217,6 +221,10 @@ impl<'a> Mutations<'a> { let name = attribute.name; self.edits.push(RemoveAttribute { name, root }); } + + pub(crate) fn mark_dirty_scope(&mut self, scope: ScopeId) { + self.dirty_scopes.insert(scope); + } } // refs are only assigned once diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 64da46279d..74234a84c8 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -175,6 +175,7 @@ impl Debug for VNode<'_> { .field("key", &el.key) .field("attrs", &el.attributes) .field("children", &el.children) + .field("id", &el.id) .finish(), VNode::Text(t) => write!(s, "VNode::VText {{ text: {} }}", t.text), VNode::Placeholder(t) => write!(s, "VNode::VPlaceholder {{ id: {:?} }}", t.id), @@ -365,9 +366,7 @@ pub struct EventHandler<'bump, T = ()> { impl<'a, T> Default for EventHandler<'a, T> { fn default() -> Self { - Self { - callback: RefCell::new(None), - } + Self { callback: RefCell::new(None) } } } @@ -441,10 +440,7 @@ pub struct NodeFactory<'a> { impl<'a> NodeFactory<'a> { pub fn new(scope: &'a ScopeState) -> NodeFactory<'a> { - NodeFactory { - scope, - bump: &scope.wip_frame().bump, - } + NodeFactory { scope, bump: &scope.wip_frame().bump } } #[inline] @@ -454,11 +450,10 @@ impl<'a> NodeFactory<'a> { /// Directly pass in text blocks without the need to use the format_args macro. pub fn static_text(&self, text: &'static str) -> VNode<'a> { - VNode::Text(self.bump.alloc(VText { - id: empty_cell(), - text, - is_static: true, - })) + VNode::Text( + self.bump + .alloc(VText { id: empty_cell(), text, is_static: true }), + ) } /// Parses a lazy text Arguments and returns a string and a flag indicating if the text is 'static @@ -481,11 +476,7 @@ impl<'a> NodeFactory<'a> { pub fn text(&self, args: Arguments) -> VNode<'a> { let (text, is_static) = self.raw_text(args); - VNode::Text(self.bump.alloc(VText { - text, - is_static, - id: empty_cell(), - })) + VNode::Text(self.bump.alloc(VText { text, is_static, id: empty_cell() })) } pub fn element( @@ -543,13 +534,7 @@ impl<'a> NodeFactory<'a> { is_volatile: bool, ) -> Attribute<'a> { let (value, is_static) = self.raw_text(val); - Attribute { - name, - value, - is_static, - namespace, - is_volatile, - } + Attribute { name, value, is_static, namespace, is_volatile } } pub fn component

( @@ -591,11 +576,7 @@ impl<'a> NodeFactory<'a> { } pub fn listener(self, event: &'static str, callback: InternalHandler<'a>) -> Listener<'a> { - Listener { - event, - mounted_node: Cell::new(None), - callback, - } + Listener { event, mounted_node: Cell::new(None), callback } } pub fn fragment_root<'b, 'c>( @@ -611,10 +592,10 @@ impl<'a> NodeFactory<'a> { if nodes.is_empty() { VNode::Placeholder(self.bump.alloc(VPlaceholder { id: empty_cell() })) } else { - VNode::Fragment(self.bump.alloc(VFragment { - children: nodes.into_bump_slice(), - key: None, - })) + VNode::Fragment( + self.bump + .alloc(VFragment { children: nodes.into_bump_slice(), key: None }), + ) } } @@ -650,10 +631,7 @@ impl<'a> NodeFactory<'a> { ); } - VNode::Fragment(self.bump.alloc(VFragment { - children, - key: None, - })) + VNode::Fragment(self.bump.alloc(VFragment { children, key: None })) } } @@ -676,10 +654,9 @@ impl<'a> NodeFactory<'a> { } else { let children = nodes.into_bump_slice(); - Some(VNode::Fragment(self.bump.alloc(VFragment { - children, - key: None, - }))) + Some(VNode::Fragment( + self.bump.alloc(VFragment { children, key: None }), + )) } } diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index d8cfd6505a..542afc6739 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -203,7 +203,8 @@ impl ScopeArena { } pub fn collect_garbage(&self, id: ElementId) { - log::debug!("collecting garbage for {:?}", id); + // println!("collecting garbage for {:?}", id); + // log::debug!("collecting garbage for {:?}", id); self.nodes.borrow_mut().remove(id.0); } @@ -304,11 +305,9 @@ impl ScopeArena { frame.node.set(unsafe { extend_vnode(node) }); } else { let frame = scope.wip_frame(); - let node = frame - .bump - .alloc(VNode::Placeholder(frame.bump.alloc(VPlaceholder { - id: Default::default(), - }))); + let node = frame.bump.alloc(VNode::Placeholder( + frame.bump.alloc(VPlaceholder { id: Default::default() }), + )); frame.node.set(unsafe { extend_vnode(node) }); } @@ -418,10 +417,7 @@ pub struct Scope<'a, P = ()> { impl

Copy for Scope<'_, P> {} impl

Clone for Scope<'_, P> { fn clone(&self) -> Self { - Self { - scope: self.scope, - props: self.props, - } + Self { scope: self.scope, props: self.props } } } @@ -784,10 +780,7 @@ impl ScopeState { /// } ///``` pub fn render<'src>(&'src self, rsx: LazyNodes<'src, '_>) -> Option> { - Some(rsx.call(NodeFactory { - scope: self, - bump: &self.wip_frame().bump, - })) + Some(rsx.call(NodeFactory { scope: self, bump: &self.wip_frame().bump })) } /// Store a value between renders @@ -894,10 +887,7 @@ impl ScopeState { self.shared_contexts.get_mut().clear(); // next: reset the node data - let SelfReferentialItems { - borrowed_props, - listeners, - } = self.items.get_mut(); + let SelfReferentialItems { borrowed_props, listeners } = self.items.get_mut(); borrowed_props.clear(); listeners.clear(); self.frames[0].reset(); @@ -955,11 +945,7 @@ pub(crate) struct TaskQueue { pub(crate) type InnerTask = Pin>>; impl TaskQueue { fn new(sender: UnboundedSender) -> Rc { - Rc::new(Self { - tasks: RefCell::new(FxHashMap::default()), - gen: Cell::new(0), - sender, - }) + Rc::new(Self { tasks: RefCell::new(FxHashMap::default()), gen: Cell::new(0), sender }) } fn push_fut(&self, task: impl Future + 'static) -> TaskId { let pinned = Box::pin(task); diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 1f443db864..d879e0340b 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -2,6 +2,7 @@ //! //! This module provides the primary mechanics to create a hook-based, concurrent VDOM for Rust. +use crate::diff_async::AsyncDiffState as DiffState; use crate::innerlude::*; use futures_channel::mpsc::{UnboundedReceiver, UnboundedSender}; use futures_util::{future::poll_fn, StreamExt}; @@ -453,7 +454,7 @@ impl VirtualDom { while !self.dirty_scopes.is_empty() { let scopes = &self.scopes; - let mut diff_state = DiffState::new(scopes); + let mut diff_state = AsyncDiffState::new(scopes); let mut ran_scopes = FxHashSet::default(); @@ -473,31 +474,32 @@ impl VirtualDom { self.scopes.run_scope(scopeid); - let (old, new) = (self.scopes.wip_head(scopeid), self.scopes.fin_head(scopeid)); - diff_state.stack.push(DiffInstruction::Diff { new, old }); + diff_state.diff_scope(scopeid); - log::debug!("pushing scope {:?} onto scope stack", scopeid); - diff_state.stack.scope_stack.push(scopeid); + let AsyncDiffState { mutations, .. } = diff_state; - let scope = scopes.get_scope(scopeid).unwrap(); - diff_state.stack.element_stack.push(scope.container); - } - } + for scope in &mutations.dirty_scopes { + self.dirty_scopes.remove(scope); + } - if diff_state.work(&mut deadline) { - let DiffState { mutations, .. } = diff_state; + committed_mutations.push(mutations); - for scope in &mutations.dirty_scopes { - self.dirty_scopes.remove(scope); - } + // if diff_state.work(&mut deadline) { + // let DiffState { mutations, .. } = diff_state; + + // for scope in &mutations.dirty_scopes { + // self.dirty_scopes.remove(scope); + // } - committed_mutations.push(mutations); - } else { - // leave the work in an incomplete state - // - // todo: we should store the edits and re-apply them later - // for now, we just dump the work completely (threadsafe) - return committed_mutations; + // committed_mutations.push(mutations); + // } else { + // // leave the work in an incomplete state + // // + // // todo: we should store the edits and re-apply them later + // // for now, we just dump the work completely (threadsafe) + // return committed_mutations; + // } + } } } @@ -526,13 +528,15 @@ impl VirtualDom { let mut diff_state = DiffState::new(&self.scopes); self.scopes.run_scope(scope_id); - diff_state - .stack - .create_node(self.scopes.fin_head(scope_id), MountType::Append); - diff_state.stack.element_stack.push(ElementId(0)); - diff_state.stack.scope_stack.push(scope_id); - diff_state.work(|| false); + diff_state.element_stack.push(ElementId(0)); + diff_state.scope_stack.push(scope_id); + + let node = self.scopes.fin_head(scope_id); + let created = diff_state.create_node(node); + + diff_state.mutations.append_children(created as u32); + self.dirty_scopes.clear(); assert!(self.dirty_scopes.is_empty()); @@ -579,12 +583,11 @@ impl VirtualDom { ); diff_machine.force_diff = true; - diff_machine.stack.push(DiffInstruction::Diff { old, new }); - diff_machine.stack.scope_stack.push(scope_id); - + diff_machine.scope_stack.push(scope_id); let scope = diff_machine.scopes.get_scope(scope_id).unwrap(); - diff_machine.stack.element_stack.push(scope.container); - diff_machine.work(|| false); + diff_machine.element_stack.push(scope.container); + + diff_machine.diff_node(old, new); diff_machine.mutations } @@ -623,10 +626,10 @@ impl VirtualDom { /// ``` pub fn diff_vnodes<'a>(&'a self, old: &'a VNode<'a>, new: &'a VNode<'a>) -> Mutations<'a> { let mut machine = DiffState::new(&self.scopes); - machine.stack.push(DiffInstruction::Diff { new, old }); - machine.stack.element_stack.push(ElementId(0)); - machine.stack.scope_stack.push(ScopeId(0)); - machine.work(|| false); + machine.element_stack.push(ElementId(0)); + machine.scope_stack.push(ScopeId(0)); + machine.diff_node(old, new); + machine.mutations } @@ -645,11 +648,11 @@ impl VirtualDom { /// ``` pub fn create_vnodes<'a>(&'a self, nodes: LazyNodes<'a, '_>) -> Mutations<'a> { let mut machine = DiffState::new(&self.scopes); - machine.stack.element_stack.push(ElementId(0)); - machine - .stack - .create_node(self.render_vnodes(nodes), MountType::Append); - machine.work(|| false); + machine.scope_stack.push(ScopeId(0)); + machine.element_stack.push(ElementId(0)); + let node = self.render_vnodes(nodes); + let created = machine.create_node(node); + machine.mutations.append_children(created as u32); machine.mutations } @@ -674,16 +677,15 @@ impl VirtualDom { let (old, new) = (self.render_vnodes(left), self.render_vnodes(right)); let mut create = DiffState::new(&self.scopes); - create.stack.scope_stack.push(ScopeId(0)); - create.stack.element_stack.push(ElementId(0)); - create.stack.create_node(old, MountType::Append); - create.work(|| false); + create.scope_stack.push(ScopeId(0)); + create.element_stack.push(ElementId(0)); + let created = create.create_node(old); + create.mutations.append_children(created as u32); let mut edit = DiffState::new(&self.scopes); - edit.stack.scope_stack.push(ScopeId(0)); - edit.stack.element_stack.push(ElementId(0)); - edit.stack.push(DiffInstruction::Diff { old, new }); - edit.work(|| false); + edit.scope_stack.push(ScopeId(0)); + edit.element_stack.push(ElementId(0)); + edit.diff_node(old, new); (create.mutations, edit.mutations) } diff --git a/packages/core/tests/diffing.rs b/packages/core/tests/diffing.rs index 5b98ea80e2..c407359c9e 100644 --- a/packages/core/tests/diffing.rs +++ b/packages/core/tests/diffing.rs @@ -1,4 +1,5 @@ #![allow(unused, non_upper_case_globals)] + //! Diffing Tests //! //! These tests only verify that the diffing algorithm works properly for single components. @@ -31,26 +32,14 @@ fn html_and_rsx_generate_the_same_output() { assert_eq!( create.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateTextNode { - root: 2, - text: "Hello world" - }, + CreateElement { root: 1, tag: "div" }, + CreateTextNode { root: 2, text: "Hello world" }, AppendChildren { many: 1 }, AppendChildren { many: 1 }, ] ); - assert_eq!( - change.edits, - [SetText { - text: "Goodbye world", - root: 2 - },] - ); + assert_eq!(change.edits, [SetText { text: "Goodbye world", root: 2 },]); } /// Should result in 3 elements on the stack @@ -67,32 +56,14 @@ fn fragments_create_properly() { assert_eq!( create.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateTextNode { - root: 2, - text: "Hello a" - }, + CreateElement { root: 1, tag: "div" }, + CreateTextNode { root: 2, text: "Hello a" }, AppendChildren { many: 1 }, - CreateElement { - root: 3, - tag: "div" - }, - CreateTextNode { - root: 4, - text: "Hello b" - }, + CreateElement { root: 3, tag: "div" }, + CreateTextNode { root: 4, text: "Hello b" }, AppendChildren { many: 1 }, - CreateElement { - root: 5, - tag: "div" - }, - CreateTextNode { - root: 6, - text: "Hello c" - }, + CreateElement { root: 5, tag: "div" }, + CreateTextNode { root: 6, text: "Hello c" }, AppendChildren { many: 1 }, AppendChildren { many: 3 }, ] @@ -116,10 +87,7 @@ fn empty_fragments_create_anchors() { assert_eq!( change.edits, [ - CreateElement { - root: 2, - tag: "div" - }, + CreateElement { root: 2, tag: "div" }, ReplaceWith { m: 1, root: 1 } ] ); @@ -138,29 +106,15 @@ fn empty_fragments_create_many_anchors() { create.edits, [CreatePlaceholder { root: 1 }, AppendChildren { many: 1 }] ); + assert_eq!( change.edits, [ - CreateElement { - root: 2, - tag: "div" - }, - CreateElement { - root: 3, - tag: "div" - }, - CreateElement { - root: 4, - tag: "div" - }, - CreateElement { - root: 5, - tag: "div" - }, - CreateElement { - root: 6, - tag: "div" - }, + CreateElement { root: 2, tag: "div" }, + CreateElement { root: 3, tag: "div" }, + CreateElement { root: 4, tag: "div" }, + CreateElement { root: 5, tag: "div" }, + CreateElement { root: 6, tag: "div" }, ReplaceWith { m: 5, root: 1 } ] ); @@ -188,32 +142,14 @@ fn empty_fragments_create_anchors_with_many_children() { assert_eq!( change.edits, [ - CreateElement { - tag: "div", - root: 2, - }, - CreateTextNode { - text: "hello: 0", - root: 3 - }, + CreateElement { tag: "div", root: 2 }, + CreateTextNode { text: "hello: 0", root: 3 }, AppendChildren { many: 1 }, - CreateElement { - tag: "div", - root: 4, - }, - CreateTextNode { - text: "hello: 1", - root: 5 - }, + CreateElement { tag: "div", root: 4 }, + CreateTextNode { text: "hello: 1", root: 5 }, AppendChildren { many: 1 }, - CreateElement { - tag: "div", - root: 6, - }, - CreateTextNode { - text: "hello: 2", - root: 7 - }, + CreateElement { tag: "div", root: 6 }, + CreateTextNode { text: "hello: 2", root: 7 }, AppendChildren { many: 1 }, ReplaceWith { root: 1, m: 3 } ] @@ -236,23 +172,11 @@ fn many_items_become_fragment() { assert_eq!( create.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateTextNode { - text: "hello", - root: 2 - }, + CreateElement { root: 1, tag: "div" }, + CreateTextNode { text: "hello", root: 2 }, AppendChildren { many: 1 }, - CreateElement { - root: 3, - tag: "div" - }, - CreateTextNode { - text: "hello", - root: 4 - }, + CreateElement { root: 3, tag: "div" }, + CreateTextNode { text: "hello", root: 4 }, AppendChildren { many: 1 }, AppendChildren { many: 2 }, ] @@ -315,7 +239,7 @@ fn two_fragments_with_differrent_elements_are_differet() { // replace the divs with new h1s CreateElement { tag: "h1", root: 7 }, ReplaceWith { root: 1, m: 1 }, - CreateElement { tag: "h1", root: 8 }, + CreateElement { tag: "h1", root: 1 }, // notice how 1 gets re-used ReplaceWith { root: 2, m: 1 }, ] ); @@ -339,39 +263,27 @@ fn two_fragments_with_differrent_elements_are_differet_shorter() { assert_eq!( create.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateElement { - root: 2, - tag: "div" - }, - CreateElement { - root: 3, - tag: "div" - }, - CreateElement { - root: 4, - tag: "div" - }, - CreateElement { - root: 5, - tag: "div" - }, + CreateElement { root: 1, tag: "div" }, + CreateElement { root: 2, tag: "div" }, + CreateElement { root: 3, tag: "div" }, + CreateElement { root: 4, tag: "div" }, + CreateElement { root: 5, tag: "div" }, CreateElement { root: 6, tag: "p" }, AppendChildren { many: 6 }, ] ); + + // note: key reuse is always the last node that got used + // slab maintains a linked list, essentially assert_eq!( change.edits, [ Remove { root: 3 }, Remove { root: 4 }, Remove { root: 5 }, - CreateElement { root: 7, tag: "h1" }, - ReplaceWith { root: 1, m: 1 }, - CreateElement { root: 8, tag: "h1" }, + CreateElement { root: 5, tag: "h1" }, // 3 gets reused + ReplaceWith { root: 1, m: 1 }, // 1 gets deleted + CreateElement { root: 1, tag: "h1" }, // 1 gets reused ReplaceWith { root: 2, m: 1 }, ] ); @@ -395,14 +307,8 @@ fn two_fragments_with_same_elements_are_differet() { assert_eq!( create.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateElement { - root: 2, - tag: "div" - }, + CreateElement { root: 1, tag: "div" }, + CreateElement { root: 2, tag: "div" }, CreateElement { root: 3, tag: "p" }, AppendChildren { many: 3 }, ] @@ -410,18 +316,9 @@ fn two_fragments_with_same_elements_are_differet() { assert_eq!( change.edits, [ - CreateElement { - root: 4, - tag: "div" - }, - CreateElement { - root: 5, - tag: "div" - }, - CreateElement { - root: 6, - tag: "div" - }, + CreateElement { root: 4, tag: "div" }, + CreateElement { root: 5, tag: "div" }, + CreateElement { root: 6, tag: "div" }, InsertAfter { root: 2, n: 3 }, ] ); @@ -628,14 +525,8 @@ fn keyed_diffing_additions() { assert_eq!( change.edits, [ - CreateElement { - root: 6, - tag: "div" - }, - CreateElement { - root: 7, - tag: "div" - }, + CreateElement { root: 6, tag: "div" }, + CreateElement { root: 7, tag: "div" }, InsertAfter { n: 2, root: 5 } ] ); @@ -663,14 +554,8 @@ fn keyed_diffing_additions_and_moves_on_ends() { change.edits, [ // create 11, 12 - CreateElement { - tag: "div", - root: 5 - }, - CreateElement { - tag: "div", - root: 6 - }, + CreateElement { tag: "div", root: 5 }, + CreateElement { tag: "div", root: 6 }, InsertAfter { root: 3, n: 2 }, // move 7 to the front PushRoot { root: 4 }, @@ -684,43 +569,32 @@ fn keyed_diffing_additions_and_moves_in_middle() { let dom = new_dom(); let left = rsx!({ - [/**/ 4, 5, 6, 7 /**/].iter().map(|f| { + [/**/ 1, 2, 3, 4 /**/].iter().map(|f| { rsx! { div { key: "{f}" }} }) }); let right = rsx!({ - [/**/ 7, 4, 13, 17, 5, 11, 12, 6 /**/].iter().map(|f| { + [/**/ 4, 1, 7, 8, 2, 5, 6, 3 /**/].iter().map(|f| { rsx! { div { key: "{f}" }} }) }); + // LIS: 4, 5, 6 let (_, change) = dom.diff_lazynodes(left, right); log::debug!("{:#?}", change); assert_eq!( change.edits, [ - // create 13, 17 - CreateElement { - tag: "div", - root: 5 - }, - CreateElement { - tag: "div", - root: 6 - }, - InsertBefore { root: 2, n: 2 }, - // create 11, 12 - CreateElement { - tag: "div", - root: 7 - }, - CreateElement { - tag: "div", - root: 8 - }, + // create 5, 6 + CreateElement { tag: "div", root: 5 }, + CreateElement { tag: "div", root: 6 }, InsertBefore { root: 3, n: 2 }, + // create 7, 8 + CreateElement { tag: "div", root: 7 }, + CreateElement { tag: "div", root: 8 }, + InsertBefore { root: 2, n: 2 }, // move 7 PushRoot { root: 4 }, InsertBefore { root: 1, n: 1 } @@ -756,16 +630,10 @@ fn controlled_keyed_diffing_out_of_order() { // remove 7 // create 9 and insert before 6 - CreateElement { - root: 5, - tag: "div" - }, + CreateElement { root: 5, tag: "div" }, InsertBefore { n: 1, root: 3 }, // create 0 and insert before 5 - CreateElement { - root: 6, - tag: "div" - }, + CreateElement { root: 6, tag: "div" }, InsertBefore { n: 1, root: 2 }, ] ); @@ -792,10 +660,7 @@ fn controlled_keyed_diffing_out_of_order_max_test() { assert_eq!( changes.edits, [ - CreateElement { - root: 6, - tag: "div" - }, + CreateElement { root: 6, tag: "div" }, InsertBefore { n: 1, root: 3 }, PushRoot { root: 4 }, InsertBefore { n: 1, root: 1 }, From 1ea42799c0f63a34f4eb1208bd78b2c165eb920e Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Sun, 30 Jan 2022 18:34:24 -0500 Subject: [PATCH 03/14] wip: clean up the core crate after switching to recursive diff engine --- packages/core/src/diff.rs | 996 +++++++++----------------- packages/core/src/diff_async.rs | 1102 ----------------------------- packages/core/src/lib.rs | 6 +- packages/core/src/scopes.rs | 3 +- packages/core/src/virtual_dom.rs | 2 +- packages/core/tests/earlyabort.rs | 20 +- packages/core/tests/lifecycle.rs | 119 +--- packages/core/tests/passthru.rs | 108 +++ 8 files changed, 475 insertions(+), 1881 deletions(-) delete mode 100644 packages/core/src/diff_async.rs create mode 100644 packages/core/tests/passthru.rs diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 3b9c7692cc..d790292c84 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -1,342 +1,108 @@ -//! This module contains the stateful DiffState and all methods to diff VNodes, their properties, and their children. -//! -//! The [`DiffState`] calculates the diffs between the old and new frames, updates the new nodes, and generates a set -//! of mutations for the RealDom to apply. -//! -//! ## Notice: -//! -//! The inspiration and code for this module was originally taken from Dodrio (@fitzgen) and then modified to support -//! Components, Fragments, Suspense, SubTree memoization, incremental diffing, cancellation, NodeRefs, pausing, priority -//! scheduling, and additional batching operations. -//! -//! ## Implementation Details: -//! -//! ### IDs for elements -//! -------------------- -//! All nodes are addressed by their IDs. The RealDom provides an imperative interface for making changes to these nodes. -//! We don't necessarily require that DOM changes happen instantly during the diffing process, so the implementor may choose -//! to batch nodes if it is more performant for their application. The element IDs are indices into the internal element -//! array. The expectation is that implementors will use the ID as an index into a Vec of real nodes, allowing for passive -//! garbage collection as the VirtualDOM replaces old nodes. -//! -//! When new vnodes are created through `cx.render`, they won't know which real node they correspond to. During diffing, -//! we always make sure to copy over the ID. If we don't do this properly, the ElementId will be populated incorrectly -//! and brick the user's page. -//! -//! ### Fragment Support -//! -------------------- -//! Fragments (nodes without a parent) are supported through a combination of "replace with" and anchor vnodes. Fragments -//! can be particularly challenging when they are empty, so the anchor node lets us "reserve" a spot for the empty -//! fragment to be replaced with when it is no longer empty. This is guaranteed by logic in the NodeFactory - it is -//! impossible to craft a fragment with 0 elements - they must always have at least a single placeholder element. Adding -//! "dummy" nodes _is_ inefficient, but it makes our diffing algorithm faster and the implementation is completely up to -//! the platform. -//! -//! Other implementations either don't support fragments or use a "child + sibling" pattern to represent them. Our code is -//! vastly simpler and more performant when we can just create a placeholder element while the fragment has no children. -//! -//! ### Suspense -//! ------------ -//! Dioxus implements Suspense slightly differently than React. In React, each fiber is manually progressed until it runs -//! into a promise-like value. React will then work on the next "ready" fiber, checking back on the previous fiber once -//! it has finished its new work. In Dioxus, we use a similar approach, but try to completely render the tree before -//! switching sub-fibers. Instead, each future is submitted into a futures-queue and the node is manually loaded later on. -//! Due to the frequent calls to "yield_now" we can get the pure "fetch-as-you-render" behavior of React Fiber. -//! -//! We're able to use this approach because we use placeholder nodes - futures that aren't ready still get submitted to -//! DOM, but as a placeholder. -//! -//! Right now, the "suspense" queue is intertwined with hooks. In the future, we should allow any future to drive attributes -//! and contents, without the need for the "use_suspense" hook. In the interim, this is the quickest way to get Suspense working. -//! -//! ## Subtree Memoization -//! ----------------------- -//! We also employ "subtree memoization" which saves us from having to check trees which hold no dynamic content. We can -//! detect if a subtree is "static" by checking if its children are "static". Since we dive into the tree depth-first, the -//! calls to "create" propagate this information upwards. Structures like the one below are entirely static: -//! ```rust, ignore -//! rsx!( div { class: "hello world", "this node is entirely static" } ) -//! ``` -//! Because the subtrees won't be diffed, their "real node" data will be stale (invalid), so it's up to the reconciler to -//! track nodes created in a scope and clean up all relevant data. Support for this is currently WIP and depends on comp-time -//! hashing of the subtree from the rsx! macro. We do a very limited form of static analysis via static string pointers as -//! a way of short-circuiting the most expensive checks. -//! -//! ## Bloom Filter and Heuristics -//! ------------------------------ -//! For all components, we employ some basic heuristics to speed up allocations and pre-size bump arenas. The heuristics are -//! currently very rough, but will get better as time goes on. The information currently tracked includes the size of a -//! bump arena after first render, the number of hooks, and the number of nodes in the tree. -//! -//! ## Garbage Collection -//! --------------------- -//! Dioxus uses a passive garbage collection system to clean up old nodes once the work has been completed. This garbage -//! collection is done internally once the main diffing work is complete. After the "garbage" is collected, Dioxus will then -//! start to re-use old keys for new nodes. This results in a passive memory management system that is very efficient. -//! -//! The IDs used by the key/map are just an index into a Vec. This means that Dioxus will drive the key allocation strategy -//! so the client only needs to maintain a simple list of nodes. By default, Dioxus will not manually clean up old nodes -//! for the client. As new nodes are created, old nodes will be over-written. -//! -//! ## Further Reading and Thoughts -//! ---------------------------- -//! There are more ways of increasing diff performance here that are currently not implemented. -//! - Strong memoization of subtrees. -//! - Guided diffing. -//! - Certain web-dom-specific optimizations. -//! -//! More info on how to improve this diffing algorithm: -//! - - use crate::innerlude::*; use fxhash::{FxHashMap, FxHashSet}; use smallvec::{smallvec, SmallVec}; -use DomEdit::*; - -/// Our DiffState is an iterative tree differ. -/// -/// It uses techniques of a stack machine to allow pausing and restarting of the diff algorithm. This -/// was originally implemented using recursive techniques, but Rust lacks the ability to call async functions recursively, -/// meaning we could not "pause" the original diffing algorithm. -/// -/// Instead, we use a traditional stack machine approach to diff and create new nodes. The diff algorithm periodically -/// calls "yield_now" which allows the machine to pause and return control to the caller. The caller can then wait for -/// the next period of idle time, preventing our diff algorithm from blocking the main thread. -/// -/// Funnily enough, this stack machine's entire job is to create instructions for another stack machine to execute. It's -/// stack machines all the way down! -pub(crate) struct DiffState<'bump> { + +pub(crate) struct AsyncDiffState<'bump> { pub(crate) scopes: &'bump ScopeArena, pub(crate) mutations: Mutations<'bump>, - pub(crate) stack: DiffStack<'bump>, pub(crate) force_diff: bool, + pub(crate) element_stack: SmallVec<[ElementId; 10]>, + pub(crate) scope_stack: SmallVec<[ScopeId; 5]>, } -impl<'bump> DiffState<'bump> { - pub(crate) fn new(scopes: &'bump ScopeArena) -> Self { +impl<'b> AsyncDiffState<'b> { + pub fn new(scopes: &'b ScopeArena) -> Self { Self { scopes, mutations: Mutations::new(), - stack: DiffStack::new(), force_diff: false, - } - } -} - -/// The stack instructions we use to diff and create new nodes. -#[derive(Debug)] -pub(crate) enum DiffInstruction<'a> { - Diff { - old: &'a VNode<'a>, - new: &'a VNode<'a>, - }, - - Create { - node: &'a VNode<'a>, - }, - - /// pushes the node elements onto the stack for use in mount - PrepareMove { - node: &'a VNode<'a>, - }, - - Mount { - and: MountType<'a>, - }, - - PopScope, - PopElement, -} - -#[derive(Debug, Clone, Copy)] -pub(crate) enum MountType<'a> { - Absorb, - Append, - Replace { old: &'a VNode<'a> }, - InsertAfter { other_node: &'a VNode<'a> }, - InsertBefore { other_node: &'a VNode<'a> }, -} - -pub(crate) struct DiffStack<'bump> { - pub(crate) instructions: Vec>, - pub(crate) nodes_created_stack: SmallVec<[usize; 10]>, - pub(crate) scope_stack: SmallVec<[ScopeId; 5]>, - pub(crate) element_stack: SmallVec<[ElementId; 10]>, -} - -impl<'bump> DiffStack<'bump> { - fn new() -> Self { - Self { - instructions: Vec::with_capacity(1000), - nodes_created_stack: smallvec![], - scope_stack: smallvec![], element_stack: smallvec![], + scope_stack: smallvec![], } } - fn pop(&mut self) -> Option> { - self.instructions.pop() - } - - fn pop_off_scope(&mut self) { - self.scope_stack.pop(); - } - - pub(crate) fn push(&mut self, instruction: DiffInstruction<'bump>) { - self.instructions.push(instruction) - } - - fn create_children(&mut self, children: &'bump [VNode<'bump>], and: MountType<'bump>) { - self.nodes_created_stack.push(0); - self.instructions.push(DiffInstruction::Mount { and }); - - for child in children.iter().rev() { - self.instructions - .push(DiffInstruction::Create { node: child }); - } - } - - // todo: subtrees - // fn push_subtree(&mut self) { - // self.nodes_created_stack.push(0); - // self.instructions.push(DiffInstruction::Mount { - // and: MountType::Append, - // }); - // } - - fn push_nodes_created(&mut self, count: usize) { - self.nodes_created_stack.push(count); - } - - pub(crate) fn create_node(&mut self, node: &'bump VNode<'bump>, and: MountType<'bump>) { - self.nodes_created_stack.push(0); - self.instructions.push(DiffInstruction::Mount { and }); - self.instructions.push(DiffInstruction::Create { node }); - } - - fn add_child_count(&mut self, count: usize) { - *self.nodes_created_stack.last_mut().unwrap() += count; - } - - fn pop_nodes_created(&mut self) -> usize { - self.nodes_created_stack.pop().unwrap() - } - - pub fn current_scope(&self) -> Option { - self.scope_stack.last().copied() - } - - fn create_component(&mut self, idx: ScopeId, node: &'bump VNode<'bump>) { - // Push the new scope onto the stack - self.scope_stack.push(idx); - - self.instructions.push(DiffInstruction::PopScope); - - // Run the creation algorithm with this scope on the stack - // ?? I think we treat components as fragments?? - self.instructions.push(DiffInstruction::Create { node }); + pub fn diff_scope(&mut self, scopeid: ScopeId) { + let (old, new) = (self.scopes.wip_head(scopeid), self.scopes.fin_head(scopeid)); + self.scope_stack.push(scopeid); + let scope = self.scopes.get_scope(scopeid).unwrap(); + self.element_stack.push(scope.container); + self.diff_node(old, new); } -} -impl<'bump> DiffState<'bump> { - /// Progress the diffing for this "fiber" - /// - /// This method implements a depth-first iterative tree traversal. - /// - /// We do depth-first to maintain high cache locality (nodes were originally generated recursively). - /// - /// Returns a `bool` indicating that the work completed properly. - pub fn work(&mut self, mut deadline_expired: impl FnMut() -> bool) -> bool { - while let Some(instruction) = self.stack.pop() { - match instruction { - DiffInstruction::Diff { old, new } => self.diff_node(old, new), - DiffInstruction::Create { node } => self.create_node(node), - DiffInstruction::Mount { and } => self.mount(and), - DiffInstruction::PrepareMove { node } => { - let num_on_stack = self.push_all_nodes(node); - self.stack.add_child_count(num_on_stack); - } - DiffInstruction::PopScope => self.stack.pop_off_scope(), - DiffInstruction::PopElement => { - self.stack.element_stack.pop(); + pub fn diff_node(&mut self, old_node: &'b VNode<'b>, new_node: &'b VNode<'b>) { + use VNode::*; + match (old_node, new_node) { + // Check the most common cases first + // these are *actual* elements, not wrappers around lists + (Text(old), Text(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - text are the sames"); + return; } - }; - - if deadline_expired() { - log::trace!("Deadline expired before we could finish!"); - return false; - } - } - - true - } - // recursively push all the nodes of a tree onto the stack and return how many are there - fn push_all_nodes(&mut self, node: &'bump VNode<'bump>) -> usize { - match node { - VNode::Text(_) | VNode::Placeholder(_) => { - self.mutations.push_root(node.mounted_id()); - 1 - } + if let Some(root) = old.id.get() { + if old.text != new.text { + self.mutations.set_text(new.text, root.as_u64()); + } + self.scopes.update_node(new_node, root); - VNode::Fragment(_) | VNode::Component(_) => { - // - let mut added = 0; - for child in node.children() { - added += self.push_all_nodes(child); + new.id.set(Some(root)); } - added } - VNode::Element(el) => { - let mut num_on_stack = 0; - for child in el.children.iter() { - num_on_stack += self.push_all_nodes(child); + (Placeholder(old), Placeholder(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - placeholder are the sames"); + return; } - self.mutations.push_root(el.id.get().unwrap()); - num_on_stack + 1 + if let Some(root) = old.id.get() { + self.scopes.update_node(new_node, root); + new.id.set(Some(root)) + } } - } - } - fn mount(&mut self, and: MountType<'bump>) { - let nodes_created = self.stack.pop_nodes_created(); - match and { - // add the nodes from this virtual list to the parent - // used by fragments and components - MountType::Absorb => { - self.stack.add_child_count(nodes_created); + (Element(old), Element(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - element are the sames"); + return; + } + self.diff_element_nodes(old, new, old_node, new_node) } - MountType::Replace { old } => { - self.replace_node(old, nodes_created); + // These two sets are pointers to nodes but are not actually nodes themselves + (Component(old), Component(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - placeholder are the sames"); + return; + } + self.diff_component_nodes(old_node, new_node, *old, *new) } - MountType::Append => { - self.mutations.append_children(nodes_created as u32); - // self.mutations.edits.push(AppendChildren { - // many: nodes_created as u32, - // }); + (Fragment(old), Fragment(new)) => { + if std::ptr::eq(old, new) { + log::trace!("skipping node diff - fragment are the sames"); + return; + } + self.diff_fragment_nodes(old, new) } - MountType::InsertAfter { other_node } => { - let root = self.find_last_element(other_node).unwrap(); - self.mutations.insert_after(root, nodes_created as u32); + // The normal pathway still works, but generates slightly weird instructions + // This pathway ensures uses the ReplaceAll, not the InsertAfter and remove + (Placeholder(_), Fragment(_)) => { + log::debug!("replacing placeholder with fragment {:?}", new_node); + self.replace_node(old_node, new_node); } - MountType::InsertBefore { other_node } => { - let root = self.find_first_element_id(other_node).unwrap(); - self.mutations.insert_before(root, nodes_created as u32); - } + // Anything else is just a basic replace and create + ( + Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), + Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), + ) => self.replace_node(old_node, new_node), } } - // ================================= - // Tools for creating new nodes - // ================================= - - fn create_node(&mut self, node: &'bump VNode<'bump>) { + pub fn create_node(&mut self, node: &'b VNode<'b>) -> usize { match node { VNode::Text(vtext) => self.create_text_node(vtext, node), VNode::Placeholder(anchor) => self.create_anchor_node(anchor, node), @@ -346,24 +112,23 @@ impl<'bump> DiffState<'bump> { } } - fn create_text_node(&mut self, vtext: &'bump VText<'bump>, node: &'bump VNode<'bump>) { + fn create_text_node(&mut self, vtext: &'b VText<'b>, node: &'b VNode<'b>) -> usize { let real_id = self.scopes.reserve_node(node); - self.mutations.create_text_node(vtext.text, real_id); vtext.id.set(Some(real_id)); - self.stack.add_child_count(1); + + 1 } - fn create_anchor_node(&mut self, anchor: &'bump VPlaceholder, node: &'bump VNode<'bump>) { + fn create_anchor_node(&mut self, anchor: &'b VPlaceholder, node: &'b VNode<'b>) -> usize { let real_id = self.scopes.reserve_node(node); - self.mutations.create_placeholder(real_id); anchor.id.set(Some(real_id)); - self.stack.add_child_count(1); + 1 } - fn create_element_node(&mut self, element: &'bump VElement<'bump>, node: &'bump VNode<'bump>) { + fn create_element_node(&mut self, element: &'b VElement<'b>, node: &'b VNode<'b>) -> usize { let VElement { tag: tag_name, listeners, @@ -376,21 +141,19 @@ impl<'bump> DiffState<'bump> { } = element; // set the parent ID for event bubbling - self.stack.instructions.push(DiffInstruction::PopElement); + // self.stack.instructions.push(DiffInstruction::PopElement); - let parent = self.stack.element_stack.last().unwrap(); + let parent = self.element_stack.last().unwrap(); parent_id.set(Some(*parent)); // set the id of the element let real_id = self.scopes.reserve_node(node); - self.stack.element_stack.push(real_id); + self.element_stack.push(real_id); dom_id.set(Some(real_id)); self.mutations.create_element(tag_name, *namespace, real_id); - self.stack.add_child_count(1); - - if let Some(cur_scope_id) = self.stack.current_scope() { + if let Some(cur_scope_id) = self.current_scope() { for listener in *listeners { listener.mounted_node.set(Some(real_id)); self.mutations.new_event_listener(listener, cur_scope_id); @@ -403,26 +166,19 @@ impl<'bump> DiffState<'bump> { self.mutations.set_attribute(attr, real_id.as_u64()); } - // todo: the settext optimization - // - // if children.len() == 1 { - // if let VNode::Text(vtext) = children[0] { - // self.mutations.set_text(vtext.text, real_id.as_u64()); - // return; - // } - // } - if !children.is_empty() { - self.stack.create_children(children, MountType::Append); + self.create_and_append_children(children); } + + 1 } - fn create_fragment_node(&mut self, frag: &'bump VFragment<'bump>) { - self.stack.create_children(frag.children, MountType::Absorb); + fn create_fragment_node(&mut self, frag: &'b VFragment<'b>) -> usize { + self.create_children(frag.children) } - fn create_component_node(&mut self, vcomponent: &'bump VComponent<'bump>) { - let parent_idx = self.stack.current_scope().unwrap(); + fn create_component_node(&mut self, vcomponent: &'b VComponent<'b>) -> usize { + let parent_idx = self.current_scope().unwrap(); // the component might already exist - if it does, we need to reuse it // this makes figure out when to drop the component more complicated @@ -431,13 +187,13 @@ impl<'bump> DiffState<'bump> { idx } else { // Insert a new scope into our component list - let props: Box = vcomponent.props.borrow_mut().take().unwrap(); + let props: Box = vcomponent.props.borrow_mut().take().unwrap(); let props: Box = unsafe { std::mem::transmute(props) }; let new_idx = self.scopes.new_with_key( vcomponent.user_fc, props, Some(parent_idx), - self.stack.element_stack.last().copied().unwrap(), + self.element_stack.last().copied().unwrap(), 0, ); @@ -463,104 +219,27 @@ impl<'bump> DiffState<'bump> { } } + self.enter_scope(new_idx); + // Run the scope for one iteration to initialize it self.scopes.run_scope(new_idx); + self.mutations.mark_dirty_scope(new_idx); // Take the node that was just generated from running the component let nextnode = self.scopes.fin_head(new_idx); - self.stack.create_component(new_idx, nextnode); - - // Finally, insert this scope as a seen node. - self.mutations.mark_dirty_scope(new_idx); - // self.mutations.dirty_scopes.insert(new_idx); - } + let created = self.create_node(nextnode); - // ================================= - // Tools for diffing nodes - // ================================= + self.leave_scope(); - pub fn diff_node(&mut self, old_node: &'bump VNode<'bump>, new_node: &'bump VNode<'bump>) { - use VNode::*; - match (old_node, new_node) { - // Check the most common cases first - // these are *actual* elements, not wrappers around lists - (Text(old), Text(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - text are the sames"); - return; - } - - if let Some(root) = old.id.get() { - if old.text != new.text { - self.mutations.set_text(new.text, root.as_u64()); - } - self.scopes.update_node(new_node, root); - - new.id.set(Some(root)); - } - } - - (Placeholder(old), Placeholder(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - placeholder are the sames"); - return; - } - - if let Some(root) = old.id.get() { - self.scopes.update_node(new_node, root); - new.id.set(Some(root)) - } - } - - (Element(old), Element(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - element are the sames"); - return; - } - self.diff_element_nodes(old, new, old_node, new_node) - } - - // These two sets are pointers to nodes but are not actually nodes themselves - (Component(old), Component(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - placeholder are the sames"); - return; - } - self.diff_component_nodes(old_node, new_node, *old, *new) - } - - (Fragment(old), Fragment(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - fragment are the sames"); - return; - } - self.diff_fragment_nodes(old, new) - } - - // The normal pathway still works, but generates slightly weird instructions - // This pathway ensures uses the ReplaceAll, not the InsertAfter and remove - (Placeholder(_), Fragment(new)) => { - log::debug!("replacing placeholder with fragment {:?}", new_node); - self.stack - .create_children(new.children, MountType::Replace { old: old_node }); - } - - // Anything else is just a basic replace and create - ( - Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), - Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), - ) => self - .stack - .create_node(new_node, MountType::Replace { old: old_node }), - } + created } fn diff_element_nodes( &mut self, - old: &'bump VElement<'bump>, - new: &'bump VElement<'bump>, - old_node: &'bump VNode<'bump>, - new_node: &'bump VNode<'bump>, + old: &'b VElement<'b>, + new: &'b VElement<'b>, + old_node: &'b VNode<'b>, + new_node: &'b VNode<'b>, ) { let root = old.id.get().unwrap(); @@ -569,13 +248,7 @@ impl<'bump> DiffState<'bump> { // // This case is rather rare (typically only in non-keyed lists) if new.tag != old.tag || new.namespace != old.namespace { - // maybe make this an instruction? - // issue is that we need the "vnode" but this method only has the velement - self.stack.push_nodes_created(0); - self.stack.push(DiffInstruction::Mount { - and: MountType::Replace { old: old_node }, - }); - self.create_element_node(new, new_node); + self.replace_node(old_node, new_node); return; } @@ -618,7 +291,7 @@ impl<'bump> DiffState<'bump> { // We also need to make sure that all listeners are properly attached to the parent scope (fix_listener) // // TODO: take a more efficient path than this - if let Some(cur_scope_id) = self.stack.current_scope() { + if let Some(cur_scope_id) = self.current_scope() { if old.listeners.len() == new.listeners.len() { for (old_l, new_l) in old.listeners.iter().zip(new.listeners.iter()) { if old_l.event != new_l.event { @@ -640,87 +313,24 @@ impl<'bump> DiffState<'bump> { } } - if old.children.is_empty() && !new.children.is_empty() { - self.mutations.push_root(root); - // self.mutations.edits.push(PushRoot { - // root: root.as_u64(), - // }); - self.stack.element_stack.push(root); - self.stack.instructions.push(DiffInstruction::PopElement); - self.stack.create_children(new.children, MountType::Append); - } else { - self.stack.element_stack.push(root); - self.stack.instructions.push(DiffInstruction::PopElement); - self.diff_children(old.children, new.children); - } - - // todo: this is for the "settext" optimization - // it works, but i'm not sure if it's the direction we want to take right away - // I haven't benchmarked the performance imporvemenet yet. Perhaps - // we can make it a config? - - // match (old.children.len(), new.children.len()) { - // (0, 0) => {} - // (1, 1) => { - // let old1 = &old.children[0]; - // let new1 = &new.children[0]; - - // match (old1, new1) { - // (VNode::Text(old_text), VNode::Text(new_text)) => { - // if old_text.text != new_text.text { - // self.mutations.set_text(new_text.text, root.as_u64()); - // } - // } - // (VNode::Text(_old_text), _) => { - // self.stack.element_stack.push(root); - // self.stack.instructions.push(DiffInstruction::PopElement); - // self.stack.create_node(new1, MountType::Append); - // } - // (_, VNode::Text(new_text)) => { - // self.remove_nodes([old1], false); - // self.mutations.set_text(new_text.text, root.as_u64()); - // } - // _ => { - // self.stack.element_stack.push(root); - // self.stack.instructions.push(DiffInstruction::PopElement); - // self.diff_children(old.children, new.children); - // } - // } - // } - // (0, 1) => { - // if let VNode::Text(text) = &new.children[0] { - // self.mutations.set_text(text.text, root.as_u64()); - // } else { - // self.stack.element_stack.push(root); - // self.stack.instructions.push(DiffInstruction::PopElement); - // } - // } - // (0, _) => { - // self.mutations.edits.push(PushRoot { - // root: root.as_u64(), - // }); - // self.stack.element_stack.push(root); - // self.stack.instructions.push(DiffInstruction::PopElement); - // self.stack.create_children(new.children, MountType::Append); - // } - // (_, 0) => { - // self.remove_nodes(old.children, false); - // self.mutations.set_text("", root.as_u64()); - // } - // (_, _) => { - // self.stack.element_stack.push(root); - // self.stack.instructions.push(DiffInstruction::PopElement); - // self.diff_children(old.children, new.children); - // } - // } + match (old.children.len(), new.children.len()) { + (0, 0) => {} + (0, _) => { + let created = self.create_children(new.children); + self.mutations.append_children(created as u32); + } + (_, _) => { + self.diff_children(old.children, new.children); + } + }; } fn diff_component_nodes( &mut self, - old_node: &'bump VNode<'bump>, - new_node: &'bump VNode<'bump>, - old: &'bump VComponent<'bump>, - new: &'bump VComponent<'bump>, + old_node: &'b VNode<'b>, + new_node: &'b VNode<'b>, + old: &'b VComponent<'b>, + new: &'b VComponent<'b>, ) { let scope_addr = old.scope.get().unwrap(); log::trace!( @@ -736,7 +346,7 @@ impl<'bump> DiffState<'bump> { // Make sure we're dealing with the same component (by function pointer) if old.user_fc == new.user_fc { - self.stack.scope_stack.push(scope_addr); + self.enter_scope(scope_addr); // Make sure the new component vnode is referencing the right scope id new.scope.set(Some(scope_addr)); @@ -786,16 +396,14 @@ impl<'bump> DiffState<'bump> { drop(new_props); }; - self.stack.scope_stack.pop(); + self.leave_scope(); } else { - // - log::debug!("scope stack is {:#?}", self.stack.scope_stack); - self.stack - .create_node(new_node, MountType::Replace { old: old_node }); + log::debug!("scope stack is {:#?}", self.scope_stack); + self.replace_node(old_node, new_node); } } - fn diff_fragment_nodes(&mut self, old: &'bump VFragment<'bump>, new: &'bump VFragment<'bump>) { + fn diff_fragment_nodes(&mut self, old: &'b VFragment<'b>, new: &'b VFragment<'b>) { // This is the case where options or direct vnodes might be used. // In this case, it's faster to just skip ahead to their diff if old.children.len() == 1 && new.children.len() == 1 { @@ -828,15 +436,12 @@ impl<'bump> DiffState<'bump> { // // Fragment nodes cannot generate empty children lists, so we can assume that when a list is empty, it belongs only // to an element, and appending makes sense. - fn diff_children(&mut self, old: &'bump [VNode<'bump>], new: &'bump [VNode<'bump>]) { + fn diff_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { // Remember, fragments can never be empty (they always have a single child) match (old, new) { ([], []) => {} - ([], _) => self.stack.create_children(new, MountType::Append), - (_, []) => { - log::debug!("removing nodes {:?}", old); - self.remove_nodes(old, true) - } + ([], _) => self.create_and_append_children(new), + (_, []) => self.remove_nodes(old, true), _ => { let new_is_keyed = new[0].key().is_some(); let old_is_keyed = old[0].key().is_some(); @@ -867,29 +472,20 @@ impl<'bump> DiffState<'bump> { // [... parent] // // the change list stack is in the same state when this function returns. - fn diff_non_keyed_children(&mut self, old: &'bump [VNode<'bump>], new: &'bump [VNode<'bump>]) { + fn diff_non_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { // Handled these cases in `diff_children` before calling this function. debug_assert!(!new.is_empty()); debug_assert!(!old.is_empty()); - for (new, old) in new.iter().zip(old.iter()).rev() { - self.stack.push(DiffInstruction::Diff { new, old }); - } - use std::cmp::Ordering; match old.len().cmp(&new.len()) { Ordering::Greater => self.remove_nodes(&old[new.len()..], true), - Ordering::Less => { - self.stack.create_children( - &new[old.len()..], - MountType::InsertAfter { - other_node: old.last().unwrap(), - }, - ); - } - Ordering::Equal => { - // nothing - they're the same size - } + Ordering::Less => self.create_and_insert_after(&new[old.len()..], old.last().unwrap()), + Ordering::Equal => {} + } + + for (new, old) in new.iter().zip(old.iter()) { + self.diff_node(old, new); } } @@ -909,10 +505,10 @@ impl<'bump> DiffState<'bump> { // https://github.com/infernojs/inferno/blob/36fd96/packages/inferno/src/DOM/patching.ts#L530-L739 // // The stack is empty upon entry. - fn diff_keyed_children(&mut self, old: &'bump [VNode<'bump>], new: &'bump [VNode<'bump>]) { + fn diff_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { if cfg!(debug_assertions) { let mut keys = fxhash::FxHashSet::default(); - let mut assert_unique_keys = |children: &'bump [VNode<'bump>]| { + let mut assert_unique_keys = |children: &'b [VNode<'b>]| { keys.clear(); for child in children { let key = child.key(); @@ -953,6 +549,7 @@ impl<'bump> DiffState<'bump> { !((old_middle.len() == new_middle.len()) && old_middle.is_empty()), "keyed children must have the same number of children" ); + if new_middle.is_empty() { // remove the old elements self.remove_nodes(old_middle, true); @@ -962,30 +559,15 @@ impl<'bump> DiffState<'bump> { if left_offset == 0 { // insert at the beginning of the old list let foothold = &old[old.len() - right_offset]; - self.stack.create_children( - new_middle, - MountType::InsertBefore { - other_node: foothold, - }, - ); + self.create_and_insert_before(new_middle, foothold); } else if right_offset == 0 { // insert at the end the old list let foothold = old.last().unwrap(); - self.stack.create_children( - new_middle, - MountType::InsertAfter { - other_node: foothold, - }, - ); + self.create_and_insert_after(new_middle, foothold); } else { // inserting in the middle let foothold = &old[left_offset - 1]; - self.stack.create_children( - new_middle, - MountType::InsertAfter { - other_node: foothold, - }, - ); + self.create_and_insert_after(new_middle, foothold); } } else { self.diff_keyed_middle(old_middle, new_middle); @@ -999,9 +581,8 @@ impl<'bump> DiffState<'bump> { /// If there is no offset, then this function returns None and the diffing is complete. fn diff_keyed_ends( &mut self, - - old: &'bump [VNode<'bump>], - new: &'bump [VNode<'bump>], + old: &'b [VNode<'b>], + new: &'b [VNode<'b>], ) -> Option<(usize, usize)> { let mut left_offset = 0; @@ -1010,19 +591,14 @@ impl<'bump> DiffState<'bump> { if old.key() != new.key() { break; } - self.stack.push(DiffInstruction::Diff { old, new }); + self.diff_node(old, new); left_offset += 1; } // If that was all of the old children, then create and append the remaining // new children and we're finished. if left_offset == old.len() { - self.stack.create_children( - &new[left_offset..], - MountType::InsertAfter { - other_node: old.last().unwrap(), - }, - ); + self.create_and_insert_after(&new[left_offset..], old.last().unwrap()); return None; } @@ -1060,7 +636,7 @@ impl<'bump> DiffState<'bump> { // This function will load the appropriate nodes onto the stack and do diffing in place. // // Upon exit from this function, it will be restored to that same self. - fn diff_keyed_middle(&mut self, old: &'bump [VNode<'bump>], new: &'bump [VNode<'bump>]) { + fn diff_keyed_middle(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { /* 1. Map the old keys into a numerical ordering based on indices. 2. Create a map of old key to its index @@ -1118,10 +694,12 @@ impl<'bump> DiffState<'bump> { if shared_keys.is_empty() { if let Some(first_old) = old.get(0) { self.remove_nodes(&old[1..], true); - self.stack - .create_children(new, MountType::Replace { old: first_old }) + let nodes_created = self.create_children(new); + self.replace_inner(first_old, nodes_created); } else { - self.stack.create_children(new, MountType::Append {}); + // I think this is wrong - why are we appending? + // only valid of the if there are no trailing elements + self.create_and_append_children(new); } return; } @@ -1149,33 +727,31 @@ impl<'bump> DiffState<'bump> { lis_sequence.pop(); } - let apply = |new_idx, new_node: &'bump VNode<'bump>, stack: &mut DiffStack<'bump>| { - let old_index = new_index_to_old_index[new_idx]; - if old_index == u32::MAX as usize { - stack.create_node(new_node, MountType::Absorb); - } else { - // this function should never take LIS indices - stack.push(DiffInstruction::PrepareMove { node: new_node }); - stack.push(DiffInstruction::Diff { - new: new_node, - old: &old[old_index], - }); - } - }; + for idx in lis_sequence.iter() { + self.diff_node(&old[new_index_to_old_index[*idx]], &new[*idx]); + } - // add mount instruction for the last items not covered by the lis - let first_lis = *lis_sequence.first().unwrap(); - if first_lis > 0 { - self.stack.push_nodes_created(0); - self.stack.push(DiffInstruction::Mount { - and: MountType::InsertBefore { - other_node: &new[first_lis], - }, - }); - - for (idx, new_node) in new[..first_lis].iter().enumerate().rev() { - apply(idx, new_node, &mut self.stack); + let mut nodes_created = 0; + + // add mount instruction for the first items not covered by the lis + let last = *lis_sequence.last().unwrap(); + if last < (new.len() - 1) { + for (idx, new_node) in new[(last + 1)..].iter().enumerate() { + let new_idx = idx + last + 1; + let old_index = new_index_to_old_index[new_idx]; + if old_index == u32::MAX as usize { + nodes_created += self.create_node(new_node); + } else { + self.diff_node(&old[old_index], new_node); + nodes_created += self.push_all_nodes(new_node); + } } + + self.mutations.insert_after( + self.find_last_element(&new[last]).unwrap(), + nodes_created as u32, + ); + nodes_created = 0; } // for each spacing, generate a mount instruction @@ -1183,86 +759,55 @@ impl<'bump> DiffState<'bump> { let mut last = *lis_iter.next().unwrap(); for next in lis_iter { if last - next > 1 { - self.stack.push_nodes_created(0); - self.stack.push(DiffInstruction::Mount { - and: MountType::InsertBefore { - other_node: &new[last], - }, - }); - for (idx, new_node) in new[(next + 1)..last].iter().enumerate().rev() { - apply(idx + next + 1, new_node, &mut self.stack); + for (idx, new_node) in new[(next + 1)..last].iter().enumerate() { + let new_idx = idx + next + 1; + + let old_index = new_index_to_old_index[new_idx]; + if old_index == u32::MAX as usize { + nodes_created += self.create_node(new_node); + } else { + self.diff_node(&old[old_index], new_node); + nodes_created += self.push_all_nodes(new_node); + } } + + self.mutations.insert_before( + self.find_first_element(&new[last]).unwrap(), + nodes_created as u32, + ); + + nodes_created = 0; } last = *next; } - // add mount instruction for the first items not covered by the lis - let last = *lis_sequence.last().unwrap(); - if last < (new.len() - 1) { - self.stack.push_nodes_created(0); - self.stack.push(DiffInstruction::Mount { - and: MountType::InsertAfter { - other_node: &new[last], - }, - }); - for (idx, new_node) in new[(last + 1)..].iter().enumerate().rev() { - apply(idx + last + 1, new_node, &mut self.stack); + // add mount instruction for the last items not covered by the lis + let first_lis = *lis_sequence.first().unwrap(); + if first_lis > 0 { + for (idx, new_node) in new[..first_lis].iter().enumerate() { + let old_index = new_index_to_old_index[idx]; + if old_index == u32::MAX as usize { + nodes_created += self.create_node(new_node); + } else { + self.diff_node(&old[old_index], new_node); + nodes_created += self.push_all_nodes(new_node); + } } - } - for idx in lis_sequence.iter().rev() { - self.stack.push(DiffInstruction::Diff { - new: &new[*idx], - old: &old[new_index_to_old_index[*idx]], - }); + self.mutations.insert_before( + self.find_first_element(&new[first_lis]).unwrap(), + nodes_created as u32, + ); } } - // ===================== - // Utilities - // ===================== - - fn find_last_element(&mut self, vnode: &'bump VNode<'bump>) -> Option { - let mut search_node = Some(vnode); - - loop { - match &search_node.take().unwrap() { - VNode::Text(t) => break t.id.get(), - VNode::Element(t) => break t.id.get(), - VNode::Placeholder(t) => break t.id.get(), - VNode::Fragment(frag) => { - search_node = frag.children.last(); - } - VNode::Component(el) => { - let scope_id = el.scope.get().unwrap(); - search_node = Some(self.scopes.root_node(scope_id)); - } - } - } + fn replace_node(&mut self, old: &'b VNode<'b>, new: &'b VNode<'b>) { + log::debug!("Replacing node\n old: {:?}\n new: {:?}", old, new); + let nodes_created = self.create_node(new); + self.replace_inner(old, nodes_created); } - fn find_first_element_id(&mut self, vnode: &'bump VNode<'bump>) -> Option { - let mut search_node = Some(vnode); - - loop { - match &search_node.take().unwrap() { - // the ones that have a direct id - VNode::Fragment(frag) => { - search_node = Some(&frag.children[0]); - } - VNode::Component(el) => { - let scope_id = el.scope.get().unwrap(); - search_node = Some(self.scopes.root_node(scope_id)); - } - VNode::Text(t) => break t.id.get(), - VNode::Element(t) => break t.id.get(), - VNode::Placeholder(t) => break t.id.get(), - } - } - } - - fn replace_node(&mut self, old: &'bump VNode<'bump>, nodes_created: usize) { - log::debug!("Replacing node {:?}", old); + fn replace_inner(&mut self, old: &'b VNode<'b>, nodes_created: usize) { match old { VNode::Element(el) => { let id = old @@ -1271,6 +816,7 @@ impl<'bump> DiffState<'bump> { self.mutations.replace_with(id, nodes_created as u32); self.remove_nodes(el.children, false); + self.scopes.collect_garbage(id); } VNode::Text(_) | VNode::Placeholder(_) => { @@ -1279,42 +825,37 @@ impl<'bump> DiffState<'bump> { .unwrap_or_else(|| panic!("broke on {:?}", old)); self.mutations.replace_with(id, nodes_created as u32); + self.scopes.collect_garbage(id); } VNode::Fragment(f) => { - self.replace_node(&f.children[0], nodes_created); + self.replace_inner(&f.children[0], nodes_created); self.remove_nodes(f.children.iter().skip(1), true); } VNode::Component(c) => { let node = self.scopes.fin_head(c.scope.get().unwrap()); - self.replace_node(node, nodes_created); + self.replace_inner(node, nodes_created); let scope_id = c.scope.get().unwrap(); // we can only remove components if they are actively being diffed - if self.stack.scope_stack.contains(&c.originator) { + if self.scope_stack.contains(&c.originator) { + log::debug!("Removing component {:?}", old); + self.scopes.try_remove(scope_id).unwrap(); } } } } - /// schedules nodes for garbage collection and pushes "remove" to the mutation stack - /// remove can happen whenever - pub(crate) fn remove_nodes( - &mut self, - nodes: impl IntoIterator>, - gen_muts: bool, - ) { - // or cache the vec on the diff machine + pub fn remove_nodes(&mut self, nodes: impl IntoIterator>, gen_muts: bool) { for node in nodes { - log::debug!("removing {:?}", node); match node { VNode::Text(t) => { // this check exists because our null node will be removed but does not have an ID if let Some(id) = t.id.get() { - // self.scopes.collect_garbage(id); + self.scopes.collect_garbage(id); if gen_muts { self.mutations.remove(id.as_u64()); @@ -1323,7 +864,7 @@ impl<'bump> DiffState<'bump> { } VNode::Placeholder(a) => { let id = a.id.get().unwrap(); - // self.scopes.collect_garbage(id); + self.scopes.collect_garbage(id); if gen_muts { self.mutations.remove(id.as_u64()); @@ -1336,9 +877,9 @@ impl<'bump> DiffState<'bump> { self.mutations.remove(id.as_u64()); } - self.remove_nodes(e.children, false); + self.scopes.collect_garbage(id); - // self.scopes.collect_garbage(id); + self.remove_nodes(e.children, false); } VNode::Fragment(f) => { @@ -1348,14 +889,119 @@ impl<'bump> DiffState<'bump> { VNode::Component(c) => { let scope_id = c.scope.get().unwrap(); let root = self.scopes.root_node(scope_id); - self.remove_nodes(Some(root), gen_muts); + self.remove_nodes([root], gen_muts); // we can only remove this node if the originator is actively - if self.stack.scope_stack.contains(&c.originator) { + if self.scope_stack.contains(&c.originator) { self.scopes.try_remove(scope_id).unwrap(); } } } } } + + fn create_children(&mut self, nodes: &'b [VNode<'b>]) -> usize { + let mut created = 0; + for node in nodes { + created += self.create_node(node); + } + created + } + + fn create_and_append_children(&mut self, nodes: &'b [VNode<'b>]) { + let created = self.create_children(nodes); + self.mutations.append_children(created as u32); + } + + fn create_and_insert_after(&mut self, nodes: &'b [VNode<'b>], after: &'b VNode<'b>) { + let created = self.create_children(nodes); + let last = self.find_last_element(after).unwrap(); + self.mutations.insert_after(last, created as u32); + } + + fn create_and_insert_before(&mut self, nodes: &'b [VNode<'b>], before: &'b VNode<'b>) { + let created = self.create_children(nodes); + let first = self.find_first_element(before).unwrap(); + self.mutations.insert_before(first, created as u32); + } + + fn current_scope(&self) -> Option { + self.scope_stack.last().copied() + } + + fn enter_scope(&mut self, scope: ScopeId) { + self.scope_stack.push(scope); + } + + fn leave_scope(&mut self) { + self.scope_stack.pop(); + } + + fn find_last_element(&self, vnode: &'b VNode<'b>) -> Option { + let mut search_node = Some(vnode); + + loop { + match &search_node.take().unwrap() { + VNode::Text(t) => break t.id.get(), + VNode::Element(t) => break t.id.get(), + VNode::Placeholder(t) => break t.id.get(), + VNode::Fragment(frag) => { + search_node = frag.children.last(); + } + VNode::Component(el) => { + let scope_id = el.scope.get().unwrap(); + search_node = Some(self.scopes.root_node(scope_id)); + } + } + } + } + + fn find_first_element(&self, vnode: &'b VNode<'b>) -> Option { + let mut search_node = Some(vnode); + + loop { + match &search_node.take().unwrap() { + // the ones that have a direct id + VNode::Fragment(frag) => { + search_node = Some(&frag.children[0]); + } + VNode::Component(el) => { + let scope_id = el.scope.get().unwrap(); + search_node = Some(self.scopes.root_node(scope_id)); + } + VNode::Text(t) => break t.id.get(), + VNode::Element(t) => break t.id.get(), + VNode::Placeholder(t) => break t.id.get(), + } + } + } + + // recursively push all the nodes of a tree onto the stack and return how many are there + fn push_all_nodes(&mut self, node: &'b VNode<'b>) -> usize { + match node { + VNode::Text(_) | VNode::Placeholder(_) => { + self.mutations.push_root(node.mounted_id()); + 1 + } + + VNode::Fragment(_) | VNode::Component(_) => { + // + let mut added = 0; + for child in node.children() { + added += self.push_all_nodes(child); + } + added + } + + VNode::Element(el) => { + let mut num_on_stack = 0; + for child in el.children.iter() { + num_on_stack += self.push_all_nodes(child); + } + self.mutations.push_root(el.id.get().unwrap()); + + num_on_stack + 1 + } + } + } } diff --git a/packages/core/src/diff_async.rs b/packages/core/src/diff_async.rs deleted file mode 100644 index e5d63ffb56..0000000000 --- a/packages/core/src/diff_async.rs +++ /dev/null @@ -1,1102 +0,0 @@ -use crate::innerlude::*; -use fxhash::{FxHashMap, FxHashSet}; -use smallvec::{smallvec, SmallVec}; - -pub(crate) struct AsyncDiffState<'bump> { - pub(crate) scopes: &'bump ScopeArena, - pub(crate) mutations: Mutations<'bump>, - pub(crate) force_diff: bool, - pub(crate) element_stack: SmallVec<[ElementId; 10]>, - pub(crate) scope_stack: SmallVec<[ScopeId; 5]>, -} - -impl<'b> AsyncDiffState<'b> { - pub fn new(scopes: &'b ScopeArena) -> Self { - Self { - scopes, - mutations: Mutations::new(), - force_diff: false, - element_stack: smallvec![], - scope_stack: smallvec![], - } - } - - pub fn diff_scope(&mut self, scopeid: ScopeId) { - let (old, new) = (self.scopes.wip_head(scopeid), self.scopes.fin_head(scopeid)); - self.scope_stack.push(scopeid); - let scope = self.scopes.get_scope(scopeid).unwrap(); - self.element_stack.push(scope.container); - self.diff_node(old, new); - } - - pub fn diff_node(&mut self, old_node: &'b VNode<'b>, new_node: &'b VNode<'b>) { - use VNode::*; - match (old_node, new_node) { - // Check the most common cases first - // these are *actual* elements, not wrappers around lists - (Text(old), Text(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - text are the sames"); - return; - } - - if let Some(root) = old.id.get() { - if old.text != new.text { - self.mutations.set_text(new.text, root.as_u64()); - } - self.scopes.update_node(new_node, root); - - new.id.set(Some(root)); - } - } - - (Placeholder(old), Placeholder(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - placeholder are the sames"); - return; - } - - if let Some(root) = old.id.get() { - self.scopes.update_node(new_node, root); - new.id.set(Some(root)) - } - } - - (Element(old), Element(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - element are the sames"); - return; - } - self.diff_element_nodes(old, new, old_node, new_node) - } - - // These two sets are pointers to nodes but are not actually nodes themselves - (Component(old), Component(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - placeholder are the sames"); - return; - } - self.diff_component_nodes(old_node, new_node, *old, *new) - } - - (Fragment(old), Fragment(new)) => { - if std::ptr::eq(old, new) { - log::trace!("skipping node diff - fragment are the sames"); - return; - } - self.diff_fragment_nodes(old, new) - } - - // The normal pathway still works, but generates slightly weird instructions - // This pathway ensures uses the ReplaceAll, not the InsertAfter and remove - (Placeholder(_), Fragment(_)) => { - log::debug!("replacing placeholder with fragment {:?}", new_node); - self.replace_node(old_node, new_node); - } - - // Anything else is just a basic replace and create - ( - Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), - Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), - ) => self.replace_node(old_node, new_node), - } - } - - pub fn create_node(&mut self, node: &'b VNode<'b>) -> usize { - match node { - VNode::Text(vtext) => self.create_text_node(vtext, node), - VNode::Placeholder(anchor) => self.create_anchor_node(anchor, node), - VNode::Element(element) => self.create_element_node(element, node), - VNode::Fragment(frag) => self.create_fragment_node(frag), - VNode::Component(component) => self.create_component_node(*component), - } - } - - fn create_text_node(&mut self, vtext: &'b VText<'b>, node: &'b VNode<'b>) -> usize { - let real_id = self.scopes.reserve_node(node); - self.mutations.create_text_node(vtext.text, real_id); - vtext.id.set(Some(real_id)); - - 1 - } - - fn create_anchor_node(&mut self, anchor: &'b VPlaceholder, node: &'b VNode<'b>) -> usize { - let real_id = self.scopes.reserve_node(node); - self.mutations.create_placeholder(real_id); - anchor.id.set(Some(real_id)); - - 1 - } - - fn create_element_node(&mut self, element: &'b VElement<'b>, node: &'b VNode<'b>) -> usize { - let VElement { - tag: tag_name, - listeners, - attributes, - children, - namespace, - id: dom_id, - parent: parent_id, - .. - } = element; - - // set the parent ID for event bubbling - // self.stack.instructions.push(DiffInstruction::PopElement); - - let parent = self.element_stack.last().unwrap(); - parent_id.set(Some(*parent)); - - // set the id of the element - let real_id = self.scopes.reserve_node(node); - self.element_stack.push(real_id); - dom_id.set(Some(real_id)); - - self.mutations.create_element(tag_name, *namespace, real_id); - - if let Some(cur_scope_id) = self.current_scope() { - for listener in *listeners { - listener.mounted_node.set(Some(real_id)); - self.mutations.new_event_listener(listener, cur_scope_id); - } - } else { - log::warn!("create element called with no scope on the stack - this is an error for a live dom"); - } - - for attr in *attributes { - self.mutations.set_attribute(attr, real_id.as_u64()); - } - - if !children.is_empty() { - self.create_and_append_children(children); - } - - 1 - } - - fn create_fragment_node(&mut self, frag: &'b VFragment<'b>) -> usize { - self.create_children(frag.children) - } - - fn create_component_node(&mut self, vcomponent: &'b VComponent<'b>) -> usize { - let parent_idx = self.current_scope().unwrap(); - - // the component might already exist - if it does, we need to reuse it - // this makes figure out when to drop the component more complicated - let new_idx = if let Some(idx) = vcomponent.scope.get() { - assert!(self.scopes.get_scope(idx).is_some()); - idx - } else { - // Insert a new scope into our component list - let props: Box = vcomponent.props.borrow_mut().take().unwrap(); - let props: Box = unsafe { std::mem::transmute(props) }; - let new_idx = self.scopes.new_with_key( - vcomponent.user_fc, - props, - Some(parent_idx), - self.element_stack.last().copied().unwrap(), - 0, - ); - - new_idx - }; - - log::info!( - "created component {:?} with parent {:?} and originator {:?}", - new_idx, - parent_idx, - vcomponent.originator - ); - - // Actually initialize the caller's slot with the right address - vcomponent.scope.set(Some(new_idx)); - - match vcomponent.can_memoize { - true => { - // todo: implement promotion logic. save us from boxing props that we don't need - } - false => { - // track this component internally so we know the right drop order - } - } - - // Run the scope for one iteration to initialize it - self.scopes.run_scope(new_idx); - - self.mutations.mark_dirty_scope(new_idx); - - // self.stack.create_component(new_idx, nextnode); - // Finally, insert this scope as a seen node. - - // Take the node that was just generated from running the component - let nextnode = self.scopes.fin_head(new_idx); - self.create_node(nextnode) - } - - fn diff_element_nodes( - &mut self, - old: &'b VElement<'b>, - new: &'b VElement<'b>, - old_node: &'b VNode<'b>, - new_node: &'b VNode<'b>, - ) { - let root = old.id.get().unwrap(); - - // If the element type is completely different, the element needs to be re-rendered completely - // This is an optimization React makes due to how users structure their code - // - // This case is rather rare (typically only in non-keyed lists) - if new.tag != old.tag || new.namespace != old.namespace { - self.replace_node(old_node, new_node); - return; - } - - self.scopes.update_node(new_node, root); - - new.id.set(Some(root)); - new.parent.set(old.parent.get()); - - // todo: attributes currently rely on the element on top of the stack, but in theory, we only need the id of the - // element to modify its attributes. - // it would result in fewer instructions if we just set the id directly. - // it would also clean up this code some, but that's not very important anyways - - // Diff Attributes - // - // It's extraordinarily rare to have the number/order of attributes change - // In these cases, we just completely erase the old set and make a new set - // - // TODO: take a more efficient path than this - if old.attributes.len() == new.attributes.len() { - for (old_attr, new_attr) in old.attributes.iter().zip(new.attributes.iter()) { - if old_attr.value != new_attr.value || new_attr.is_volatile { - self.mutations.set_attribute(new_attr, root.as_u64()); - } - } - } else { - for attribute in old.attributes { - self.mutations.remove_attribute(attribute, root.as_u64()); - } - for attribute in new.attributes { - self.mutations.set_attribute(attribute, root.as_u64()) - } - } - - // Diff listeners - // - // It's extraordinarily rare to have the number/order of listeners change - // In the cases where the listeners change, we completely wipe the data attributes and add new ones - // - // We also need to make sure that all listeners are properly attached to the parent scope (fix_listener) - // - // TODO: take a more efficient path than this - if let Some(cur_scope_id) = self.current_scope() { - if old.listeners.len() == new.listeners.len() { - for (old_l, new_l) in old.listeners.iter().zip(new.listeners.iter()) { - if old_l.event != new_l.event { - self.mutations - .remove_event_listener(old_l.event, root.as_u64()); - self.mutations.new_event_listener(new_l, cur_scope_id); - } - new_l.mounted_node.set(old_l.mounted_node.get()); - } - } else { - for listener in old.listeners { - self.mutations - .remove_event_listener(listener.event, root.as_u64()); - } - for listener in new.listeners { - listener.mounted_node.set(Some(root)); - self.mutations.new_event_listener(listener, cur_scope_id); - } - } - } - - match (old.children.len(), new.children.len()) { - (0, 0) => {} - (0, _) => { - let created = self.create_children(new.children); - self.mutations.append_children(created as u32); - } - (_, _) => { - self.diff_children(old.children, new.children); - } - }; - } - - fn diff_component_nodes( - &mut self, - old_node: &'b VNode<'b>, - new_node: &'b VNode<'b>, - old: &'b VComponent<'b>, - new: &'b VComponent<'b>, - ) { - let scope_addr = old.scope.get().unwrap(); - log::trace!( - "diff_component_nodes. old: {:#?} new: {:#?}", - old_node, - new_node - ); - - if std::ptr::eq(old, new) { - log::trace!("skipping component diff - component is the sames"); - return; - } - - // Make sure we're dealing with the same component (by function pointer) - if old.user_fc == new.user_fc { - self.enter_scope(scope_addr); - - // Make sure the new component vnode is referencing the right scope id - new.scope.set(Some(scope_addr)); - - // make sure the component's caller function is up to date - let scope = self - .scopes - .get_scope(scope_addr) - .unwrap_or_else(|| panic!("could not find {:?}", scope_addr)); - - // take the new props out regardless - // when memoizing, push to the existing scope if memoization happens - let new_props = new.props.borrow_mut().take().unwrap(); - - let should_run = { - if old.can_memoize { - let props_are_the_same = unsafe { - scope - .props - .borrow() - .as_ref() - .unwrap() - .memoize(new_props.as_ref()) - }; - !props_are_the_same || self.force_diff - } else { - true - } - }; - - if should_run { - let _old_props = scope - .props - .replace(unsafe { std::mem::transmute(Some(new_props)) }); - - // this should auto drop the previous props - self.scopes.run_scope(scope_addr); - self.mutations.mark_dirty_scope(scope_addr); - - self.diff_node( - self.scopes.wip_head(scope_addr), - self.scopes.fin_head(scope_addr), - ); - } else { - log::trace!("memoized"); - // memoization has taken place - drop(new_props); - }; - - self.leave_scope(); - } else { - log::debug!("scope stack is {:#?}", self.scope_stack); - self.replace_node(old_node, new_node); - } - } - - fn diff_fragment_nodes(&mut self, old: &'b VFragment<'b>, new: &'b VFragment<'b>) { - // This is the case where options or direct vnodes might be used. - // In this case, it's faster to just skip ahead to their diff - if old.children.len() == 1 && new.children.len() == 1 { - self.diff_node(&old.children[0], &new.children[0]); - return; - } - - debug_assert!(!old.children.is_empty()); - debug_assert!(!new.children.is_empty()); - - self.diff_children(old.children, new.children); - } - - // ============================================= - // Utilities for creating new diff instructions - // ============================================= - - // Diff the given set of old and new children. - // - // The parent must be on top of the change list stack when this function is - // entered: - // - // [... parent] - // - // the change list stack is in the same state when this function returns. - // - // If old no anchors are provided, then it's assumed that we can freely append to the parent. - // - // Remember, non-empty lists does not mean that there are real elements, just that there are virtual elements. - // - // Fragment nodes cannot generate empty children lists, so we can assume that when a list is empty, it belongs only - // to an element, and appending makes sense. - fn diff_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { - // Remember, fragments can never be empty (they always have a single child) - match (old, new) { - ([], []) => {} - ([], _) => self.create_and_append_children(new), - (_, []) => self.remove_nodes(old, true), - _ => { - let new_is_keyed = new[0].key().is_some(); - let old_is_keyed = old[0].key().is_some(); - - debug_assert!( - new.iter().all(|n| n.key().is_some() == new_is_keyed), - "all siblings must be keyed or all siblings must be non-keyed" - ); - debug_assert!( - old.iter().all(|o| o.key().is_some() == old_is_keyed), - "all siblings must be keyed or all siblings must be non-keyed" - ); - - if new_is_keyed && old_is_keyed { - self.diff_keyed_children(old, new); - } else { - self.diff_non_keyed_children(old, new); - } - } - } - } - - // Diff children that are not keyed. - // - // The parent must be on the top of the change list stack when entering this - // function: - // - // [... parent] - // - // the change list stack is in the same state when this function returns. - fn diff_non_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { - // Handled these cases in `diff_children` before calling this function. - debug_assert!(!new.is_empty()); - debug_assert!(!old.is_empty()); - - use std::cmp::Ordering; - match old.len().cmp(&new.len()) { - Ordering::Greater => self.remove_nodes(&old[new.len()..], true), - Ordering::Less => self.create_and_insert_after(&new[old.len()..], old.last().unwrap()), - Ordering::Equal => {} - } - - for (new, old) in new.iter().zip(old.iter()) { - self.diff_node(old, new); - } - } - - // Diffing "keyed" children. - // - // With keyed children, we care about whether we delete, move, or create nodes - // versus mutate existing nodes in place. Presumably there is some sort of CSS - // transition animation that makes the virtual DOM diffing algorithm - // observable. By specifying keys for nodes, we know which virtual DOM nodes - // must reuse (or not reuse) the same physical DOM nodes. - // - // This is loosely based on Inferno's keyed patching implementation. However, we - // have to modify the algorithm since we are compiling the diff down into change - // list instructions that will be executed later, rather than applying the - // changes to the DOM directly as we compare virtual DOMs. - // - // https://github.com/infernojs/inferno/blob/36fd96/packages/inferno/src/DOM/patching.ts#L530-L739 - // - // The stack is empty upon entry. - fn diff_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { - if cfg!(debug_assertions) { - let mut keys = fxhash::FxHashSet::default(); - let mut assert_unique_keys = |children: &'b [VNode<'b>]| { - keys.clear(); - for child in children { - let key = child.key(); - debug_assert!( - key.is_some(), - "if any sibling is keyed, all siblings must be keyed" - ); - keys.insert(key); - } - debug_assert_eq!( - children.len(), - keys.len(), - "keyed siblings must each have a unique key" - ); - }; - assert_unique_keys(old); - assert_unique_keys(new); - } - - // First up, we diff all the nodes with the same key at the beginning of the - // children. - // - // `shared_prefix_count` is the count of how many nodes at the start of - // `new` and `old` share the same keys. - let (left_offset, right_offset) = match self.diff_keyed_ends(old, new) { - Some(count) => count, - None => return, - }; - - // Ok, we now hopefully have a smaller range of children in the middle - // within which to re-order nodes with the same keys, remove old nodes with - // now-unused keys, and create new nodes with fresh keys. - - let old_middle = &old[left_offset..(old.len() - right_offset)]; - let new_middle = &new[left_offset..(new.len() - right_offset)]; - - debug_assert!( - !((old_middle.len() == new_middle.len()) && old_middle.is_empty()), - "keyed children must have the same number of children" - ); - - if new_middle.is_empty() { - // remove the old elements - self.remove_nodes(old_middle, true); - } else if old_middle.is_empty() { - // there were no old elements, so just create the new elements - // we need to find the right "foothold" though - we shouldn't use the "append" at all - if left_offset == 0 { - // insert at the beginning of the old list - let foothold = &old[old.len() - right_offset]; - self.create_and_insert_before(new_middle, foothold); - } else if right_offset == 0 { - // insert at the end the old list - let foothold = old.last().unwrap(); - self.create_and_insert_after(new_middle, foothold); - } else { - // inserting in the middle - let foothold = &old[left_offset - 1]; - self.create_and_insert_after(new_middle, foothold); - } - } else { - self.diff_keyed_middle(old_middle, new_middle); - } - } - - /// Diff both ends of the children that share keys. - /// - /// Returns a left offset and right offset of that indicates a smaller section to pass onto the middle diffing. - /// - /// If there is no offset, then this function returns None and the diffing is complete. - fn diff_keyed_ends( - &mut self, - old: &'b [VNode<'b>], - new: &'b [VNode<'b>], - ) -> Option<(usize, usize)> { - let mut left_offset = 0; - - for (old, new) in old.iter().zip(new.iter()) { - // abort early if we finally run into nodes with different keys - if old.key() != new.key() { - break; - } - self.diff_node(old, new); - left_offset += 1; - } - - // If that was all of the old children, then create and append the remaining - // new children and we're finished. - if left_offset == old.len() { - self.create_and_insert_after(&new[left_offset..], old.last().unwrap()); - return None; - } - - // And if that was all of the new children, then remove all of the remaining - // old children and we're finished. - if left_offset == new.len() { - self.remove_nodes(&old[left_offset..], true); - return None; - } - - // if the shared prefix is less than either length, then we need to walk backwards - let mut right_offset = 0; - for (old, new) in old.iter().rev().zip(new.iter().rev()) { - // abort early if we finally run into nodes with different keys - if old.key() != new.key() { - break; - } - self.diff_node(old, new); - right_offset += 1; - } - - Some((left_offset, right_offset)) - } - - // The most-general, expensive code path for keyed children diffing. - // - // We find the longest subsequence within `old` of children that are relatively - // ordered the same way in `new` (via finding a longest-increasing-subsequence - // of the old child's index within `new`). The children that are elements of - // this subsequence will remain in place, minimizing the number of DOM moves we - // will have to do. - // - // Upon entry to this function, the change list stack must be empty. - // - // This function will load the appropriate nodes onto the stack and do diffing in place. - // - // Upon exit from this function, it will be restored to that same self. - fn diff_keyed_middle(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { - /* - 1. Map the old keys into a numerical ordering based on indices. - 2. Create a map of old key to its index - 3. Map each new key to the old key, carrying over the old index. - - IE if we have ABCD becomes BACD, our sequence would be 1,0,2,3 - - if we have ABCD to ABDE, our sequence would be 0,1,3,MAX because E doesn't exist - - now, we should have a list of integers that indicates where in the old list the new items map to. - - 4. Compute the LIS of this list - - this indicates the longest list of new children that won't need to be moved. - - 5. Identify which nodes need to be removed - 6. Identify which nodes will need to be diffed - - 7. Going along each item in the new list, create it and insert it before the next closest item in the LIS. - - if the item already existed, just move it to the right place. - - 8. Finally, generate instructions to remove any old children. - 9. Generate instructions to finally diff children that are the same between both - */ - - // 0. Debug sanity checks - // Should have already diffed the shared-key prefixes and suffixes. - debug_assert_ne!(new.first().map(|n| n.key()), old.first().map(|o| o.key())); - debug_assert_ne!(new.last().map(|n| n.key()), old.last().map(|o| o.key())); - - // 1. Map the old keys into a numerical ordering based on indices. - // 2. Create a map of old key to its index - // IE if the keys were A B C, then we would have (A, 1) (B, 2) (C, 3). - let old_key_to_old_index = old - .iter() - .enumerate() - .map(|(i, o)| (o.key().unwrap(), i)) - .collect::>(); - - let mut shared_keys = FxHashSet::default(); - - // 3. Map each new key to the old key, carrying over the old index. - let new_index_to_old_index = new - .iter() - .map(|node| { - let key = node.key().unwrap(); - if let Some(&index) = old_key_to_old_index.get(&key) { - shared_keys.insert(key); - index - } else { - u32::MAX as usize - } - }) - .collect::>(); - - // If none of the old keys are reused by the new children, then we remove all the remaining old children and - // create the new children afresh. - if shared_keys.is_empty() { - if let Some(first_old) = old.get(0) { - self.remove_nodes(&old[1..], true); - let nodes_created = self.create_children(new); - self.replace_inner(first_old, nodes_created); - } else { - // I think this is wrong - why are we appending? - // only valid of the if there are no trailing elements - self.create_and_append_children(new); - } - return; - } - - // 4. Compute the LIS of this list - let mut lis_sequence = Vec::default(); - lis_sequence.reserve(new_index_to_old_index.len()); - - let mut predecessors = vec![0; new_index_to_old_index.len()]; - let mut starts = vec![0; new_index_to_old_index.len()]; - - longest_increasing_subsequence::lis_with( - &new_index_to_old_index, - &mut lis_sequence, - |a, b| a < b, - &mut predecessors, - &mut starts, - ); - - // the lis comes out backwards, I think. can't quite tell. - lis_sequence.sort_unstable(); - - // if a new node gets u32 max and is at the end, then it might be part of our LIS (because u32 max is a valid LIS) - if lis_sequence.last().map(|f| new_index_to_old_index[*f]) == Some(u32::MAX as usize) { - lis_sequence.pop(); - } - - for idx in lis_sequence.iter() { - self.diff_node(&old[new_index_to_old_index[*idx]], &new[*idx]); - } - - let mut nodes_created = 0; - - // add mount instruction for the first items not covered by the lis - let last = *lis_sequence.last().unwrap(); - if last < (new.len() - 1) { - for (idx, new_node) in new[(last + 1)..].iter().enumerate() { - let new_idx = idx + last + 1; - let old_index = new_index_to_old_index[new_idx]; - if old_index == u32::MAX as usize { - nodes_created += self.create_node(new_node); - } else { - self.diff_node(&old[old_index], new_node); - nodes_created += self.push_all_nodes(new_node); - } - } - - self.mutations.insert_after( - self.find_last_element(&new[last]).unwrap(), - nodes_created as u32, - ); - nodes_created = 0; - } - - // for each spacing, generate a mount instruction - let mut lis_iter = lis_sequence.iter().rev(); - let mut last = *lis_iter.next().unwrap(); - for next in lis_iter { - if last - next > 1 { - for (idx, new_node) in new[(next + 1)..last].iter().enumerate() { - let new_idx = idx + next + 1; - - let old_index = new_index_to_old_index[new_idx]; - if old_index == u32::MAX as usize { - nodes_created += self.create_node(new_node); - } else { - self.diff_node(&old[old_index], new_node); - nodes_created += self.push_all_nodes(new_node); - } - } - - self.mutations.insert_before( - self.find_first_element(&new[last]).unwrap(), - nodes_created as u32, - ); - - nodes_created = 0; - } - last = *next; - } - - // add mount instruction for the last items not covered by the lis - let first_lis = *lis_sequence.first().unwrap(); - if first_lis > 0 { - for (idx, new_node) in new[..first_lis].iter().enumerate() { - let old_index = new_index_to_old_index[idx]; - if old_index == u32::MAX as usize { - nodes_created += self.create_node(new_node); - } else { - self.diff_node(&old[old_index], new_node); - nodes_created += self.push_all_nodes(new_node); - } - } - - self.mutations.insert_before( - self.find_first_element(&new[first_lis]).unwrap(), - nodes_created as u32, - ); - } - } - - fn replace_node(&mut self, old: &'b VNode<'b>, new: &'b VNode<'b>) { - // fn replace_node(&mut self, old: &'b VNode<'b>, new: &'b VNode<'b>) { - log::debug!("Replacing node {:?}", old); - let nodes_created = self.create_node(new); - self.replace_inner(old, nodes_created); - } - - fn replace_inner(&mut self, old: &'b VNode<'b>, nodes_created: usize) { - match old { - VNode::Element(el) => { - let id = old - .try_mounted_id() - .unwrap_or_else(|| panic!("broke on {:?}", old)); - - self.mutations.replace_with(id, nodes_created as u32); - self.remove_nodes(el.children, false); - self.scopes.collect_garbage(id); - } - - VNode::Text(_) | VNode::Placeholder(_) => { - let id = old - .try_mounted_id() - .unwrap_or_else(|| panic!("broke on {:?}", old)); - - self.mutations.replace_with(id, nodes_created as u32); - self.scopes.collect_garbage(id); - } - - VNode::Fragment(f) => { - self.replace_inner(&f.children[0], nodes_created); - self.remove_nodes(f.children.iter().skip(1), true); - } - - VNode::Component(c) => { - let node = self.scopes.fin_head(c.scope.get().unwrap()); - self.replace_node(node, node); - - let scope_id = c.scope.get().unwrap(); - - // we can only remove components if they are actively being diffed - if self.scope_stack.contains(&c.originator) { - self.scopes.try_remove(scope_id).unwrap(); - } - } - } - } - - pub fn remove_nodes(&mut self, nodes: impl IntoIterator>, gen_muts: bool) { - for node in nodes { - match node { - VNode::Text(t) => { - // this check exists because our null node will be removed but does not have an ID - if let Some(id) = t.id.get() { - self.scopes.collect_garbage(id); - - if gen_muts { - self.mutations.remove(id.as_u64()); - } - } - } - VNode::Placeholder(a) => { - let id = a.id.get().unwrap(); - self.scopes.collect_garbage(id); - - if gen_muts { - self.mutations.remove(id.as_u64()); - } - } - VNode::Element(e) => { - let id = e.id.get().unwrap(); - - if gen_muts { - self.mutations.remove(id.as_u64()); - } - - self.scopes.collect_garbage(id); - - self.remove_nodes(e.children, false); - } - - VNode::Fragment(f) => { - self.remove_nodes(f.children, gen_muts); - } - - VNode::Component(c) => { - let scope_id = c.scope.get().unwrap(); - let root = self.scopes.root_node(scope_id); - self.remove_nodes([root], gen_muts); - - // we can only remove this node if the originator is actively - if self.scope_stack.contains(&c.originator) { - self.scopes.try_remove(scope_id).unwrap(); - } - } - } - } - } - - fn create_children(&mut self, nodes: &'b [VNode<'b>]) -> usize { - let mut created = 0; - for node in nodes { - created += self.create_node(node); - } - created - } - - fn create_and_append_children(&mut self, nodes: &'b [VNode<'b>]) { - let created = self.create_children(nodes); - self.mutations.append_children(created as u32); - } - - fn create_and_insert_after(&mut self, nodes: &'b [VNode<'b>], after: &'b VNode<'b>) { - let created = self.create_children(nodes); - let last = self.find_last_element(after).unwrap(); - self.mutations.insert_after(last, created as u32); - } - - fn create_and_insert_before(&mut self, nodes: &'b [VNode<'b>], before: &'b VNode<'b>) { - let created = self.create_children(nodes); - let first = self.find_first_element(before).unwrap(); - self.mutations.insert_before(first, created as u32); - } - - fn current_scope(&self) -> Option { - self.scope_stack.last().copied() - } - - fn enter_scope(&mut self, scope: ScopeId) { - self.scope_stack.push(scope); - } - - fn leave_scope(&mut self) { - self.scope_stack.pop(); - } - - fn find_last_element(&self, vnode: &'b VNode<'b>) -> Option { - let mut search_node = Some(vnode); - - loop { - match &search_node.take().unwrap() { - VNode::Text(t) => break t.id.get(), - VNode::Element(t) => break t.id.get(), - VNode::Placeholder(t) => break t.id.get(), - VNode::Fragment(frag) => { - search_node = frag.children.last(); - } - VNode::Component(el) => { - let scope_id = el.scope.get().unwrap(); - search_node = Some(self.scopes.root_node(scope_id)); - } - } - } - } - - fn find_first_element(&self, vnode: &'b VNode<'b>) -> Option { - let mut search_node = Some(vnode); - - loop { - match &search_node.take().unwrap() { - // the ones that have a direct id - VNode::Fragment(frag) => { - search_node = Some(&frag.children[0]); - } - VNode::Component(el) => { - let scope_id = el.scope.get().unwrap(); - search_node = Some(self.scopes.root_node(scope_id)); - } - VNode::Text(t) => break t.id.get(), - VNode::Element(t) => break t.id.get(), - VNode::Placeholder(t) => break t.id.get(), - } - } - } - - // fn replace_node(&mut self, old: &'b VNode<'b>, nodes_created: usize) { - // log::debug!("Replacing node {:?}", old); - // match old { - // VNode::Element(el) => { - // let id = old - // .try_mounted_id() - // .unwrap_or_else(|| panic!("broke on {:?}", old)); - - // self.mutations.replace_with(id, nodes_created as u32); - // self.remove_nodes(el.children, false); - // } - - // VNode::Text(_) | VNode::Placeholder(_) => { - // let id = old - // .try_mounted_id() - // .unwrap_or_else(|| panic!("broke on {:?}", old)); - - // self.mutations.replace_with(id, nodes_created as u32); - // } - - // VNode::Fragment(f) => { - // self.replace_node(&f.children[0], nodes_created); - // self.remove_nodes(f.children.iter().skip(1), true); - // } - - // VNode::Component(c) => { - // let node = self.scopes.fin_head(c.scope.get().unwrap()); - // self.replace_node(node, nodes_created); - - // let scope_id = c.scope.get().unwrap(); - - // // we can only remove components if they are actively being diffed - // if self.stack.scope_stack.contains(&c.originator) { - // self.scopes.try_remove(scope_id).unwrap(); - // } - // } - // } - // } - - // /// schedules nodes for garbage collection and pushes "remove" to the mutation stack - // /// remove can happen whenever - // pub(crate) fn remove_nodes( - // &mut self, - // nodes: impl IntoIterator>, - // gen_muts: bool, - // ) { - // // or cache the vec on the diff machine - // for node in nodes { - // log::debug!("removing {:?}", node); - // match node { - // VNode::Text(t) => { - // // this check exists because our null node will be removed but does not have an ID - // if let Some(id) = t.id.get() { - // // self.scopes.collect_garbage(id); - - // if gen_muts { - // self.mutations.remove(id.as_u64()); - // } - // } - // } - // VNode::Placeholder(a) => { - // let id = a.id.get().unwrap(); - // // self.scopes.collect_garbage(id); - - // if gen_muts { - // self.mutations.remove(id.as_u64()); - // } - // } - // VNode::Element(e) => { - // let id = e.id.get().unwrap(); - - // if gen_muts { - // self.mutations.remove(id.as_u64()); - // } - - // self.remove_nodes(e.children, false); - - // // self.scopes.collect_garbage(id); - // } - - // VNode::Fragment(f) => { - // self.remove_nodes(f.children, gen_muts); - // } - - // VNode::Component(c) => { - // let scope_id = c.scope.get().unwrap(); - // let root = self.scopes.root_node(scope_id); - // self.remove_nodes(Some(root), gen_muts); - - // // we can only remove this node if the originator is actively - // if self.stack.scope_stack.contains(&c.originator) { - // self.scopes.try_remove(scope_id).unwrap(); - // } - // } - // } - // } - // } - - // recursively push all the nodes of a tree onto the stack and return how many are there - fn push_all_nodes(&mut self, node: &'b VNode<'b>) -> usize { - match node { - VNode::Text(_) | VNode::Placeholder(_) => { - self.mutations.push_root(node.mounted_id()); - 1 - } - - VNode::Fragment(_) | VNode::Component(_) => { - // - let mut added = 0; - for child in node.children() { - added += self.push_all_nodes(child); - } - added - } - - VNode::Element(el) => { - let mut num_on_stack = 0; - for child in el.children.iter() { - num_on_stack += self.push_all_nodes(child); - } - self.mutations.push_root(el.id.get().unwrap()); - - num_on_stack + 1 - } - } - } -} diff --git a/packages/core/src/lib.rs b/packages/core/src/lib.rs index 48a8a35056..dba4effbbc 100644 --- a/packages/core/src/lib.rs +++ b/packages/core/src/lib.rs @@ -1,8 +1,7 @@ #![allow(non_snake_case)] #![doc = include_str!("../README.md")] -// pub(crate) mod diff; -pub(crate) mod diff_async; +pub(crate) mod diff; pub(crate) mod events; pub(crate) mod lazynodes; pub(crate) mod mutations; @@ -13,8 +12,7 @@ pub(crate) mod util; pub(crate) mod virtual_dom; pub(crate) mod innerlude { - pub(crate) use crate::diff_async::*; - // pub(crate) use crate::diff::*; + pub(crate) use crate::diff::*; pub use crate::events::*; pub use crate::lazynodes::*; pub use crate::mutations::*; diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 542afc6739..0ebbea9b9a 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -255,7 +255,7 @@ impl ScopeArena { let scope = unsafe { &mut *self.get_scope_raw(id).expect("could not find scope") }; // if cfg!(debug_assertions) { - log::debug!("running scope {:?} symbol: {:?}", id, scope.fnptr); + // log::debug!("running scope {:?} symbol: {:?}", id, scope.fnptr); // todo: resolve frames properly backtrace::resolve(scope.fnptr, |symbol| { @@ -733,7 +733,6 @@ impl ScopeState { while let Some(parent_ptr) = search_parent { // safety: all parent pointers are valid thanks to the bump arena let parent = unsafe { &*parent_ptr }; - log::trace!("Searching parent scope {:?}", parent.scope_id()); if let Some(shared) = parent.shared_contexts.borrow().get(&TypeId::of::()) { return Some(shared.clone().downcast::().unwrap()); } diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index d879e0340b..032aa61981 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -2,7 +2,7 @@ //! //! This module provides the primary mechanics to create a hook-based, concurrent VDOM for Rust. -use crate::diff_async::AsyncDiffState as DiffState; +use crate::diff::AsyncDiffState as DiffState; use crate::innerlude::*; use futures_channel::mpsc::{UnboundedReceiver, UnboundedSender}; use futures_util::{future::poll_fn, StreamExt}; diff --git a/packages/core/tests/earlyabort.rs b/packages/core/tests/earlyabort.rs index 7dc2ae4d45..3999a74029 100644 --- a/packages/core/tests/earlyabort.rs +++ b/packages/core/tests/earlyabort.rs @@ -42,14 +42,8 @@ fn test_early_abort() { assert_eq!( edits.edits, [ - CreateElement { - tag: "div", - root: 1, - }, - CreateTextNode { - text: "Hello, world!", - root: 2, - }, + CreateElement { tag: "div", root: 1 }, + CreateTextNode { text: "Hello, world!", root: 2 }, AppendChildren { many: 1 }, AppendChildren { many: 1 }, ] @@ -65,14 +59,8 @@ fn test_early_abort() { assert_eq!( edits.edits, [ - CreateElement { - tag: "div", - root: 2, - }, - CreateTextNode { - text: "Hello, world!", - root: 4, - }, + CreateElement { tag: "div", root: 1 }, // keys get reused + CreateTextNode { text: "Hello, world!", root: 2 }, // keys get reused AppendChildren { many: 1 }, ReplaceWith { root: 3, m: 1 }, ] diff --git a/packages/core/tests/lifecycle.rs b/packages/core/tests/lifecycle.rs index 6c0df4e1ed..f977ba96ec 100644 --- a/packages/core/tests/lifecycle.rs +++ b/packages/core/tests/lifecycle.rs @@ -27,12 +27,7 @@ fn manual_diffing() { }; let value = Arc::new(Mutex::new("Hello")); - let mut dom = VirtualDom::new_with_props( - App, - AppProps { - value: value.clone(), - }, - ); + let mut dom = VirtualDom::new_with_props(App, AppProps { value: value.clone() }); let _ = dom.rebuild(); @@ -45,7 +40,7 @@ fn manual_diffing() { #[test] fn events_generate() { - static App: Component = |cx| { + fn app(cx: Scope) -> Element { let count = cx.use_hook(|_| 0); let inner = match *count { @@ -66,7 +61,7 @@ fn events_generate() { cx.render(inner) }; - let mut dom = VirtualDom::new(App); + let mut dom = VirtualDom::new(app); let mut channel = dom.get_scheduler_channel(); assert!(dom.has_work()); @@ -74,28 +69,12 @@ fn events_generate() { assert_eq!( edits.edits, [ - CreateElement { - tag: "div", - root: 1, - }, - NewEventListener { - event_name: "click", - scope: ScopeId(0), - root: 1, - }, - CreateElement { - tag: "div", - root: 2, - }, - CreateTextNode { - text: "nested", - root: 3, - }, + CreateElement { tag: "div", root: 1 }, + NewEventListener { event_name: "click", scope: ScopeId(0), root: 1 }, + CreateElement { tag: "div", root: 2 }, + CreateTextNode { text: "nested", root: 3 }, AppendChildren { many: 1 }, - CreateTextNode { - text: "Click me!", - root: 4, - }, + CreateTextNode { text: "Click me!", root: 4 }, AppendChildren { many: 2 }, AppendChildren { many: 1 }, ] @@ -104,7 +83,7 @@ fn events_generate() { #[test] fn components_generate() { - static App: Component = |cx| { + fn app(cx: Scope) -> Element { let render_phase = cx.use_hook(|_| 0); *render_phase += 1; @@ -121,106 +100,84 @@ fn components_generate() { }) }; - static Child: Component = |cx| { + fn Child(cx: Scope) -> Element { + log::debug!("Running child"); cx.render(rsx! { h1 {} }) - }; + } - let mut dom = VirtualDom::new(App); + let mut dom = VirtualDom::new(app); let edits = dom.rebuild(); assert_eq!( edits.edits, [ - CreateTextNode { - text: "Text0", - root: 1, - }, + CreateTextNode { text: "Text0", root: 1 }, AppendChildren { many: 1 }, ] ); - let edits = dom.hard_diff(ScopeId(0)); assert_eq!( - edits.edits, + dom.hard_diff(ScopeId(0)).edits, [ - CreateElement { - tag: "div", - root: 2, - }, + CreateElement { tag: "div", root: 2 }, ReplaceWith { root: 1, m: 1 }, ] ); - let edits = dom.hard_diff(ScopeId(0)); assert_eq!( - edits.edits, + dom.hard_diff(ScopeId(0)).edits, [ - CreateTextNode { - text: "Text2", - root: 3, - }, + CreateTextNode { text: "Text2", root: 1 }, ReplaceWith { root: 2, m: 1 }, ] ); - let edits = dom.hard_diff(ScopeId(0)); + // child {} assert_eq!( - edits.edits, + dom.hard_diff(ScopeId(0)).edits, [ - CreateElement { tag: "h1", root: 4 }, - ReplaceWith { root: 3, m: 1 }, + CreateElement { tag: "h1", root: 2 }, + ReplaceWith { root: 1, m: 1 }, ] ); - let edits = dom.hard_diff(ScopeId(0)); + // placeholder assert_eq!( - edits.edits, - [CreatePlaceholder { root: 5 }, ReplaceWith { root: 4, m: 1 },] + dom.hard_diff(ScopeId(0)).edits, + [CreatePlaceholder { root: 1 }, ReplaceWith { root: 2, m: 1 },] ); - let edits = dom.hard_diff(ScopeId(0)); assert_eq!( - edits.edits, + dom.hard_diff(ScopeId(0)).edits, [ - CreateTextNode { - text: "text 3", - root: 6, - }, - ReplaceWith { root: 5, m: 1 }, + CreateTextNode { text: "text 3", root: 2 }, + ReplaceWith { root: 1, m: 1 }, ] ); - let edits = dom.hard_diff(ScopeId(0)); assert_eq!( - edits.edits, + dom.hard_diff(ScopeId(0)).edits, [ - CreateTextNode { - text: "text 0", - root: 7, - }, - CreateTextNode { - text: "text 1", - root: 8, - }, - ReplaceWith { root: 6, m: 2 }, + CreateTextNode { text: "text 0", root: 1 }, + CreateTextNode { text: "text 1", root: 3 }, + ReplaceWith { root: 2, m: 2 }, ] ); - let edits = dom.hard_diff(ScopeId(0)); assert_eq!( - edits.edits, + dom.hard_diff(ScopeId(0)).edits, [ - CreateElement { tag: "h1", root: 9 }, - ReplaceWith { root: 7, m: 1 }, - Remove { root: 8 }, + CreateElement { tag: "h1", root: 2 }, + ReplaceWith { root: 1, m: 1 }, + Remove { root: 3 }, ] ); } #[test] fn component_swap() { - static App: Component = |cx| { + fn app(cx: Scope) -> Element { let render_phase = cx.use_hook(|_| 0); *render_phase += 1; @@ -296,7 +253,7 @@ fn component_swap() { }) }; - let mut dom = VirtualDom::new(App); + let mut dom = VirtualDom::new(app); let edits = dom.rebuild(); dbg!(&edits); diff --git a/packages/core/tests/passthru.rs b/packages/core/tests/passthru.rs new file mode 100644 index 0000000000..e00aa545c4 --- /dev/null +++ b/packages/core/tests/passthru.rs @@ -0,0 +1,108 @@ +#![allow(unused, non_upper_case_globals)] + +//! Diffing Tests +//! +//! These tests only verify that the diffing algorithm works properly for single components. +//! +//! It does not validated that component lifecycles work properly. This is done in another test file. + +use dioxus::{prelude::*, DomEdit, ScopeId}; +use dioxus_core as dioxus; +use dioxus_core_macro::*; +use dioxus_html as dioxus_elements; + +mod test_logging; + +fn new_dom() -> VirtualDom { + const IS_LOGGING_ENABLED: bool = false; + test_logging::set_up_logging(IS_LOGGING_ENABLED); + VirtualDom::new(|cx| rsx!(cx, "hi")) +} + +use DomEdit::*; + +/// Should push the text node onto the stack and modify it +#[test] +fn nested_passthru_creates() { + fn app(cx: Scope) -> Element { + cx.render(rsx! { + Child { + Child { + Child { + div { + "hi" + } + } + } + } + }) + }; + + #[inline_props] + fn Child<'a>(cx: Scope, children: Element<'a>) -> Element { + cx.render(rsx! { + children + }) + }; + + let mut dom = VirtualDom::new(app); + let mut channel = dom.get_scheduler_channel(); + assert!(dom.has_work()); + + let edits = dom.rebuild(); + assert_eq!( + edits.edits, + [ + CreateElement { tag: "div", root: 1 }, + CreateTextNode { text: "hi", root: 2 }, + AppendChildren { many: 1 }, + AppendChildren { many: 1 }, + ] + ) +} + +/// Should push the text node onto the stack and modify it +#[test] +fn nested_passthru_creates_add() { + fn app(cx: Scope) -> Element { + cx.render(rsx! { + Child { + "1" + Child { + "2" + Child { + "3" + div { + "hi" + } + } + } + } + }) + }; + + #[inline_props] + fn Child<'a>(cx: Scope, children: Element<'a>) -> Element { + cx.render(rsx! { + children + }) + }; + + let mut dom = VirtualDom::new(app); + let mut channel = dom.get_scheduler_channel(); + assert!(dom.has_work()); + + let edits = dom.rebuild(); + assert_eq!( + edits.edits, + [ + CreateTextNode { text: "1", root: 1 }, + CreateTextNode { text: "2", root: 2 }, + CreateTextNode { text: "3", root: 3 }, + CreateElement { tag: "div", root: 4 }, + CreateTextNode { text: "hi", root: 5 }, + AppendChildren { many: 1 }, + AppendChildren { many: 4 }, + ] + ) +} From cbd471fa46d80fb3ea4d5e484be4bf62a31f770f Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 01:24:11 -0500 Subject: [PATCH 04/14] debugging: add some more debug tooling --- packages/core/src/diff.rs | 24 ++++++++++++++++++++---- packages/core/src/nodes.rs | 16 +++++++--------- packages/core/src/scopes.rs | 8 ++++---- packages/core/src/virtual_dom.rs | 13 +++++++++++++ 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index d790292c84..6f7f12fa42 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -406,10 +406,14 @@ impl<'b> AsyncDiffState<'b> { fn diff_fragment_nodes(&mut self, old: &'b VFragment<'b>, new: &'b VFragment<'b>) { // This is the case where options or direct vnodes might be used. // In this case, it's faster to just skip ahead to their diff - if old.children.len() == 1 && new.children.len() == 1 { - self.diff_node(&old.children[0], &new.children[0]); - return; - } + // if old.children.len() == 1 && new.children.len() == 1 { + // if std::ptr::eq(old, new) { + // log::debug!("skipping fragment diff - fragment is the same"); + // return; + // } + // self.diff_node(&old.children[0], &new.children[0]); + // return; + // } debug_assert!(!old.children.is_empty()); debug_assert!(!new.children.is_empty()); @@ -437,6 +441,11 @@ impl<'b> AsyncDiffState<'b> { // Fragment nodes cannot generate empty children lists, so we can assume that when a list is empty, it belongs only // to an element, and appending makes sense. fn diff_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { + if std::ptr::eq(old, new) { + log::debug!("skipping fragment diff - fragment is the same"); + return; + } + // Remember, fragments can never be empty (they always have a single child) match (old, new) { ([], []) => {} @@ -484,7 +493,13 @@ impl<'b> AsyncDiffState<'b> { Ordering::Equal => {} } + // panic!( + // "diff_children: new_is_keyed: {:#?}, old_is_keyed: {:#?}. stack: {:#?}, oldptr: {:#?}, newptr: {:#?}", + // old, new, self.scope_stack, old as *const _, new as *const _ + // ); + for (new, old) in new.iter().zip(old.iter()) { + log::debug!("diffing nonkeyed {:#?} {:#?}", old, new); self.diff_node(old, new); } } @@ -893,6 +908,7 @@ impl<'b> AsyncDiffState<'b> { // we can only remove this node if the originator is actively if self.scope_stack.contains(&c.originator) { + log::debug!("I am allowed to remove component because scope stack contains originator. Scope stack: {:#?} {:#?} {:#?}", self.scope_stack, c.originator, c.scope); self.scopes.try_remove(scope_id).unwrap(); } } diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 74234a84c8..2f1453e1dd 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -182,15 +182,13 @@ impl Debug for VNode<'_> { VNode::Fragment(frag) => { write!(s, "VNode::VFragment {{ children: {:?} }}", frag.children) } - VNode::Component(comp) => { - s.debug_struct("VNode::VComponent") - .field("fnptr", &comp.user_fc) - .field("key", &comp.key) - .field("scope", &comp.scope) - .field("originator", &comp.originator) - .finish() - //write!(s, "VNode::VComponent {{ fc: {:?}}}", comp.user_fc) - } + VNode::Component(comp) => s + .debug_struct("VNode::VComponent") + .field("fnptr", &comp.user_fc) + .field("key", &comp.key) + .field("scope", &comp.scope) + .field("originator", &comp.originator) + .finish(), } } } diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 0ebbea9b9a..93a0c94a78 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -98,7 +98,8 @@ impl ScopeArena { let height = parent_scope .map(|id| self.get_scope(id).map(|scope| scope.height)) .flatten() - .unwrap_or_default(); + .unwrap_or_default() + + 1; let parent_scope = parent_scope.map(|f| self.get_scope_raw(f)).flatten(); @@ -203,9 +204,8 @@ impl ScopeArena { } pub fn collect_garbage(&self, id: ElementId) { - // println!("collecting garbage for {:?}", id); - // log::debug!("collecting garbage for {:?}", id); - self.nodes.borrow_mut().remove(id.0); + let node = self.nodes.borrow_mut().remove(id.0); + log::debug!("collecting garbage for {:?}, {:?}", id, unsafe { &*node }); } /// This method cleans up any references to data held within our hook list. This prevents mutable aliasing from diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 032aa61981..613ea92924 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -462,14 +462,27 @@ impl VirtualDom { self.dirty_scopes .retain(|id| scopes.get_scope(*id).is_some()); + log::debug!("dirty_scopes: {:?}", self.dirty_scopes); + + for id in &self.dirty_scopes { + let scope = scopes.get_scope(*id).unwrap(); + + log::debug!("dirty scope: {:?} with height {:?}", id, scope.height); + } + self.dirty_scopes.sort_by(|a, b| { let h1 = scopes.get_scope(*a).unwrap().height; let h2 = scopes.get_scope(*b).unwrap().height; h1.cmp(&h2).reverse() }); + log::debug!("dirty_scopes: {:?}", self.dirty_scopes); + if let Some(scopeid) = self.dirty_scopes.pop() { + log::debug!("diffing scope {:?}", scopeid); if !ran_scopes.contains(&scopeid) { + log::debug!("running scope scope {:?}", scopeid); + ran_scopes.insert(scopeid); self.scopes.run_scope(scopeid); From 11f6b938891ab67a05de6926373fbf331577775d Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 02:33:16 -0500 Subject: [PATCH 05/14] fix: remove nodes is in a happier state --- packages/core-macro/src/rsx/component.rs | 3 ++ packages/core/src/diff.rs | 37 ++++++++++++++++-------- packages/core/src/mutations.rs | 6 ++-- packages/core/src/nodes.rs | 4 +++ packages/core/src/scopes.rs | 24 ++++++++++----- packages/core/src/virtual_dom.rs | 14 ++------- 6 files changed, 54 insertions(+), 34 deletions(-) diff --git a/packages/core-macro/src/rsx/component.rs b/packages/core-macro/src/rsx/component.rs index 5257022a43..4e9d449cf6 100644 --- a/packages/core-macro/src/rsx/component.rs +++ b/packages/core-macro/src/rsx/component.rs @@ -135,11 +135,14 @@ impl ToTokens for Component { None => quote! { None }, }; + let fn_name = self.name.segments.last().unwrap().ident.to_string(); + tokens.append_all(quote! { __cx.component( #name, #builder, #key_token, + #fn_name ) }) } diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 6f7f12fa42..db898f194a 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -26,7 +26,10 @@ impl<'b> AsyncDiffState<'b> { self.scope_stack.push(scopeid); let scope = self.scopes.get_scope(scopeid).unwrap(); self.element_stack.push(scope.container); + self.diff_node(old, new); + + self.mutations.mark_dirty_scope(scopeid); } pub fn diff_node(&mut self, old_node: &'b VNode<'b>, new_node: &'b VNode<'b>) { @@ -87,13 +90,6 @@ impl<'b> AsyncDiffState<'b> { self.diff_fragment_nodes(old, new) } - // The normal pathway still works, but generates slightly weird instructions - // This pathway ensures uses the ReplaceAll, not the InsertAfter and remove - (Placeholder(_), Fragment(_)) => { - log::debug!("replacing placeholder with fragment {:?}", new_node); - self.replace_node(old_node, new_node); - } - // Anything else is just a basic replace and create ( Component(_) | Fragment(_) | Text(_) | Element(_) | Placeholder(_), @@ -201,7 +197,8 @@ impl<'b> AsyncDiffState<'b> { }; log::info!( - "created component {:?} with parent {:?} and originator {:?}", + "created component \"{}\", id: {:?} parent {:?} orig: {:?}", + vcomponent.fn_name, new_idx, parent_idx, vcomponent.originator @@ -442,7 +439,7 @@ impl<'b> AsyncDiffState<'b> { // to an element, and appending makes sense. fn diff_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { if std::ptr::eq(old, new) { - log::debug!("skipping fragment diff - fragment is the same"); + log::debug!("skipping fragment diff - fragment is the same {:?}", old); return; } @@ -499,7 +496,7 @@ impl<'b> AsyncDiffState<'b> { // ); for (new, old) in new.iter().zip(old.iter()) { - log::debug!("diffing nonkeyed {:#?} {:#?}", old, new); + // log::debug!("diffing nonkeyed {:#?} {:#?}", old, new); self.diff_node(old, new); } } @@ -849,10 +846,15 @@ impl<'b> AsyncDiffState<'b> { } VNode::Component(c) => { - let node = self.scopes.fin_head(c.scope.get().unwrap()); + log::info!("Replacing component {:?}", old); + let scope_id = c.scope.get().unwrap(); + let node = self.scopes.fin_head(scope_id); + + self.enter_scope(scope_id); + self.replace_inner(node, nodes_created); - let scope_id = c.scope.get().unwrap(); + log::info!("Replacing component x2 {:?}", old); // we can only remove components if they are actively being diffed if self.scope_stack.contains(&c.originator) { @@ -860,6 +862,7 @@ impl<'b> AsyncDiffState<'b> { self.scopes.try_remove(scope_id).unwrap(); } + self.leave_scope(); } } } @@ -902,15 +905,25 @@ impl<'b> AsyncDiffState<'b> { } VNode::Component(c) => { + self.enter_scope(c.scope.get().unwrap()); + let scope_id = c.scope.get().unwrap(); let root = self.scopes.root_node(scope_id); self.remove_nodes([root], gen_muts); + log::info!( + "trying to remove scope {:?}, stack is {:#?}, originator is {:?}", + scope_id, + self.scope_stack, + c.originator + ); + // we can only remove this node if the originator is actively if self.scope_stack.contains(&c.originator) { log::debug!("I am allowed to remove component because scope stack contains originator. Scope stack: {:#?} {:#?} {:#?}", self.scope_stack, c.originator, c.scope); self.scopes.try_remove(scope_id).unwrap(); } + self.leave_scope(); } } } diff --git a/packages/core/src/mutations.rs b/packages/core/src/mutations.rs index bd94d0b71c..5af7271542 100644 --- a/packages/core/src/mutations.rs +++ b/packages/core/src/mutations.rs @@ -17,7 +17,7 @@ use std::{any::Any, fmt::Debug}; /// Mutations are the only link between the RealDOM and the VirtualDOM. pub struct Mutations<'a> { pub edits: Vec>, - pub dirty_scopes: FxHashSet, + pub diffed_scopes: FxHashSet, pub refs: Vec>, } @@ -118,7 +118,7 @@ impl<'a> Mutations<'a> { Self { edits: Vec::new(), refs: Vec::new(), - dirty_scopes: Default::default(), + diffed_scopes: Default::default(), } } @@ -223,7 +223,7 @@ impl<'a> Mutations<'a> { } pub(crate) fn mark_dirty_scope(&mut self, scope: ScopeId) { - self.dirty_scopes.insert(scope); + self.diffed_scopes.insert(scope); } } diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 2f1453e1dd..e95f3f8255 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -184,6 +184,7 @@ impl Debug for VNode<'_> { } VNode::Component(comp) => s .debug_struct("VNode::VComponent") + .field("name", &comp.fn_name) .field("fnptr", &comp.user_fc) .field("key", &comp.key) .field("scope", &comp.scope) @@ -390,6 +391,7 @@ pub struct VComponent<'src> { pub scope: Cell>, pub can_memoize: bool, pub user_fc: FcSlot, + pub fn_name: &'static str, pub props: RefCell>>, } @@ -540,6 +542,7 @@ impl<'a> NodeFactory<'a> { component: fn(Scope<'a, P>) -> Element, props: P, key: Option, + fn_name: &'static str, ) -> VNode<'a> where P: Properties + 'a, @@ -550,6 +553,7 @@ impl<'a> NodeFactory<'a> { can_memoize: P::IS_STATIC, user_fc: component as *mut std::os::raw::c_void, originator: self.scope.scope_id(), + fn_name, props: RefCell::new(Some(Box::new(VComponentProps { // local_props: RefCell::new(Some(props)), // heap_props: RefCell::new(None), diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 93a0c94a78..351bcf4f71 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -113,13 +113,22 @@ impl ScopeArena { if let Some(old_scope) = self.free_scopes.borrow_mut().pop() { // reuse the old scope let scope = unsafe { &mut *old_scope }; - scope.props.get_mut().replace(vcomp); + + scope.container = container; + scope.our_arena_idx = new_scope_id; scope.parent_scope = parent_scope; scope.height = height; - scope.subtree.set(subtree); - scope.our_arena_idx = new_scope_id; - scope.container = container; scope.fnptr = fc_ptr; + scope.props.get_mut().replace(vcomp); + scope.subtree.set(subtree); + scope.frames[0].reset(); + scope.frames[1].reset(); + scope.shared_contexts.get_mut().clear(); + scope.items.get_mut().listeners.clear(); + scope.items.get_mut().borrowed_props.clear(); + scope.hook_idx.set(0); + scope.hook_vals.get_mut().clear(); + let any_item = self.scopes.borrow_mut().insert(new_scope_id, scope); debug_assert!(any_item.is_none()); } else { @@ -177,7 +186,7 @@ impl ScopeArena { let scope = unsafe { &mut *self.scopes.borrow_mut().remove(&id).unwrap() }; scope.reset(); - self.free_scopes.borrow_mut().push(scope); + // self.free_scopes.borrow_mut().push(scope); Some(()) } @@ -204,8 +213,9 @@ impl ScopeArena { } pub fn collect_garbage(&self, id: ElementId) { - let node = self.nodes.borrow_mut().remove(id.0); - log::debug!("collecting garbage for {:?}, {:?}", id, unsafe { &*node }); + let node = self.nodes.borrow_mut().get(id.0).unwrap().clone(); + // let node = self.nodes.borrow_mut().remove(id.0); + // log::debug!("collecting garbage for {:?}, {:?}", id, unsafe { &*node }); } /// This method cleans up any references to data held within our hook list. This prevents mutable aliasing from diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 613ea92924..9c4cd40184 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -462,14 +462,6 @@ impl VirtualDom { self.dirty_scopes .retain(|id| scopes.get_scope(*id).is_some()); - log::debug!("dirty_scopes: {:?}", self.dirty_scopes); - - for id in &self.dirty_scopes { - let scope = scopes.get_scope(*id).unwrap(); - - log::debug!("dirty scope: {:?} with height {:?}", id, scope.height); - } - self.dirty_scopes.sort_by(|a, b| { let h1 = scopes.get_scope(*a).unwrap().height; let h2 = scopes.get_scope(*b).unwrap().height; @@ -479,10 +471,7 @@ impl VirtualDom { log::debug!("dirty_scopes: {:?}", self.dirty_scopes); if let Some(scopeid) = self.dirty_scopes.pop() { - log::debug!("diffing scope {:?}", scopeid); if !ran_scopes.contains(&scopeid) { - log::debug!("running scope scope {:?}", scopeid); - ran_scopes.insert(scopeid); self.scopes.run_scope(scopeid); @@ -491,7 +480,8 @@ impl VirtualDom { let AsyncDiffState { mutations, .. } = diff_state; - for scope in &mutations.dirty_scopes { + log::debug!("succesffuly resolved scopes {:?}", mutations.diffed_scopes); + for scope in &mutations.diffed_scopes { self.dirty_scopes.remove(scope); } From 5bffbba682c634b2882d43e574d7415981f0d45e Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 02:40:12 -0500 Subject: [PATCH 06/14] fmt: apply cargofmt with new rules for lit structs --- packages/core/src/events.rs | 4 +- packages/core/src/lazynodes.rs | 27 +----- packages/core/src/mutations.rs | 34 ++----- packages/core/src/nodes.rs | 6 +- packages/core/src/scopes.rs | 10 +- packages/core/src/util.rs | 5 +- packages/core/tests/create_dom.rs | 140 ++++++---------------------- packages/core/tests/diffing.rs | 1 - packages/core/tests/miri_stress.rs | 7 +- packages/core/tests/sharedstate.rs | 5 +- packages/core/tests/vdom_rebuild.rs | 10 +- 11 files changed, 54 insertions(+), 195 deletions(-) diff --git a/packages/core/src/events.rs b/packages/core/src/events.rs index 8386cedd8a..597a129923 100644 --- a/packages/core/src/events.rs +++ b/packages/core/src/events.rs @@ -12,9 +12,7 @@ pub(crate) struct BubbleState { impl BubbleState { pub fn new() -> Self { - Self { - canceled: Cell::new(false), - } + Self { canceled: Cell::new(false) } } } diff --git a/packages/core/src/lazynodes.rs b/packages/core/src/lazynodes.rs index 1340c916c5..0fbed45e16 100644 --- a/packages/core/src/lazynodes.rs +++ b/packages/core/src/lazynodes.rs @@ -55,9 +55,7 @@ impl<'a, 'b> LazyNodes<'a, 'b> { fac.map(inner) }; - Self { - inner: StackNodeStorage::Heap(Box::new(val)), - } + Self { inner: StackNodeStorage::Heap(Box::new(val)) } } pub fn new(_val: impl FnOnce(NodeFactory<'a>) -> VNode<'a> + 'b) -> Self { @@ -71,9 +69,7 @@ impl<'a, 'b> LazyNodes<'a, 'b> { // miri does not know how to work with mucking directly into bytes if cfg!(miri) { - Self { - inner: StackNodeStorage::Heap(Box::new(val)), - } + Self { inner: StackNodeStorage::Heap(Box::new(val)) } } else { unsafe { LazyNodes::new_inner(val) } } @@ -117,9 +113,7 @@ impl<'a, 'b> LazyNodes<'a, 'b> { let max_size = mem::size_of::(); if stored_size > max_size { - Self { - inner: StackNodeStorage::Heap(Box::new(val)), - } + Self { inner: StackNodeStorage::Heap(Box::new(val)) } } else { let mut buf: StackHeapSize = StackHeapSize::default(); @@ -143,13 +137,7 @@ impl<'a, 'b> LazyNodes<'a, 'b> { std::mem::forget(val); - Self { - inner: StackNodeStorage::Stack(LazyStack { - _align: [], - buf, - dropped: false, - }), - } + Self { inner: StackNodeStorage::Stack(LazyStack { _align: [], buf, dropped: false }) } } } @@ -298,12 +286,7 @@ mod tests { } let inner = Rc::new(0); - let mut dom = VirtualDom::new_with_props( - app, - AppProps { - inner: inner.clone(), - }, - ); + let mut dom = VirtualDom::new_with_props(app, AppProps { inner: inner.clone() }); dom.rebuild(); drop(dom); diff --git a/packages/core/src/mutations.rs b/packages/core/src/mutations.rs index 5af7271542..6a0785e233 100644 --- a/packages/core/src/mutations.rs +++ b/packages/core/src/mutations.rs @@ -115,11 +115,7 @@ use DomEdit::*; impl<'a> Mutations<'a> { pub(crate) fn new() -> Self { - Self { - edits: Vec::new(), - refs: Vec::new(), - diffed_scopes: Default::default(), - } + Self { edits: Vec::new(), refs: Vec::new(), diffed_scopes: Default::default() } } // Navigation @@ -178,19 +174,12 @@ impl<'a> Mutations<'a> { // events pub(crate) fn new_event_listener(&mut self, listener: &Listener, scope: ScopeId) { - let Listener { - event, - mounted_node, - .. - } = listener; + let Listener { event, mounted_node, .. } = listener; let element_id = mounted_node.get().unwrap().as_u64(); - self.edits.push(NewEventListener { - scope, - event_name: event, - root: element_id, - }); + self.edits + .push(NewEventListener { scope, event_name: event, root: element_id }); } pub(crate) fn remove_event_listener(&mut self, event: &'static str, root: u64) { self.edits.push(RemoveEventListener { event, root }); @@ -202,19 +191,10 @@ impl<'a> Mutations<'a> { } pub(crate) fn set_attribute(&mut self, attribute: &'a Attribute, root: u64) { - let Attribute { - name, - value, - namespace, - .. - } = attribute; + let Attribute { name, value, namespace, .. } = attribute; - self.edits.push(SetAttribute { - field: name, - value, - ns: *namespace, - root, - }); + self.edits + .push(SetAttribute { field: name, value, ns: *namespace, root }); } pub(crate) fn remove_attribute(&mut self, attribute: &Attribute, root: u64) { diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index e95f3f8255..264fb56bb8 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -4,7 +4,7 @@ //! cheap and *very* fast to construct - building a full tree should be quick. use crate::{ - innerlude::{Element, FcSlot, Properties, Scope, ScopeId, ScopeState}, + innerlude::{ComponentPtr, Element, Properties, Scope, ScopeId, ScopeState}, lazynodes::LazyNodes, AnyEvent, Component, }; @@ -390,7 +390,7 @@ pub struct VComponent<'src> { pub originator: ScopeId, pub scope: Cell>, pub can_memoize: bool, - pub user_fc: FcSlot, + pub user_fc: ComponentPtr, pub fn_name: &'static str, pub props: RefCell>>, } @@ -551,7 +551,7 @@ impl<'a> NodeFactory<'a> { key: key.map(|f| self.raw_text(f).0), scope: Default::default(), can_memoize: P::IS_STATIC, - user_fc: component as *mut std::os::raw::c_void, + user_fc: component as ComponentPtr, originator: self.scope.scope_id(), fn_name, props: RefCell::new(Some(Box::new(VComponentProps { diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 351bcf4f71..6b65c9197d 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -14,8 +14,8 @@ use std::{ }; /// for traceability, we use the raw fn pointer to identify the function -/// we can use this with the traceback crate to resolve funciton names -pub(crate) type FcSlot = *mut std::os::raw::c_void; +/// we also get the component name, but that's not necessarily unique in the app +pub(crate) type ComponentPtr = *mut std::os::raw::c_void; pub(crate) struct Heuristic { hook_arena_size: usize, @@ -30,7 +30,7 @@ pub(crate) struct ScopeArena { pub scope_gen: Cell, pub bump: Bump, pub scopes: RefCell>, - pub heuristics: RefCell>, + pub heuristics: RefCell>, pub free_scopes: RefCell>, pub nodes: RefCell>>, pub tasks: Rc, @@ -84,7 +84,7 @@ impl ScopeArena { pub(crate) fn new_with_key( &self, - fc_ptr: FcSlot, + fc_ptr: ComponentPtr, vcomp: Box, parent_scope: Option, container: ElementId, @@ -469,7 +469,7 @@ pub struct ScopeState { pub(crate) container: ElementId, pub(crate) our_arena_idx: ScopeId, pub(crate) height: u32, - pub(crate) fnptr: FcSlot, + pub(crate) fnptr: ComponentPtr, // todo: subtrees pub(crate) is_subtree_root: Cell, diff --git a/packages/core/src/util.rs b/packages/core/src/util.rs index b9640792d5..24caab579e 100644 --- a/packages/core/src/util.rs +++ b/packages/core/src/util.rs @@ -10,10 +10,7 @@ pub struct ElementIdIterator<'a> { impl<'a> ElementIdIterator<'a> { pub fn new(vdom: &'a VirtualDom, node: &'a VNode<'a>) -> Self { - Self { - vdom, - stack: smallvec::smallvec![(0, node)], - } + Self { vdom, stack: smallvec::smallvec![(0, node)] } } } diff --git a/packages/core/tests/create_dom.rs b/packages/core/tests/create_dom.rs index 044be289c7..0cdc37c6dc 100644 --- a/packages/core/tests/create_dom.rs +++ b/packages/core/tests/create_dom.rs @@ -36,18 +36,9 @@ fn test_original_diff() { assert_eq!( mutations.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateElement { - root: 2, - tag: "div" - }, - CreateTextNode { - root: 3, - text: "Hello, world!" - }, + CreateElement { root: 1, tag: "div" }, + CreateElement { root: 2, tag: "div" }, + CreateTextNode { root: 3, text: "Hello, world!" }, AppendChildren { many: 1 }, AppendChildren { many: 1 }, AppendChildren { many: 1 }, @@ -81,34 +72,13 @@ fn create() { assert_eq!( mutations.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateElement { - root: 2, - tag: "div" - }, - CreateTextNode { - root: 3, - text: "Hello, world!" - }, - CreateElement { - root: 4, - tag: "div" - }, - CreateElement { - root: 5, - tag: "div" - }, - CreateTextNode { - root: 6, - text: "hello" - }, - CreateTextNode { - root: 7, - text: "world" - }, + CreateElement { root: 1, tag: "div" }, + CreateElement { root: 2, tag: "div" }, + CreateTextNode { root: 3, text: "Hello, world!" }, + CreateElement { root: 4, tag: "div" }, + CreateElement { root: 5, tag: "div" }, + CreateTextNode { root: 6, text: "hello" }, + CreateTextNode { root: 7, text: "world" }, AppendChildren { many: 2 }, AppendChildren { many: 1 }, AppendChildren { many: 2 }, @@ -135,32 +105,14 @@ fn create_list() { assert_eq!( mutations.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateTextNode { - root: 2, - text: "hello" - }, + CreateElement { root: 1, tag: "div" }, + CreateTextNode { root: 2, text: "hello" }, AppendChildren { many: 1 }, - CreateElement { - root: 3, - tag: "div" - }, - CreateTextNode { - root: 4, - text: "hello" - }, + CreateElement { root: 3, tag: "div" }, + CreateTextNode { root: 4, text: "hello" }, AppendChildren { many: 1 }, - CreateElement { - root: 5, - tag: "div" - }, - CreateTextNode { - root: 6, - text: "hello" - }, + CreateElement { root: 5, tag: "div" }, + CreateTextNode { root: 6, text: "hello" }, AppendChildren { many: 1 }, AppendChildren { many: 3 }, ] @@ -185,22 +137,10 @@ fn create_simple() { assert_eq!( mutations.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateElement { - root: 2, - tag: "div" - }, - CreateElement { - root: 3, - tag: "div" - }, - CreateElement { - root: 4, - tag: "div" - }, + CreateElement { root: 1, tag: "div" }, + CreateElement { root: 2, tag: "div" }, + CreateElement { root: 3, tag: "div" }, + CreateElement { root: 4, tag: "div" }, AppendChildren { many: 4 }, ] ); @@ -235,36 +175,18 @@ fn create_components() { mutations.edits, [ CreateElement { root: 1, tag: "h1" }, - CreateElement { - root: 2, - tag: "div" - }, - CreateTextNode { - root: 3, - text: "abc1" - }, + CreateElement { root: 2, tag: "div" }, + CreateTextNode { root: 3, text: "abc1" }, AppendChildren { many: 1 }, CreateElement { root: 4, tag: "p" }, CreateElement { root: 5, tag: "h1" }, - CreateElement { - root: 6, - tag: "div" - }, - CreateTextNode { - root: 7, - text: "abc2" - }, + CreateElement { root: 6, tag: "div" }, + CreateTextNode { root: 7, text: "abc2" }, AppendChildren { many: 1 }, CreateElement { root: 8, tag: "p" }, CreateElement { root: 9, tag: "h1" }, - CreateElement { - root: 10, - tag: "div" - }, - CreateTextNode { - root: 11, - text: "abc3" - }, + CreateElement { root: 10, tag: "div" }, + CreateTextNode { root: 11, text: "abc3" }, AppendChildren { many: 1 }, CreateElement { root: 12, tag: "p" }, AppendChildren { many: 9 }, @@ -285,14 +207,8 @@ fn anchors() { assert_eq!( mutations.edits, [ - CreateElement { - root: 1, - tag: "div" - }, - CreateTextNode { - root: 2, - text: "hello" - }, + CreateElement { root: 1, tag: "div" }, + CreateTextNode { root: 2, text: "hello" }, AppendChildren { many: 1 }, CreatePlaceholder { root: 3 }, AppendChildren { many: 2 }, diff --git a/packages/core/tests/diffing.rs b/packages/core/tests/diffing.rs index c407359c9e..6bc9b5596b 100644 --- a/packages/core/tests/diffing.rs +++ b/packages/core/tests/diffing.rs @@ -580,7 +580,6 @@ fn keyed_diffing_additions_and_moves_in_middle() { }) }); - // LIS: 4, 5, 6 let (_, change) = dom.diff_lazynodes(left, right); log::debug!("{:#?}", change); diff --git a/packages/core/tests/miri_stress.rs b/packages/core/tests/miri_stress.rs index 0f4f64546d..8ce0301478 100644 --- a/packages/core/tests/miri_stress.rs +++ b/packages/core/tests/miri_stress.rs @@ -152,12 +152,7 @@ fn free_works_on_root_props() { } } - let mut dom = new_dom( - app, - Custom { - val: String::from("asd"), - }, - ); + let mut dom = new_dom(app, Custom { val: String::from("asd") }); dom.rebuild(); } diff --git a/packages/core/tests/sharedstate.rs b/packages/core/tests/sharedstate.rs index 41b1787938..70e8235692 100644 --- a/packages/core/tests/sharedstate.rs +++ b/packages/core/tests/sharedstate.rs @@ -29,10 +29,7 @@ fn shared_state_test() { assert_eq!( edits, [ - CreateTextNode { - root: 1, - text: "Hello, world!" - }, + CreateTextNode { root: 1, text: "Hello, world!" }, AppendChildren { many: 1 }, ] ); diff --git a/packages/core/tests/vdom_rebuild.rs b/packages/core/tests/vdom_rebuild.rs index 5d1dbcdf80..83b4ff35f7 100644 --- a/packages/core/tests/vdom_rebuild.rs +++ b/packages/core/tests/vdom_rebuild.rs @@ -68,15 +68,9 @@ fn conditional_rendering() { mutations.edits, [ CreateElement { root: 1, tag: "h1" }, - CreateTextNode { - root: 2, - text: "hello" - }, + CreateTextNode { root: 2, text: "hello" }, AppendChildren { many: 1 }, - CreateElement { - root: 3, - tag: "span" - }, + CreateElement { root: 3, tag: "span" }, CreateTextNode { root: 4, text: "a" }, AppendChildren { many: 1 }, CreatePlaceholder { root: 5 }, From 00aa0e5e86d298c8498b15c742d0bbe3981dd649 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 02:44:16 -0500 Subject: [PATCH 07/14] fmt: apply formatting just to tests --- packages/core/src/events.rs | 4 +- packages/core/src/lazynodes.rs | 27 +++++++++--- packages/core/src/mutations.rs | 34 ++++++++++++--- packages/core/src/nodes.rs | 58 +++++++++++++++++-------- packages/core/src/scopes.rs | 29 ++++++++++--- packages/core/src/util.rs | 5 ++- packages/core/{ => tests}/.rustfmt.toml | 0 7 files changed, 119 insertions(+), 38 deletions(-) rename packages/core/{ => tests}/.rustfmt.toml (100%) diff --git a/packages/core/src/events.rs b/packages/core/src/events.rs index 597a129923..8386cedd8a 100644 --- a/packages/core/src/events.rs +++ b/packages/core/src/events.rs @@ -12,7 +12,9 @@ pub(crate) struct BubbleState { impl BubbleState { pub fn new() -> Self { - Self { canceled: Cell::new(false) } + Self { + canceled: Cell::new(false), + } } } diff --git a/packages/core/src/lazynodes.rs b/packages/core/src/lazynodes.rs index 0fbed45e16..1340c916c5 100644 --- a/packages/core/src/lazynodes.rs +++ b/packages/core/src/lazynodes.rs @@ -55,7 +55,9 @@ impl<'a, 'b> LazyNodes<'a, 'b> { fac.map(inner) }; - Self { inner: StackNodeStorage::Heap(Box::new(val)) } + Self { + inner: StackNodeStorage::Heap(Box::new(val)), + } } pub fn new(_val: impl FnOnce(NodeFactory<'a>) -> VNode<'a> + 'b) -> Self { @@ -69,7 +71,9 @@ impl<'a, 'b> LazyNodes<'a, 'b> { // miri does not know how to work with mucking directly into bytes if cfg!(miri) { - Self { inner: StackNodeStorage::Heap(Box::new(val)) } + Self { + inner: StackNodeStorage::Heap(Box::new(val)), + } } else { unsafe { LazyNodes::new_inner(val) } } @@ -113,7 +117,9 @@ impl<'a, 'b> LazyNodes<'a, 'b> { let max_size = mem::size_of::(); if stored_size > max_size { - Self { inner: StackNodeStorage::Heap(Box::new(val)) } + Self { + inner: StackNodeStorage::Heap(Box::new(val)), + } } else { let mut buf: StackHeapSize = StackHeapSize::default(); @@ -137,7 +143,13 @@ impl<'a, 'b> LazyNodes<'a, 'b> { std::mem::forget(val); - Self { inner: StackNodeStorage::Stack(LazyStack { _align: [], buf, dropped: false }) } + Self { + inner: StackNodeStorage::Stack(LazyStack { + _align: [], + buf, + dropped: false, + }), + } } } @@ -286,7 +298,12 @@ mod tests { } let inner = Rc::new(0); - let mut dom = VirtualDom::new_with_props(app, AppProps { inner: inner.clone() }); + let mut dom = VirtualDom::new_with_props( + app, + AppProps { + inner: inner.clone(), + }, + ); dom.rebuild(); drop(dom); diff --git a/packages/core/src/mutations.rs b/packages/core/src/mutations.rs index 6a0785e233..5af7271542 100644 --- a/packages/core/src/mutations.rs +++ b/packages/core/src/mutations.rs @@ -115,7 +115,11 @@ use DomEdit::*; impl<'a> Mutations<'a> { pub(crate) fn new() -> Self { - Self { edits: Vec::new(), refs: Vec::new(), diffed_scopes: Default::default() } + Self { + edits: Vec::new(), + refs: Vec::new(), + diffed_scopes: Default::default(), + } } // Navigation @@ -174,12 +178,19 @@ impl<'a> Mutations<'a> { // events pub(crate) fn new_event_listener(&mut self, listener: &Listener, scope: ScopeId) { - let Listener { event, mounted_node, .. } = listener; + let Listener { + event, + mounted_node, + .. + } = listener; let element_id = mounted_node.get().unwrap().as_u64(); - self.edits - .push(NewEventListener { scope, event_name: event, root: element_id }); + self.edits.push(NewEventListener { + scope, + event_name: event, + root: element_id, + }); } pub(crate) fn remove_event_listener(&mut self, event: &'static str, root: u64) { self.edits.push(RemoveEventListener { event, root }); @@ -191,10 +202,19 @@ impl<'a> Mutations<'a> { } pub(crate) fn set_attribute(&mut self, attribute: &'a Attribute, root: u64) { - let Attribute { name, value, namespace, .. } = attribute; + let Attribute { + name, + value, + namespace, + .. + } = attribute; - self.edits - .push(SetAttribute { field: name, value, ns: *namespace, root }); + self.edits.push(SetAttribute { + field: name, + value, + ns: *namespace, + root, + }); } pub(crate) fn remove_attribute(&mut self, attribute: &Attribute, root: u64) { diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 264fb56bb8..b849819f0d 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -365,7 +365,9 @@ pub struct EventHandler<'bump, T = ()> { impl<'a, T> Default for EventHandler<'a, T> { fn default() -> Self { - Self { callback: RefCell::new(None) } + Self { + callback: RefCell::new(None), + } } } @@ -440,7 +442,10 @@ pub struct NodeFactory<'a> { impl<'a> NodeFactory<'a> { pub fn new(scope: &'a ScopeState) -> NodeFactory<'a> { - NodeFactory { scope, bump: &scope.wip_frame().bump } + NodeFactory { + scope, + bump: &scope.wip_frame().bump, + } } #[inline] @@ -450,10 +455,11 @@ impl<'a> NodeFactory<'a> { /// Directly pass in text blocks without the need to use the format_args macro. pub fn static_text(&self, text: &'static str) -> VNode<'a> { - VNode::Text( - self.bump - .alloc(VText { id: empty_cell(), text, is_static: true }), - ) + VNode::Text(self.bump.alloc(VText { + id: empty_cell(), + text, + is_static: true, + })) } /// Parses a lazy text Arguments and returns a string and a flag indicating if the text is 'static @@ -476,7 +482,11 @@ impl<'a> NodeFactory<'a> { pub fn text(&self, args: Arguments) -> VNode<'a> { let (text, is_static) = self.raw_text(args); - VNode::Text(self.bump.alloc(VText { text, is_static, id: empty_cell() })) + VNode::Text(self.bump.alloc(VText { + text, + is_static, + id: empty_cell(), + })) } pub fn element( @@ -534,7 +544,13 @@ impl<'a> NodeFactory<'a> { is_volatile: bool, ) -> Attribute<'a> { let (value, is_static) = self.raw_text(val); - Attribute { name, value, is_static, namespace, is_volatile } + Attribute { + name, + value, + is_static, + namespace, + is_volatile, + } } pub fn component

( @@ -578,7 +594,11 @@ impl<'a> NodeFactory<'a> { } pub fn listener(self, event: &'static str, callback: InternalHandler<'a>) -> Listener<'a> { - Listener { event, mounted_node: Cell::new(None), callback } + Listener { + event, + mounted_node: Cell::new(None), + callback, + } } pub fn fragment_root<'b, 'c>( @@ -594,10 +614,10 @@ impl<'a> NodeFactory<'a> { if nodes.is_empty() { VNode::Placeholder(self.bump.alloc(VPlaceholder { id: empty_cell() })) } else { - VNode::Fragment( - self.bump - .alloc(VFragment { children: nodes.into_bump_slice(), key: None }), - ) + VNode::Fragment(self.bump.alloc(VFragment { + children: nodes.into_bump_slice(), + key: None, + })) } } @@ -633,7 +653,10 @@ impl<'a> NodeFactory<'a> { ); } - VNode::Fragment(self.bump.alloc(VFragment { children, key: None })) + VNode::Fragment(self.bump.alloc(VFragment { + children, + key: None, + })) } } @@ -656,9 +679,10 @@ impl<'a> NodeFactory<'a> { } else { let children = nodes.into_bump_slice(); - Some(VNode::Fragment( - self.bump.alloc(VFragment { children, key: None }), - )) + Some(VNode::Fragment(self.bump.alloc(VFragment { + children, + key: None, + }))) } } diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 6b65c9197d..c6c0cb846f 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -315,9 +315,11 @@ impl ScopeArena { frame.node.set(unsafe { extend_vnode(node) }); } else { let frame = scope.wip_frame(); - let node = frame.bump.alloc(VNode::Placeholder( - frame.bump.alloc(VPlaceholder { id: Default::default() }), - )); + let node = frame + .bump + .alloc(VNode::Placeholder(frame.bump.alloc(VPlaceholder { + id: Default::default(), + }))); frame.node.set(unsafe { extend_vnode(node) }); } @@ -427,7 +429,10 @@ pub struct Scope<'a, P = ()> { impl

Copy for Scope<'_, P> {} impl

Clone for Scope<'_, P> { fn clone(&self) -> Self { - Self { scope: self.scope, props: self.props } + Self { + scope: self.scope, + props: self.props, + } } } @@ -789,7 +794,10 @@ impl ScopeState { /// } ///``` pub fn render<'src>(&'src self, rsx: LazyNodes<'src, '_>) -> Option> { - Some(rsx.call(NodeFactory { scope: self, bump: &self.wip_frame().bump })) + Some(rsx.call(NodeFactory { + scope: self, + bump: &self.wip_frame().bump, + })) } /// Store a value between renders @@ -896,7 +904,10 @@ impl ScopeState { self.shared_contexts.get_mut().clear(); // next: reset the node data - let SelfReferentialItems { borrowed_props, listeners } = self.items.get_mut(); + let SelfReferentialItems { + borrowed_props, + listeners, + } = self.items.get_mut(); borrowed_props.clear(); listeners.clear(); self.frames[0].reset(); @@ -954,7 +965,11 @@ pub(crate) struct TaskQueue { pub(crate) type InnerTask = Pin>>; impl TaskQueue { fn new(sender: UnboundedSender) -> Rc { - Rc::new(Self { tasks: RefCell::new(FxHashMap::default()), gen: Cell::new(0), sender }) + Rc::new(Self { + tasks: RefCell::new(FxHashMap::default()), + gen: Cell::new(0), + sender, + }) } fn push_fut(&self, task: impl Future + 'static) -> TaskId { let pinned = Box::pin(task); diff --git a/packages/core/src/util.rs b/packages/core/src/util.rs index 24caab579e..b9640792d5 100644 --- a/packages/core/src/util.rs +++ b/packages/core/src/util.rs @@ -10,7 +10,10 @@ pub struct ElementIdIterator<'a> { impl<'a> ElementIdIterator<'a> { pub fn new(vdom: &'a VirtualDom, node: &'a VNode<'a>) -> Self { - Self { vdom, stack: smallvec::smallvec![(0, node)] } + Self { + vdom, + stack: smallvec::smallvec![(0, node)], + } } } diff --git a/packages/core/.rustfmt.toml b/packages/core/tests/.rustfmt.toml similarity index 100% rename from packages/core/.rustfmt.toml rename to packages/core/tests/.rustfmt.toml From a4ea0ba4fe3b615179b01fab3c4704a938d5a5e4 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 02:52:40 -0500 Subject: [PATCH 08/14] chore: undo dirty_scopes rename --- packages/core/src/mutations.rs | 6 +++--- packages/core/src/virtual_dom.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/core/src/mutations.rs b/packages/core/src/mutations.rs index 5af7271542..bd94d0b71c 100644 --- a/packages/core/src/mutations.rs +++ b/packages/core/src/mutations.rs @@ -17,7 +17,7 @@ use std::{any::Any, fmt::Debug}; /// Mutations are the only link between the RealDOM and the VirtualDOM. pub struct Mutations<'a> { pub edits: Vec>, - pub diffed_scopes: FxHashSet, + pub dirty_scopes: FxHashSet, pub refs: Vec>, } @@ -118,7 +118,7 @@ impl<'a> Mutations<'a> { Self { edits: Vec::new(), refs: Vec::new(), - diffed_scopes: Default::default(), + dirty_scopes: Default::default(), } } @@ -223,7 +223,7 @@ impl<'a> Mutations<'a> { } pub(crate) fn mark_dirty_scope(&mut self, scope: ScopeId) { - self.diffed_scopes.insert(scope); + self.dirty_scopes.insert(scope); } } diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 9c4cd40184..b273e62391 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -480,8 +480,8 @@ impl VirtualDom { let AsyncDiffState { mutations, .. } = diff_state; - log::debug!("succesffuly resolved scopes {:?}", mutations.diffed_scopes); - for scope in &mutations.diffed_scopes { + log::debug!("succesffuly resolved scopes {:?}", mutations.dirty_scopes); + for scope in &mutations.dirty_scopes { self.dirty_scopes.remove(scope); } From 9dda7b168b15e31f739780c04b772d9dfa09066e Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 03:04:47 -0500 Subject: [PATCH 09/14] chore: clean up scopes --- packages/core/src/scopes.rs | 7 +------ packages/core/src/virtual_dom.rs | 4 ++-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index c6c0cb846f..b90ff6f6e2 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -213,9 +213,7 @@ impl ScopeArena { } pub fn collect_garbage(&self, id: ElementId) { - let node = self.nodes.borrow_mut().get(id.0).unwrap().clone(); - // let node = self.nodes.borrow_mut().remove(id.0); - // log::debug!("collecting garbage for {:?}, {:?}", id, unsafe { &*node }); + self.nodes.borrow_mut().remove(id.0); } /// This method cleans up any references to data held within our hook list. This prevents mutable aliasing from @@ -231,8 +229,6 @@ impl ScopeArena { /// This also makes sure that drop order is consistent and predictable. All resources that rely on being dropped will /// be dropped. pub(crate) fn ensure_drop_safety(&self, scope_id: ScopeId) { - // log::trace!("Ensuring drop safety for scope {:?}", scope_id); - if let Some(scope) = self.get_scope(scope_id) { let mut items = scope.items.borrow_mut(); @@ -243,7 +239,6 @@ impl ScopeArena { if let Some(scope_id) = comp.scope.get() { self.ensure_drop_safety(scope_id); } - drop(comp.props.take()); }); diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index b273e62391..670613dd45 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -449,6 +449,7 @@ impl VirtualDom { /// apply_mutations(mutations); /// } /// ``` + #[allow(unused)] pub fn work_with_deadline(&mut self, mut deadline: impl FnMut() -> bool) -> Vec { let mut committed_mutations = vec![]; @@ -487,13 +488,12 @@ impl VirtualDom { committed_mutations.push(mutations); + // todo: pause the diff machine // if diff_state.work(&mut deadline) { // let DiffState { mutations, .. } = diff_state; - // for scope in &mutations.dirty_scopes { // self.dirty_scopes.remove(scope); // } - // committed_mutations.push(mutations); // } else { // // leave the work in an incomplete state From b4697fc9f93377c497a8eb25fa56c4e3c347c4ef Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 03:27:43 -0500 Subject: [PATCH 10/14] chore: clean up some more of the scopes file --- packages/core/src/diff.rs | 4 ++-- packages/core/src/scopes.rs | 34 +++++++------------------------- packages/core/src/virtual_dom.rs | 6 +++--- 3 files changed, 12 insertions(+), 32 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index db898f194a..572ad7f664 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -2,7 +2,7 @@ use crate::innerlude::*; use fxhash::{FxHashMap, FxHashSet}; use smallvec::{smallvec, SmallVec}; -pub(crate) struct AsyncDiffState<'bump> { +pub(crate) struct DiffState<'bump> { pub(crate) scopes: &'bump ScopeArena, pub(crate) mutations: Mutations<'bump>, pub(crate) force_diff: bool, @@ -10,7 +10,7 @@ pub(crate) struct AsyncDiffState<'bump> { pub(crate) scope_stack: SmallVec<[ScopeId; 5]>, } -impl<'b> AsyncDiffState<'b> { +impl<'b> DiffState<'b> { pub fn new(scopes: &'b ScopeArena) -> Self { Self { scopes, diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index b90ff6f6e2..ea935b4336 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -186,7 +186,7 @@ impl ScopeArena { let scope = unsafe { &mut *self.scopes.borrow_mut().remove(&id).unwrap() }; scope.reset(); - // self.free_scopes.borrow_mut().push(scope); + self.free_scopes.borrow_mut().push(scope); Some(()) } @@ -202,14 +202,11 @@ impl ScopeArena { } pub fn update_node<'a>(&self, node: &'a VNode<'a>, id: ElementId) { - let node = unsafe { extend_vnode(node) }; - - let mut nodes = self.nodes.borrow_mut(); - let entry = nodes.get_mut(id.0); - match entry { - Some(_node) => *_node = node, - None => panic!("cannot update node {}", id), - } + *self + .nodes + .borrow_mut() + .get_mut(id.0) + .expect("node to exist if it's being updated") = unsafe { extend_vnode(node) }; } pub fn collect_garbage(&self, id: ElementId) { @@ -259,24 +256,13 @@ impl ScopeArena { // todo: we *know* that this is aliased by the contents of the scope itself let scope = unsafe { &mut *self.get_scope_raw(id).expect("could not find scope") }; - // if cfg!(debug_assertions) { - // log::debug!("running scope {:?} symbol: {:?}", id, scope.fnptr); - - // todo: resolve frames properly - backtrace::resolve(scope.fnptr, |symbol| { - // backtrace::resolve(scope.fnptr as *mut std::os::raw::c_void, |symbol| { - // panic!("asd"); - // log::trace!("Running scope {:?}, ptr {:?}", id, scope.fnptr); - log::debug!("running scope {:?} symbol: {:?}", id, symbol.name()); - }); - // } + log::trace!("running scope {:?} symbol: {:?}", id, scope.fnptr); // Safety: // - We dropped the listeners, so no more &mut T can be used while these are held // - All children nodes that rely on &mut T are replaced with a new reference scope.hook_idx.set(0); - // book keeping to ensure safety around the borrowed data { // Safety: // - We've dropped all references to the wip bump frame with "ensure_drop_safety" @@ -289,8 +275,6 @@ impl ScopeArena { debug_assert!(items.borrowed_props.is_empty()); } - // safety: this is definitely not dropped - /* If the component returns None, then we fill in a placeholder node. This will wipe what was there. An alternate approach is to leave the Real Dom the same, but that can lead to safety issues and a lot more checks. @@ -332,14 +316,10 @@ impl ScopeArena { while let Some(id) = cur_el.take() { if let Some(el) = nodes.get(id.0) { - log::trace!("Found valid receiver element"); - let real_el = unsafe { &**el }; if let VNode::Element(real_el) = real_el { for listener in real_el.listeners.borrow().iter() { if listener.event == event.name { - log::trace!("Found valid receiver event"); - if state.canceled.get() { // stop bubbling if canceled break; diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 5af569de10..a7092104c0 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -2,7 +2,7 @@ //! //! This module provides the primary mechanics to create a hook-based, concurrent VDOM for Rust. -use crate::diff::AsyncDiffState as DiffState; +use crate::diff::DiffState; use crate::innerlude::*; use futures_channel::mpsc::{UnboundedReceiver, UnboundedSender}; use futures_util::{future::poll_fn, StreamExt}; @@ -455,7 +455,7 @@ impl VirtualDom { while !self.dirty_scopes.is_empty() { let scopes = &self.scopes; - let mut diff_state = AsyncDiffState::new(scopes); + let mut diff_state = DiffState::new(scopes); let mut ran_scopes = FxHashSet::default(); @@ -479,7 +479,7 @@ impl VirtualDom { diff_state.diff_scope(scopeid); - let AsyncDiffState { mutations, .. } = diff_state; + let DiffState { mutations, .. } = diff_state; log::debug!("succesffuly resolved scopes {:?}", mutations.dirty_scopes); for scope in &mutations.dirty_scopes { From 923fb0701d91eafb9f7225ba376154b072728645 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 03:34:52 -0500 Subject: [PATCH 11/14] fix: clippy --- packages/core/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/lib.rs b/packages/core/src/lib.rs index dba4effbbc..e81ddca870 100644 --- a/packages/core/src/lib.rs +++ b/packages/core/src/lib.rs @@ -12,7 +12,6 @@ pub(crate) mod util; pub(crate) mod virtual_dom; pub(crate) mod innerlude { - pub(crate) use crate::diff::*; pub use crate::events::*; pub use crate::lazynodes::*; pub use crate::mutations::*; From 06418f73db4bf3721d2d9ca9bf2fdd184663158d Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 12:29:11 -0500 Subject: [PATCH 12/14] fix: element stack not being updated properly --- packages/core/src/diff.rs | 2 ++ packages/core/src/scopes.rs | 15 +++++++------- packages/router/src/service.rs | 37 +--------------------------------- 3 files changed, 10 insertions(+), 44 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 572ad7f664..1a53b01c4f 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -166,6 +166,8 @@ impl<'b> DiffState<'b> { self.create_and_append_children(children); } + self.element_stack.pop(); + 1 } diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index ea935b4336..00c3024bf0 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -96,10 +96,9 @@ impl ScopeArena { // Get the height of the scope let height = parent_scope - .map(|id| self.get_scope(id).map(|scope| scope.height)) + .map(|id| self.get_scope(id).map(|scope| scope.height + 1)) .flatten() - .unwrap_or_default() - + 1; + .unwrap_or_default(); let parent_scope = parent_scope.map(|f| self.get_scope_raw(f)).flatten(); @@ -202,11 +201,8 @@ impl ScopeArena { } pub fn update_node<'a>(&self, node: &'a VNode<'a>, id: ElementId) { - *self - .nodes - .borrow_mut() - .get_mut(id.0) - .expect("node to exist if it's being updated") = unsafe { extend_vnode(node) }; + let node = unsafe { extend_vnode(node) }; + *self.nodes.borrow_mut().get_mut(id.0).unwrap() = node; } pub fn collect_garbage(&self, id: ElementId) { @@ -317,9 +313,12 @@ impl ScopeArena { while let Some(id) = cur_el.take() { if let Some(el) = nodes.get(id.0) { let real_el = unsafe { &**el }; + log::debug!("looking for listener on {:?}", real_el); + if let VNode::Element(real_el) = real_el { for listener in real_el.listeners.borrow().iter() { if listener.event == event.name { + log::debug!("calling listener {:?}", listener.event); if state.canceled.get() { // stop bubbling if canceled break; diff --git a/packages/router/src/service.rs b/packages/router/src/service.rs index b6d8ba11b7..a4ab4dc0d5 100644 --- a/packages/router/src/service.rs +++ b/packages/router/src/service.rs @@ -62,12 +62,10 @@ impl RouterService { root_found.set(None); // checking if the route is valid is cheap, so we do it for (slot, root) in slots.borrow_mut().iter().rev() { - // log::trace!("regenerating slot {:?} for root '{}'", slot, root); regen_route(*slot); } for listener in onchange_listeners.borrow_mut().iter() { - // log::trace!("regenerating listener {:?}", listener); regen_route(*listener); } @@ -91,31 +89,24 @@ impl RouterService { } pub fn push_route(&self, route: &str) { - // log::trace!("Pushing route: {}", route); self.history.borrow_mut().push(route); } pub fn register_total_route(&self, route: String, scope: ScopeId, fallback: bool) { let clean = clean_route(route); - // log::trace!("Registered route '{}' with scope id {:?}", clean, scope); self.slots.borrow_mut().push((scope, clean)); } pub fn should_render(&self, scope: ScopeId) -> bool { - // log::trace!("Should render scope id {:?}?", scope); if let Some(root_id) = self.root_found.get() { - // log::trace!(" we already found a root with scope id {:?}", root_id); if root_id == scope { - // log::trace!(" yes - it's a match"); return true; } - // log::trace!(" no - it's not a match"); return false; } let location = self.history.borrow().location(); let path = location.path(); - // log::trace!(" current path is '{}'", path); let roots = self.slots.borrow(); @@ -124,23 +115,15 @@ impl RouterService { // fallback logic match root { Some((id, route)) => { - // log::trace!( - // " matched given scope id {:?} with route root '{}'", - // scope, - // route, - // ); if let Some(params) = route_matches_path(route, path) { - // log::trace!(" and it matches the current path '{}'", path); self.root_found.set(Some(*id)); *self.cur_path_params.borrow_mut() = params; true } else { if route == "" { - // log::trace!(" and the route is the root, so we will use that without a better match"); self.root_found.set(Some(*id)); true } else { - // log::trace!(" and the route '{}' is not the root nor does it match the current path", route); false } } @@ -158,12 +141,10 @@ impl RouterService { } pub fn subscribe_onchange(&self, id: ScopeId) { - // log::trace!("Subscribing onchange for scope id {:?}", id); self.onchange_listeners.borrow_mut().insert(id); } pub fn unsubscribe_onchange(&self, id: ScopeId) { - // log::trace!("Subscribing onchange for scope id {:?}", id); self.onchange_listeners.borrow_mut().remove(&id); } } @@ -186,36 +167,20 @@ fn route_matches_path(route: &str, path: &str) -> Option let route_pieces = route.split('/').collect::>(); let path_pieces = clean_path(path).split('/').collect::>(); - // log::trace!( - // " checking route pieces {:?} vs path pieces {:?}", - // route_pieces, - // path_pieces, - // ); - if route_pieces.len() != path_pieces.len() { - // log::trace!(" the routes are different lengths"); return None; } let mut matches = HashMap::new(); for (i, r) in route_pieces.iter().enumerate() { - // log::trace!(" checking route piece '{}' vs path", r); // If this is a parameter then it matches as long as there's // _any_thing in that spot in the path. if r.starts_with(':') { - // log::trace!( - // " route piece '{}' starts with a colon so it matches anything", - // r, - // ); let param = &r[1..]; matches.insert(param.to_string(), path_pieces[i].to_string()); continue; } - // log::trace!( - // " route piece '{}' must be an exact match for path piece '{}'", - // r, - // path_pieces[i], - // ); + if path_pieces[i] != *r { return None; } From c4e6496d9d7064d97359edd7ae267d4bc6b98843 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 31 Jan 2022 12:49:21 -0500 Subject: [PATCH 13/14] chore: enable a pedantic clippy on the diffing algorithm --- packages/core/src/diff.rs | 130 ++++++++++++++++--------------------- packages/core/src/nodes.rs | 7 -- 2 files changed, 56 insertions(+), 81 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index 1a53b01c4f..d40f499344 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -1,4 +1,10 @@ -use crate::innerlude::*; +#![warn(clippy::pedantic)] +#![allow(clippy::cast_possible_truncation)] + +use crate::innerlude::{ + AnyProps, ElementId, Mutations, ScopeArena, ScopeId, VComponent, VElement, VFragment, VNode, + VPlaceholder, VText, +}; use fxhash::{FxHashMap, FxHashSet}; use smallvec::{smallvec, SmallVec}; @@ -33,7 +39,7 @@ impl<'b> DiffState<'b> { } pub fn diff_node(&mut self, old_node: &'b VNode<'b>, new_node: &'b VNode<'b>) { - use VNode::*; + use VNode::{Component, Element, Fragment, Placeholder, Text}; match (old_node, new_node) { // Check the most common cases first // these are *actual* elements, not wrappers around lists @@ -61,7 +67,7 @@ impl<'b> DiffState<'b> { if let Some(root) = old.id.get() { self.scopes.update_node(new_node, root); - new.id.set(Some(root)) + new.id.set(Some(root)); } } @@ -70,7 +76,7 @@ impl<'b> DiffState<'b> { log::trace!("skipping node diff - element are the sames"); return; } - self.diff_element_nodes(old, new, old_node, new_node) + self.diff_element_nodes(old, new, old_node, new_node); } // These two sets are pointers to nodes but are not actually nodes themselves @@ -79,7 +85,7 @@ impl<'b> DiffState<'b> { log::trace!("skipping node diff - placeholder are the sames"); return; } - self.diff_component_nodes(old_node, new_node, *old, *new) + self.diff_component_nodes(old_node, new_node, *old, *new); } (Fragment(old), Fragment(new)) => { @@ -87,7 +93,7 @@ impl<'b> DiffState<'b> { log::trace!("skipping node diff - fragment are the sames"); return; } - self.diff_fragment_nodes(old, new) + self.diff_fragment_nodes(old, new); } // Anything else is just a basic replace and create @@ -136,17 +142,15 @@ impl<'b> DiffState<'b> { .. } = element; - // set the parent ID for event bubbling - // self.stack.instructions.push(DiffInstruction::PopElement); - let parent = self.element_stack.last().unwrap(); parent_id.set(Some(*parent)); // set the id of the element let real_id = self.scopes.reserve_node(node); - self.element_stack.push(real_id); dom_id.set(Some(real_id)); + self.element_stack.push(real_id); + self.mutations.create_element(tag_name, *namespace, real_id); if let Some(cur_scope_id) = self.current_scope() { @@ -155,7 +159,7 @@ impl<'b> DiffState<'b> { self.mutations.new_event_listener(listener, cur_scope_id); } } else { - log::warn!("create element called with no scope on the stack - this is an error for a live dom"); + log::trace!("create element called with no scope on the stack - this is an error for a live dom"); } for attr in *attributes { @@ -198,7 +202,7 @@ impl<'b> DiffState<'b> { new_idx }; - log::info!( + log::trace!( "created component \"{}\", id: {:?} parent {:?} orig: {:?}", vcomponent.fn_name, new_idx, @@ -209,14 +213,11 @@ impl<'b> DiffState<'b> { // Actually initialize the caller's slot with the right address vcomponent.scope.set(Some(new_idx)); - match vcomponent.can_memoize { - true => { - // todo: implement promotion logic. save us from boxing props that we don't need - } - false => { - // track this component internally so we know the right drop order - } - } + // if vcomponent.can_memoize { + // // todo: implement promotion logic. save us from boxing props that we don't need + // } else { + // // track this component internally so we know the right drop order + // } self.enter_scope(new_idx); @@ -278,7 +279,7 @@ impl<'b> DiffState<'b> { self.mutations.remove_attribute(attribute, root.as_u64()); } for attribute in new.attributes { - self.mutations.set_attribute(attribute, root.as_u64()) + self.mutations.set_attribute(attribute, root.as_u64()); } } @@ -318,9 +319,7 @@ impl<'b> DiffState<'b> { let created = self.create_children(new.children); self.mutations.append_children(created as u32); } - (_, _) => { - self.diff_children(old.children, new.children); - } + (_, _) => self.diff_children(old.children, new.children), }; } @@ -332,14 +331,8 @@ impl<'b> DiffState<'b> { new: &'b VComponent<'b>, ) { let scope_addr = old.scope.get().unwrap(); - log::trace!( - "diff_component_nodes. old: {:#?} new: {:#?}", - old_node, - new_node - ); if std::ptr::eq(old, new) { - log::trace!("skipping component diff - component is the sames"); return; } @@ -397,7 +390,6 @@ impl<'b> DiffState<'b> { self.leave_scope(); } else { - log::debug!("scope stack is {:#?}", self.scope_stack); self.replace_node(old_node, new_node); } } @@ -405,14 +397,12 @@ impl<'b> DiffState<'b> { fn diff_fragment_nodes(&mut self, old: &'b VFragment<'b>, new: &'b VFragment<'b>) { // This is the case where options or direct vnodes might be used. // In this case, it's faster to just skip ahead to their diff - // if old.children.len() == 1 && new.children.len() == 1 { - // if std::ptr::eq(old, new) { - // log::debug!("skipping fragment diff - fragment is the same"); - // return; - // } - // self.diff_node(&old.children[0], &new.children[0]); - // return; - // } + if old.children.len() == 1 && new.children.len() == 1 { + if !std::ptr::eq(old, new) { + self.diff_node(&old.children[0], &new.children[0]); + } + return; + } debug_assert!(!old.children.is_empty()); debug_assert!(!new.children.is_empty()); @@ -441,7 +431,6 @@ impl<'b> DiffState<'b> { // to an element, and appending makes sense. fn diff_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { if std::ptr::eq(old, new) { - log::debug!("skipping fragment diff - fragment is the same {:?}", old); return; } @@ -481,24 +470,19 @@ impl<'b> DiffState<'b> { // // the change list stack is in the same state when this function returns. fn diff_non_keyed_children(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { + use std::cmp::Ordering; + // Handled these cases in `diff_children` before calling this function. debug_assert!(!new.is_empty()); debug_assert!(!old.is_empty()); - use std::cmp::Ordering; match old.len().cmp(&new.len()) { Ordering::Greater => self.remove_nodes(&old[new.len()..], true), Ordering::Less => self.create_and_insert_after(&new[old.len()..], old.last().unwrap()), Ordering::Equal => {} } - // panic!( - // "diff_children: new_is_keyed: {:#?}, old_is_keyed: {:#?}. stack: {:#?}, oldptr: {:#?}, newptr: {:#?}", - // old, new, self.scope_stack, old as *const _, new as *const _ - // ); - for (new, old) in new.iter().zip(old.iter()) { - // log::debug!("diffing nonkeyed {:#?} {:#?}", old, new); self.diff_node(old, new); } } @@ -650,6 +634,7 @@ impl<'b> DiffState<'b> { // This function will load the appropriate nodes onto the stack and do diffing in place. // // Upon exit from this function, it will be restored to that same self. + #[allow(clippy::too_many_lines)] fn diff_keyed_middle(&mut self, old: &'b [VNode<'b>], new: &'b [VNode<'b>]) { /* 1. Map the old keys into a numerical ordering based on indices. @@ -675,8 +660,8 @@ impl<'b> DiffState<'b> { // 0. Debug sanity checks // Should have already diffed the shared-key prefixes and suffixes. - debug_assert_ne!(new.first().map(|n| n.key()), old.first().map(|o| o.key())); - debug_assert_ne!(new.last().map(|n| n.key()), old.last().map(|o| o.key())); + debug_assert_ne!(new.first().map(VNode::key), old.first().map(VNode::key)); + debug_assert_ne!(new.last().map(VNode::key), old.last().map(VNode::key)); // 1. Map the old keys into a numerical ordering based on indices. // 2. Create a map of old key to its index @@ -741,7 +726,7 @@ impl<'b> DiffState<'b> { lis_sequence.pop(); } - for idx in lis_sequence.iter() { + for idx in &lis_sequence { self.diff_node(&old[new_index_to_old_index[*idx]], &new[*idx]); } @@ -775,7 +760,6 @@ impl<'b> DiffState<'b> { if last - next > 1 { for (idx, new_node) in new[(next + 1)..last].iter().enumerate() { let new_idx = idx + next + 1; - let old_index = new_index_to_old_index[new_idx]; if old_index == u32::MAX as usize { nodes_created += self.create_node(new_node); @@ -816,7 +800,6 @@ impl<'b> DiffState<'b> { } fn replace_node(&mut self, old: &'b VNode<'b>, new: &'b VNode<'b>) { - log::debug!("Replacing node\n old: {:?}\n new: {:?}", old, new); let nodes_created = self.create_node(new); self.replace_inner(old, nodes_created); } @@ -848,7 +831,7 @@ impl<'b> DiffState<'b> { } VNode::Component(c) => { - log::info!("Replacing component {:?}", old); + log::trace!("Replacing component {:?}", old); let scope_id = c.scope.get().unwrap(); let node = self.scopes.fin_head(scope_id); @@ -856,11 +839,11 @@ impl<'b> DiffState<'b> { self.replace_inner(node, nodes_created); - log::info!("Replacing component x2 {:?}", old); + log::trace!("Replacing component x2 {:?}", old); // we can only remove components if they are actively being diffed if self.scope_stack.contains(&c.originator) { - log::debug!("Removing component {:?}", old); + log::trace!("Removing component {:?}", old); self.scopes.try_remove(scope_id).unwrap(); } @@ -913,7 +896,7 @@ impl<'b> DiffState<'b> { let root = self.scopes.root_node(scope_id); self.remove_nodes([root], gen_muts); - log::info!( + log::trace!( "trying to remove scope {:?}, stack is {:#?}, originator is {:?}", scope_id, self.scope_stack, @@ -922,7 +905,7 @@ impl<'b> DiffState<'b> { // we can only remove this node if the originator is actively if self.scope_stack.contains(&c.originator) { - log::debug!("I am allowed to remove component because scope stack contains originator. Scope stack: {:#?} {:#?} {:#?}", self.scope_stack, c.originator, c.scope); + log::trace!("I am allowed to remove component because scope stack contains originator. Scope stack: {:#?} {:#?} {:#?}", self.scope_stack, c.originator, c.scope); self.scopes.try_remove(scope_id).unwrap(); } self.leave_scope(); @@ -970,15 +953,12 @@ impl<'b> DiffState<'b> { fn find_last_element(&self, vnode: &'b VNode<'b>) -> Option { let mut search_node = Some(vnode); - loop { match &search_node.take().unwrap() { VNode::Text(t) => break t.id.get(), VNode::Element(t) => break t.id.get(), VNode::Placeholder(t) => break t.id.get(), - VNode::Fragment(frag) => { - search_node = frag.children.last(); - } + VNode::Fragment(frag) => search_node = frag.children.last(), VNode::Component(el) => { let scope_id = el.scope.get().unwrap(); search_node = Some(self.scopes.root_node(scope_id)); @@ -989,20 +969,16 @@ impl<'b> DiffState<'b> { fn find_first_element(&self, vnode: &'b VNode<'b>) -> Option { let mut search_node = Some(vnode); - loop { - match &search_node.take().unwrap() { - // the ones that have a direct id - VNode::Fragment(frag) => { - search_node = Some(&frag.children[0]); - } - VNode::Component(el) => { - let scope_id = el.scope.get().unwrap(); - search_node = Some(self.scopes.root_node(scope_id)); - } + match &search_node.take().expect("search node to have an ID") { VNode::Text(t) => break t.id.get(), VNode::Element(t) => break t.id.get(), VNode::Placeholder(t) => break t.id.get(), + VNode::Fragment(frag) => search_node = Some(&frag.children[0]), + VNode::Component(el) => { + let scope = el.scope.get().expect("element to have a scope assigned"); + search_node = Some(self.scopes.root_node(scope)); + } } } } @@ -1015,20 +991,26 @@ impl<'b> DiffState<'b> { 1 } - VNode::Fragment(_) | VNode::Component(_) => { - // + VNode::Fragment(frag) => { let mut added = 0; - for child in node.children() { + for child in frag.children { added += self.push_all_nodes(child); } added } + VNode::Component(c) => { + let scope_id = c.scope.get().unwrap(); + let root = self.scopes.root_node(scope_id); + self.push_all_nodes(root) + } + VNode::Element(el) => { let mut num_on_stack = 0; for child in el.children.iter() { num_on_stack += self.push_all_nodes(child); } + self.mutations.push_root(el.id.get().unwrap()); num_on_stack + 1 diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index b849819f0d..7a36f58cef 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -147,13 +147,6 @@ impl<'src> VNode<'src> { } } - pub(crate) fn children(&self) -> &[VNode<'src>] { - match &self { - VNode::Fragment(f) => f.children, - _ => &[], - } - } - // Create an "owned" version of the vnode. pub fn decouple(&self) -> VNode<'src> { match *self { From 94c1da82644c6f4dfa824c75ba8c0b30c4bf3f9f Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Tue, 1 Feb 2022 15:00:36 -0500 Subject: [PATCH 14/14] chore: clean up documentation in diffing algorithm --- packages/core/src/diff.rs | 363 +++++++++++++++++++++++--------------- 1 file changed, 225 insertions(+), 138 deletions(-) diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index d40f499344..3f3629c106 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -1,6 +1,96 @@ #![warn(clippy::pedantic)] #![allow(clippy::cast_possible_truncation)] +//! This module contains the stateful [`DiffState`] and all methods to diff [`VNodes`], their properties, and their children. +//! +//! The [`DiffState`] calculates the diffs between the old and new frames, updates the new nodes, and generates a set +//! of mutations for the [`RealDom`] to apply. +//! +//! ## Notice: +//! +//! The inspiration and code for this module was originally taken from Dodrio (@fitzgen) and then modified to support +//! Components, Fragments, Suspense, [`SubTree`] memoization, incremental diffing, cancellation, [`NodeRefs`], pausing, priority +//! scheduling, and additional batching operations. +//! +//! ## Implementation Details: +//! +//! ### IDs for elements +//! -------------------- +//! All nodes are addressed by their IDs. The [`RealDom`] provides an imperative interface for making changes to these nodes. +//! We don't necessarily require that DOM changes happen instantly during the diffing process, so the implementor may choose +//! to batch nodes if it is more performant for their application. The element IDs are indices into the internal element +//! array. The expectation is that implementors will use the ID as an index into a Vec of real nodes, allowing for passive +//! garbage collection as the [`VirtualDOM`] replaces old nodes. +//! +//! When new vnodes are created through `cx.render`, they won't know which real node they correspond to. During diffing, +//! we always make sure to copy over the ID. If we don't do this properly, the [`ElementId`] will be populated incorrectly +//! and brick the user's page. +//! +//! ### Fragment Support +//! -------------------- +//! Fragments (nodes without a parent) are supported through a combination of "replace with" and anchor vnodes. Fragments +//! can be particularly challenging when they are empty, so the anchor node lets us "reserve" a spot for the empty +//! fragment to be replaced with when it is no longer empty. This is guaranteed by logic in the [`NodeFactory`] - it is +//! impossible to craft a fragment with 0 elements - they must always have at least a single placeholder element. Adding +//! "dummy" nodes _is_ inefficient, but it makes our diffing algorithm faster and the implementation is completely up to +//! the platform. +//! +//! Other implementations either don't support fragments or use a "child + sibling" pattern to represent them. Our code is +//! vastly simpler and more performant when we can just create a placeholder element while the fragment has no children. +//! +//! ### Suspense +//! ------------ +//! Dioxus implements Suspense slightly differently than React. In React, each fiber is manually progressed until it runs +//! into a promise-like value. React will then work on the next "ready" fiber, checking back on the previous fiber once +//! it has finished its new work. In Dioxus, we use a similar approach, but try to completely render the tree before +//! switching sub-fibers. Instead, each future is submitted into a futures-queue and the node is manually loaded later on. +//! Due to the frequent calls to [`yield_now`] we can get the pure "fetch-as-you-render" behavior of React Fiber. +//! +//! We're able to use this approach because we use placeholder nodes - futures that aren't ready still get submitted to +//! DOM, but as a placeholder. +//! +//! Right now, the "suspense" queue is intertwined with hooks. In the future, we should allow any future to drive attributes +//! and contents, without the need for the [`use_suspense`] hook. In the interim, this is the quickest way to get Suspense working. +//! +//! ## Subtree Memoization +//! ----------------------- +//! We also employ "subtree memoization" which saves us from having to check trees which hold no dynamic content. We can +//! detect if a subtree is "static" by checking if its children are "static". Since we dive into the tree depth-first, the +//! calls to "create" propagate this information upwards. Structures like the one below are entirely static: +//! ```rust, ignore +//! rsx!( div { class: "hello world", "this node is entirely static" } ) +//! ``` +//! Because the subtrees won't be diffed, their "real node" data will be stale (invalid), so it's up to the reconciler to +//! track nodes created in a scope and clean up all relevant data. Support for this is currently WIP and depends on comp-time +//! hashing of the subtree from the rsx! macro. We do a very limited form of static analysis via static string pointers as +//! a way of short-circuiting the most expensive checks. +//! +//! ## Bloom Filter and Heuristics +//! ------------------------------ +//! For all components, we employ some basic heuristics to speed up allocations and pre-size bump arenas. The heuristics are +//! currently very rough, but will get better as time goes on. The information currently tracked includes the size of a +//! bump arena after first render, the number of hooks, and the number of nodes in the tree. +//! +//! ## Garbage Collection +//! --------------------- +//! Dioxus uses a passive garbage collection system to clean up old nodes once the work has been completed. This garbage +//! collection is done internally once the main diffing work is complete. After the "garbage" is collected, Dioxus will then +//! start to re-use old keys for new nodes. This results in a passive memory management system that is very efficient. +//! +//! The IDs used by the key/map are just an index into a Vec. This means that Dioxus will drive the key allocation strategy +//! so the client only needs to maintain a simple list of nodes. By default, Dioxus will not manually clean up old nodes +//! for the client. As new nodes are created, old nodes will be over-written. +//! +//! ## Further Reading and Thoughts +//! ---------------------------- +//! There are more ways of increasing diff performance here that are currently not implemented. +//! - Strong memoization of subtrees. +//! - Guided diffing. +//! - Certain web-dom-specific optimizations. +//! +//! More info on how to improve this diffing algorithm: +//! - + use crate::innerlude::{ AnyProps, ElementId, Mutations, ScopeArena, ScopeId, VComponent, VElement, VFragment, VNode, VPlaceholder, VText, @@ -45,35 +135,38 @@ impl<'b> DiffState<'b> { // these are *actual* elements, not wrappers around lists (Text(old), Text(new)) => { if std::ptr::eq(old, new) { - log::trace!("skipping node diff - text are the sames"); return; } - if let Some(root) = old.id.get() { - if old.text != new.text { - self.mutations.set_text(new.text, root.as_u64()); - } - self.scopes.update_node(new_node, root); + let root = old + .id + .get() + .expect("existing text nodes should have an ElementId"); - new.id.set(Some(root)); + if old.text != new.text { + self.mutations.set_text(new.text, root.as_u64()); } + self.scopes.update_node(new_node, root); + + new.id.set(Some(root)); } (Placeholder(old), Placeholder(new)) => { if std::ptr::eq(old, new) { - log::trace!("skipping node diff - placeholder are the sames"); return; } - if let Some(root) = old.id.get() { - self.scopes.update_node(new_node, root); - new.id.set(Some(root)); - } + let root = old + .id + .get() + .expect("existing placeholder nodes should have an ElementId"); + + self.scopes.update_node(new_node, root); + new.id.set(Some(root)); } (Element(old), Element(new)) => { if std::ptr::eq(old, new) { - log::trace!("skipping node diff - element are the sames"); return; } self.diff_element_nodes(old, new, old_node, new_node); @@ -82,7 +175,6 @@ impl<'b> DiffState<'b> { // These two sets are pointers to nodes but are not actually nodes themselves (Component(old), Component(new)) => { if std::ptr::eq(old, new) { - log::trace!("skipping node diff - placeholder are the sames"); return; } self.diff_component_nodes(old_node, new_node, *old, *new); @@ -90,7 +182,6 @@ impl<'b> DiffState<'b> { (Fragment(old), Fragment(new)) => { if std::ptr::eq(old, new) { - log::trace!("skipping node diff - fragment are the sames"); return; } self.diff_fragment_nodes(old, new); @@ -114,19 +205,17 @@ impl<'b> DiffState<'b> { } } - fn create_text_node(&mut self, vtext: &'b VText<'b>, node: &'b VNode<'b>) -> usize { + fn create_text_node(&mut self, text: &'b VText<'b>, node: &'b VNode<'b>) -> usize { let real_id = self.scopes.reserve_node(node); - self.mutations.create_text_node(vtext.text, real_id); - vtext.id.set(Some(real_id)); - + text.id.set(Some(real_id)); + self.mutations.create_text_node(text.text, real_id); 1 } fn create_anchor_node(&mut self, anchor: &'b VPlaceholder, node: &'b VNode<'b>) -> usize { let real_id = self.scopes.reserve_node(node); - self.mutations.create_placeholder(real_id); anchor.id.set(Some(real_id)); - + self.mutations.create_placeholder(real_id); 1 } @@ -140,36 +229,35 @@ impl<'b> DiffState<'b> { id: dom_id, parent: parent_id, .. - } = element; + } = &element; - let parent = self.element_stack.last().unwrap(); - parent_id.set(Some(*parent)); + parent_id.set(self.element_stack.last().copied()); - // set the id of the element let real_id = self.scopes.reserve_node(node); + dom_id.set(Some(real_id)); self.element_stack.push(real_id); + { + self.mutations.create_element(tag_name, *namespace, real_id); - self.mutations.create_element(tag_name, *namespace, real_id); + let cur_scope_id = self + .current_scope() + .expect("diffing should always have a scope"); - if let Some(cur_scope_id) = self.current_scope() { - for listener in *listeners { + for listener in listeners.iter() { listener.mounted_node.set(Some(real_id)); self.mutations.new_event_listener(listener, cur_scope_id); } - } else { - log::trace!("create element called with no scope on the stack - this is an error for a live dom"); - } - for attr in *attributes { - self.mutations.set_attribute(attr, real_id.as_u64()); - } + for attr in attributes.iter() { + self.mutations.set_attribute(attr, real_id.as_u64()); + } - if !children.is_empty() { - self.create_and_append_children(children); + if !children.is_empty() { + self.create_and_append_children(children); + } } - self.element_stack.pop(); 1 @@ -182,25 +270,29 @@ impl<'b> DiffState<'b> { fn create_component_node(&mut self, vcomponent: &'b VComponent<'b>) -> usize { let parent_idx = self.current_scope().unwrap(); - // the component might already exist - if it does, we need to reuse it - // this makes figure out when to drop the component more complicated - let new_idx = if let Some(idx) = vcomponent.scope.get() { - assert!(self.scopes.get_scope(idx).is_some()); - idx - } else { - // Insert a new scope into our component list - let props: Box = vcomponent.props.borrow_mut().take().unwrap(); - let props: Box = unsafe { std::mem::transmute(props) }; - let new_idx = self.scopes.new_with_key( - vcomponent.user_fc, - props, - Some(parent_idx), - self.element_stack.last().copied().unwrap(), - 0, - ); + // ensure this scope doesn't already exist if we're trying to create it + debug_assert!( + vcomponent + .scope + .get() + .and_then(|f| self.scopes.get_scope(f)) + .is_none(), + "component scope already exists" + ); - new_idx - }; + // Insert a new scope into our component list + let props: Box = vcomponent.props.borrow_mut().take().unwrap(); + let props: Box = unsafe { std::mem::transmute(props) }; + let new_idx = self.scopes.new_with_key( + vcomponent.user_fc, + props, + Some(parent_idx), + self.element_stack.last().copied().unwrap(), + 0, + ); + + // Actually initialize the caller's slot with the right address + vcomponent.scope.set(Some(new_idx)); log::trace!( "created component \"{}\", id: {:?} parent {:?} orig: {:?}", @@ -210,9 +302,6 @@ impl<'b> DiffState<'b> { vcomponent.originator ); - // Actually initialize the caller's slot with the right address - vcomponent.scope.set(Some(new_idx)); - // if vcomponent.can_memoize { // // todo: implement promotion logic. save us from boxing props that we don't need // } else { @@ -221,13 +310,15 @@ impl<'b> DiffState<'b> { self.enter_scope(new_idx); - // Run the scope for one iteration to initialize it - self.scopes.run_scope(new_idx); - self.mutations.mark_dirty_scope(new_idx); + let created = { + // Run the scope for one iteration to initialize it + self.scopes.run_scope(new_idx); + self.mutations.mark_dirty_scope(new_idx); - // Take the node that was just generated from running the component - let nextnode = self.scopes.fin_head(new_idx); - let created = self.create_node(nextnode); + // Take the node that was just generated from running the component + let nextnode = self.scopes.fin_head(new_idx); + self.create_node(nextnode) + }; self.leave_scope(); @@ -241,7 +332,10 @@ impl<'b> DiffState<'b> { old_node: &'b VNode<'b>, new_node: &'b VNode<'b>, ) { - let root = old.id.get().unwrap(); + let root = old + .id + .get() + .expect("existing element nodes should have an ElementId"); // If the element type is completely different, the element needs to be re-rendered completely // This is an optimization React makes due to how users structure their code @@ -330,7 +424,10 @@ impl<'b> DiffState<'b> { old: &'b VComponent<'b>, new: &'b VComponent<'b>, ) { - let scope_addr = old.scope.get().unwrap(); + let scope_addr = old + .scope + .get() + .expect("existing component nodes should have a scope"); if std::ptr::eq(old, new) { return; @@ -339,55 +436,55 @@ impl<'b> DiffState<'b> { // Make sure we're dealing with the same component (by function pointer) if old.user_fc == new.user_fc { self.enter_scope(scope_addr); - - // Make sure the new component vnode is referencing the right scope id - new.scope.set(Some(scope_addr)); - - // make sure the component's caller function is up to date - let scope = self - .scopes - .get_scope(scope_addr) - .unwrap_or_else(|| panic!("could not find {:?}", scope_addr)); - - // take the new props out regardless - // when memoizing, push to the existing scope if memoization happens - let new_props = new.props.borrow_mut().take().unwrap(); - - let should_run = { - if old.can_memoize { - let props_are_the_same = unsafe { - scope - .props - .borrow() - .as_ref() - .unwrap() - .memoize(new_props.as_ref()) - }; - !props_are_the_same || self.force_diff - } else { - true - } - }; - - if should_run { - let _old_props = scope + { + // Make sure the new component vnode is referencing the right scope id + new.scope.set(Some(scope_addr)); + + // make sure the component's caller function is up to date + let scope = self + .scopes + .get_scope(scope_addr) + .unwrap_or_else(|| panic!("could not find {:?}", scope_addr)); + + // take the new props out regardless + // when memoizing, push to the existing scope if memoization happens + let new_props = new .props - .replace(unsafe { std::mem::transmute(Some(new_props)) }); + .borrow_mut() + .take() + .expect("new component props should exist"); + + let should_diff = { + if old.can_memoize { + // safety: we trust the implementation of "memoize" + let props_are_the_same = unsafe { + let new_ref = new_props.as_ref(); + scope.props.borrow().as_ref().unwrap().memoize(new_ref) + }; + !props_are_the_same || self.force_diff + } else { + true + } + }; - // this should auto drop the previous props - self.scopes.run_scope(scope_addr); - self.mutations.mark_dirty_scope(scope_addr); + if should_diff { + let _old_props = scope + .props + .replace(unsafe { std::mem::transmute(Some(new_props)) }); - self.diff_node( - self.scopes.wip_head(scope_addr), - self.scopes.fin_head(scope_addr), - ); - } else { - log::trace!("memoized"); - // memoization has taken place - drop(new_props); - }; + // this should auto drop the previous props + self.scopes.run_scope(scope_addr); + self.mutations.mark_dirty_scope(scope_addr); + self.diff_node( + self.scopes.wip_head(scope_addr), + self.scopes.fin_head(scope_addr), + ); + } else { + // memoization has taken place + drop(new_props); + }; + } self.leave_scope(); } else { self.replace_node(old_node, new_node); @@ -410,10 +507,6 @@ impl<'b> DiffState<'b> { self.diff_children(old.children, new.children); } - // ============================================= - // Utilities for creating new diff instructions - // ============================================= - // Diff the given set of old and new children. // // The parent must be on top of the change list stack when this function is @@ -836,16 +929,17 @@ impl<'b> DiffState<'b> { let node = self.scopes.fin_head(scope_id); self.enter_scope(scope_id); + { + self.replace_inner(node, nodes_created); - self.replace_inner(node, nodes_created); + log::trace!("Replacing component x2 {:?}", old); - log::trace!("Replacing component x2 {:?}", old); - - // we can only remove components if they are actively being diffed - if self.scope_stack.contains(&c.originator) { - log::trace!("Removing component {:?}", old); + // we can only remove components if they are actively being diffed + if self.scope_stack.contains(&c.originator) { + log::trace!("Removing component {:?}", old); - self.scopes.try_remove(scope_id).unwrap(); + self.scopes.try_remove(scope_id).unwrap(); + } } self.leave_scope(); } @@ -891,22 +985,15 @@ impl<'b> DiffState<'b> { VNode::Component(c) => { self.enter_scope(c.scope.get().unwrap()); - - let scope_id = c.scope.get().unwrap(); - let root = self.scopes.root_node(scope_id); - self.remove_nodes([root], gen_muts); - - log::trace!( - "trying to remove scope {:?}, stack is {:#?}, originator is {:?}", - scope_id, - self.scope_stack, - c.originator - ); - - // we can only remove this node if the originator is actively - if self.scope_stack.contains(&c.originator) { - log::trace!("I am allowed to remove component because scope stack contains originator. Scope stack: {:#?} {:#?} {:#?}", self.scope_stack, c.originator, c.scope); - self.scopes.try_remove(scope_id).unwrap(); + { + let scope_id = c.scope.get().unwrap(); + let root = self.scopes.root_node(scope_id); + self.remove_nodes([root], gen_muts); + + // we can only remove this node if the originator is actively in our stackß + if self.scope_stack.contains(&c.originator) { + self.scopes.try_remove(scope_id).unwrap(); + } } self.leave_scope(); }