From 2871c8bb4dbaaa29e58d6348fda734d9d60f4d71 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Wed, 19 Jul 2023 12:08:13 -0700 Subject: [PATCH 1/2] Don't use boxed cell slice, use a refcell instead --- packages/core/src/create.rs | 5 +- packages/core/src/diff.rs | 18 ++-- packages/core/src/nodes.rs | 105 +------------------- packages/core/src/scope_arena.rs | 8 +- packages/core/src/virtual_dom.rs | 3 + packages/dioxus-tui/examples/colorpicker.rs | 2 +- packages/web/src/rehydrate.rs | 2 +- 7 files changed, 24 insertions(+), 119 deletions(-) diff --git a/packages/core/src/create.rs b/packages/core/src/create.rs index 8efdf13e4..6159c882b 100644 --- a/packages/core/src/create.rs +++ b/packages/core/src/create.rs @@ -88,8 +88,7 @@ impl<'b> VirtualDom { } // Intialize the root nodes slice - node.root_ids - .intialize(vec![ElementId(0); node.template.get().roots.len()].into_boxed_slice()); + *node.root_ids.borrow_mut() = vec![ElementId(0); node.template.get().roots.len()]; // The best renderers will have templates prehydrated and registered // Just in case, let's create the template using instructions anyways @@ -328,7 +327,7 @@ impl<'b> VirtualDom { fn load_template_root(&mut self, template: &VNode, root_idx: usize) -> ElementId { // Get an ID for this root since it's a real root let this_id = self.next_root(template, root_idx); - template.root_ids.set(root_idx, this_id); + template.root_ids.borrow_mut()[root_idx] = this_id; self.mutations.push(LoadTemplate { name: template.template.get().name, diff --git a/packages/core/src/diff.rs b/packages/core/src/diff.rs index cfda18505..74988210a 100644 --- a/packages/core/src/diff.rs +++ b/packages/core/src/diff.rs @@ -129,12 +129,14 @@ impl<'b> VirtualDom { }); // Make sure the roots get transferred over while we're here - right_template.root_ids.transfer(&left_template.root_ids); + *right_template.root_ids.borrow_mut() = left_template.root_ids.borrow().clone(); + + let root_ids = right_template.root_ids.borrow(); // Update the node refs - for i in 0..right_template.root_ids.len() { - if let Some(root_id) = right_template.root_ids.get(i) { - self.update_template(root_id, right_template); + for i in 0..root_ids.len() { + if let Some(root_id) = root_ids.get(i) { + self.update_template(*root_id, right_template); } } } @@ -686,7 +688,7 @@ impl<'b> VirtualDom { Some(node) => node, None => { self.mutations.push(Mutation::PushRoot { - id: node.root_ids.get(idx).unwrap(), + id: node.root_ids.borrow()[idx], }); return 1; } @@ -821,7 +823,7 @@ impl<'b> VirtualDom { if let Some(dy) = node.dynamic_root(idx) { self.remove_dynamic_node(dy, gen_muts); } else { - let id = node.root_ids.get(idx).unwrap(); + let id = node.root_ids.borrow()[idx]; if gen_muts { self.mutations.push(Mutation::Remove { id }); } @@ -928,7 +930,7 @@ impl<'b> VirtualDom { fn find_first_element(&self, node: &'b VNode<'b>) -> ElementId { match node.dynamic_root(0) { - None => node.root_ids.get(0).unwrap(), + None => node.root_ids.borrow()[0], Some(Text(t)) => t.id.get().unwrap(), Some(Fragment(t)) => self.find_first_element(&t[0]), Some(Placeholder(t)) => t.id.get().unwrap(), @@ -944,7 +946,7 @@ impl<'b> VirtualDom { fn find_last_element(&self, node: &'b VNode<'b>) -> ElementId { match node.dynamic_root(node.template.get().roots.len() - 1) { - None => node.root_ids.last().unwrap(), + None => *node.root_ids.borrow().last().unwrap(), Some(Text(t)) => t.id.get().unwrap(), Some(Fragment(t)) => self.find_last_element(t.last().unwrap()), Some(Placeholder(t)) => t.id.get().unwrap(), diff --git a/packages/core/src/nodes.rs b/packages/core/src/nodes.rs index 209c37c77..197550a3b 100644 --- a/packages/core/src/nodes.rs +++ b/packages/core/src/nodes.rs @@ -5,7 +5,7 @@ use bumpalo::boxed::Box as BumpBox; use bumpalo::Bump; use std::{ any::{Any, TypeId}, - cell::{Cell, RefCell, UnsafeCell}, + cell::{Cell, RefCell}, fmt::{Arguments, Debug}, }; @@ -54,7 +54,7 @@ pub struct VNode<'a> { /// The IDs for the roots of this template - to be used when moving the template around and removing it from /// the actual Dom - pub root_ids: BoxedCellSlice, + pub root_ids: RefCell>, /// The dynamic parts of the template pub dynamic_nodes: &'a [DynamicNode<'a>], @@ -63,112 +63,13 @@ pub struct VNode<'a> { pub dynamic_attrs: &'a [Attribute<'a>], } -// Saftey: There is no way to get references to the internal data of this struct so no refrences will be invalidated by mutating the data with a immutable reference (The same principle behind Cell) -#[derive(Debug, Default)] -pub struct BoxedCellSlice(UnsafeCell>>); - -impl Clone for BoxedCellSlice { - fn clone(&self) -> Self { - Self(UnsafeCell::new(unsafe { (*self.0.get()).clone() })) - } -} - -impl BoxedCellSlice { - pub fn last(&self) -> Option { - unsafe { - (*self.0.get()) - .as_ref() - .and_then(|inner| inner.as_ref().last().copied()) - } - } - - pub fn get(&self, idx: usize) -> Option { - unsafe { - (*self.0.get()) - .as_ref() - .and_then(|inner| inner.as_ref().get(idx).copied()) - } - } - - pub unsafe fn get_unchecked(&self, idx: usize) -> Option { - (*self.0.get()) - .as_ref() - .and_then(|inner| inner.as_ref().get(idx).copied()) - } - - pub fn set(&self, idx: usize, new: ElementId) { - unsafe { - if let Some(inner) = &mut *self.0.get() { - inner[idx] = new; - } - } - } - - pub fn intialize(&self, contents: Box<[ElementId]>) { - unsafe { - *self.0.get() = Some(contents); - } - } - - pub fn transfer(&self, other: &Self) { - unsafe { - *self.0.get() = (*other.0.get()).clone(); - } - } - - pub fn take_from(&self, other: &Self) { - unsafe { - *self.0.get() = (*other.0.get()).take(); - } - } - - pub fn len(&self) -> usize { - unsafe { - (*self.0.get()) - .as_ref() - .map(|inner| inner.len()) - .unwrap_or(0) - } - } -} - -impl<'a> IntoIterator for &'a BoxedCellSlice { - type Item = ElementId; - - type IntoIter = BoxedCellSliceIter<'a>; - - fn into_iter(self) -> Self::IntoIter { - BoxedCellSliceIter { - index: 0, - borrow: self, - } - } -} - -pub struct BoxedCellSliceIter<'a> { - index: usize, - borrow: &'a BoxedCellSlice, -} - -impl Iterator for BoxedCellSliceIter<'_> { - type Item = ElementId; - - fn next(&mut self) -> Option { - let result = self.borrow.get(self.index); - if result.is_some() { - self.index += 1; - } - result - } -} - impl<'a> VNode<'a> { /// Create a template with no nodes that will be skipped over during diffing pub fn empty() -> Element<'a> { Some(VNode { key: None, parent: None, - root_ids: BoxedCellSlice::default(), + root_ids: Default::default(), dynamic_nodes: &[], dynamic_attrs: &[], template: Cell::new(Template { diff --git a/packages/core/src/scope_arena.rs b/packages/core/src/scope_arena.rs index d058e34de..e8134c238 100644 --- a/packages/core/src/scope_arena.rs +++ b/packages/core/src/scope_arena.rs @@ -83,12 +83,12 @@ impl VirtualDom { id: scope.id, }); - if matches!(allocated, RenderReturn::Aborted(_)) { - if scope.suspended.get() { + if scope.suspended.get() { + if matches!(allocated, RenderReturn::Aborted(_)) { self.suspended_scopes.insert(scope.id); - } else if !self.suspended_scopes.is_empty() { - _ = self.suspended_scopes.remove(&scope.id); } + } else if !self.suspended_scopes.is_empty() { + _ = self.suspended_scopes.remove(&scope.id); } // rebind the lifetime now that its stored internally diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 31a1b5912..ee6e9c5d6 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -571,12 +571,15 @@ impl VirtualDom { /// The mutations will be thrown out, so it's best to use this method for things like SSR that have async content pub async fn wait_for_suspense(&mut self) { loop { + // println!("waiting for suspense {:?}", self.suspended_scopes); if self.suspended_scopes.is_empty() { return; } + // println!("waiting for suspense"); self.wait_for_work().await; + // println!("Rendered immediately"); _ = self.render_immediate(); } } diff --git a/packages/dioxus-tui/examples/colorpicker.rs b/packages/dioxus-tui/examples/colorpicker.rs index 1de6b8209..00f8ef7e0 100644 --- a/packages/dioxus-tui/examples/colorpicker.rs +++ b/packages/dioxus-tui/examples/colorpicker.rs @@ -20,7 +20,7 @@ fn app(cx: Scope) -> Element { background_color: "hsl({hue}, 70%, {brightness}%)", onmousemove: move |evt| { if let RenderReturn::Ready(node) = cx.root_node() { - if let Some(id) = node.root_ids.get(0){ + if let Some(id) = node.root_ids.borrow().get(0).cloned() { let node = tui_query.get(mapping.get_node_id(id).unwrap()); let Size{width, height} = node.size().unwrap(); let pos = evt.inner().element_coordinates(); diff --git a/packages/web/src/rehydrate.rs b/packages/web/src/rehydrate.rs index 8af7da16d..5cc9ea57e 100644 --- a/packages/web/src/rehydrate.rs +++ b/packages/web/src/rehydrate.rs @@ -90,7 +90,7 @@ impl WebsysDom { // make sure we set the root node ids even if the node is not dynamic set_node( hydrated, - vnode.root_ids.get(i).ok_or(VNodeNotInitialized)?, + *vnode.root_ids.borrow().get(i).ok_or(VNodeNotInitialized)?, current_child.clone()?, ); From 163fe68f455c93b0a425b3d9fb0bf527e08f2063 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Wed, 19 Jul 2023 12:26:32 -0700 Subject: [PATCH 2/2] Remove a bit more unsafe --- packages/core/tests/task.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/core/tests/task.rs b/packages/core/tests/task.rs index 2704c5eb2..316cea6dd 100644 --- a/packages/core/tests/task.rs +++ b/packages/core/tests/task.rs @@ -1,9 +1,9 @@ //! Verify that tasks get polled by the virtualdom properly, and that we escape wait_for_work safely use dioxus::prelude::*; -use std::time::Duration; +use std::{sync::atomic::AtomicUsize, time::Duration}; -static mut POLL_COUNT: usize = 0; +static POLL_COUNT: AtomicUsize = AtomicUsize::new(0); #[tokio::test] async fn it_works() { @@ -18,7 +18,10 @@ async fn it_works() { // By the time the tasks are finished, we should've accumulated ticks from two tasks // Be warned that by setting the delay to too short, tokio might not schedule in the tasks - assert_eq!(unsafe { POLL_COUNT }, 135); + assert_eq!( + POLL_COUNT.fetch_add(0, std::sync::atomic::Ordering::Relaxed), + 135 + ); } fn app(cx: Scope) -> Element { @@ -26,14 +29,14 @@ fn app(cx: Scope) -> Element { cx.spawn(async { for x in 0..10 { tokio::time::sleep(Duration::from_micros(50)).await; - unsafe { POLL_COUNT += x } + POLL_COUNT.fetch_add(x, std::sync::atomic::Ordering::Relaxed); } }); cx.spawn(async { for x in 0..10 { tokio::time::sleep(Duration::from_micros(25)).await; - unsafe { POLL_COUNT += x * 2 } + POLL_COUNT.fetch_add(x * 2, std::sync::atomic::Ordering::Relaxed); } }); });