From 701093ede5db6d84581f422c98295035bd884692 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Wed, 17 Jan 2024 08:17:30 -0600 Subject: [PATCH] Try to rerun all dirty scopes before polling any tasks to fix effect ordering --- examples/counter.rs | 47 ++++++++++++++++++++++---------- packages/core/src/arena.rs | 16 +++++++---- packages/core/src/virtual_dom.rs | 33 ++++++++++++++++++---- packages/signals/src/impls.rs | 26 +++++++++++++++--- 4 files changed, 92 insertions(+), 30 deletions(-) diff --git a/examples/counter.rs b/examples/counter.rs index 6a3e1a8ae..33c611b48 100644 --- a/examples/counter.rs +++ b/examples/counter.rs @@ -14,22 +14,39 @@ fn app() -> Element { rsx! { div { button { onclick: move |_| counters.write().push(0), "Add counter" } - button { onclick: move |_| { counters.write().pop(); }, "Remove counter" } + button { + onclick: move |_| { + counters.write().pop(); + }, + "Remove counter" + } p { "Total: {sum}" } - for (i, counter) in counters.read().iter().enumerate() { - li { - button { onclick: move |_| counters.write()[i] -= 1, "-1" } - input { - value: "{counter}", - oninput: move |e| { - if let Ok(value) = e.value().parse::() { - counters.write()[i] = value; - } - } - } - button { onclick: move |_| counters.write()[i] += 1, "+1" } - button { onclick: move |_| { counters.write().remove(i); }, "x" } - } + for i in 0..counters.len() { + Child { i, counters } + } + } + } +} + +#[component] +fn Child(i: usize, counters: Signal>) -> Element { + rsx! { + li { + button { onclick: move |_| counters.write()[i] -= 1, "-1" } + input { + value: "{counters.read()[i]}", + oninput: move |e| { + if let Ok(value) = e.value().parse::() { + counters.write()[i] = value; + } + } + } + button { onclick: move |_| counters.write()[i] += 1, "+1" } + button { + onclick: move |_| { + counters.write().remove(i); + }, + "x" } } } diff --git a/packages/core/src/arena.rs b/packages/core/src/arena.rs index 76e100006..ec2412efb 100644 --- a/packages/core/src/arena.rs +++ b/packages/core/src/arena.rs @@ -68,12 +68,18 @@ impl VirtualDom { // // Note: This will not remove any ids from the arena pub(crate) fn drop_scope(&mut self, id: ScopeId) { - self.dirty_scopes.remove(&DirtyScope { - height: self.scopes[id.0].context().height, - id, - }); + let (height, spawned_tasks) = { + let scope = self.scopes.remove(id.0); + let context = scope.context(); + let spawned_tasks = context.spawned_tasks.borrow(); + let spawned_tasks: Vec<_> = spawned_tasks.iter().copied().collect(); + (context.height, spawned_tasks) + }; - self.scopes.remove(id.0); + self.dirty_scopes.remove(&DirtyScope { height, id }); + for task in spawned_tasks { + self.runtime.remove_task(task); + } } } diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index b9e555a9a..b660fd281 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -19,7 +19,12 @@ use crate::{ use futures_util::{pin_mut, StreamExt}; use rustc_hash::{FxHashMap, FxHashSet}; use slab::Slab; -use std::{any::Any, collections::BTreeSet, future::Future, rc::Rc}; +use std::{ + any::Any, + collections::{BTreeSet, VecDeque}, + future::Future, + rc::Rc, +}; /// A virtual node system that progresses user events and diffs UI trees. /// @@ -184,6 +189,7 @@ pub struct VirtualDom { pub(crate) scopes: Slab, pub(crate) dirty_scopes: BTreeSet, + pub(crate) dirty_tasks: VecDeque, // Maps a template path to a map of byte indexes to templates pub(crate) templates: FxHashMap>, @@ -227,7 +233,7 @@ impl VirtualDom { /// /// Note: the VirtualDom is not progressed, you must either "run_with_deadline" or use "rebuild" to progress it. pub fn new(app: fn() -> Element) -> Self { - Self::new_with_props(move || app(), ()) + Self::new_with_props(app, ()) } /// Create a new virtualdom and build it immediately @@ -278,6 +284,7 @@ impl VirtualDom { runtime: Runtime::new(tx), scopes: Default::default(), dirty_scopes: Default::default(), + dirty_tasks: Default::default(), templates: Default::default(), queued_templates: Default::default(), elements: Default::default(), @@ -399,9 +406,8 @@ impl VirtualDom { if let Some(msg) = some_msg.take() { match msg { SchedulerMsg::Immediate(id) => self.mark_dirty(id), - SchedulerMsg::TaskNotified(task) => self.handle_task_wakeup(task), + SchedulerMsg::TaskNotified(task) => self.queue_task_wakeup(task), } - continue; } // If they're not ready, then we should wait for them to be ready @@ -409,8 +415,18 @@ impl VirtualDom { Ok(Some(val)) => some_msg = Some(val), Ok(None) => return, Err(_) => { + // Now that we have collected all queued work, we should check if we have any dirty scopes. If there are not, then we can poll any queued futures + let has_dirty_scopes = !self.dirty_scopes.is_empty(); + + if !has_dirty_scopes { + // If we have no dirty scopes, then we should poll any tasks that have been notified + while let Some(task) = self.dirty_tasks.pop_front() { + self.handle_task_wakeup(task); + } + } + // If we have any dirty scopes, or finished fiber trees then we should exit - if !self.dirty_scopes.is_empty() || !self.suspended_scopes.is_empty() { + if has_dirty_scopes || !self.suspended_scopes.is_empty() { return; } @@ -425,7 +441,7 @@ impl VirtualDom { while let Ok(Some(msg)) = self.rx.try_next() { match msg { SchedulerMsg::Immediate(id) => self.mark_dirty(id), - SchedulerMsg::TaskNotified(task) => self.handle_task_wakeup(task), + SchedulerMsg::TaskNotified(task) => self.queue_task_wakeup(task), } } } @@ -608,6 +624,11 @@ impl VirtualDom { } } + /// Queue a task to be polled after all dirty scopes have been rendered + fn queue_task_wakeup(&mut self, id: Task) { + self.dirty_tasks.push_back(id); + } + /// Handle notifications by tasks inside the scheduler /// /// This is precise, meaning we won't poll every task, just tasks that have woken up as notified to use by the diff --git a/packages/signals/src/impls.rs b/packages/signals/src/impls.rs index 2fdf8aeef..a5773f1d2 100644 --- a/packages/signals/src/impls.rs +++ b/packages/signals/src/impls.rs @@ -10,7 +10,7 @@ use std::{ }; macro_rules! read_impls { - ($ty:ident, $bound:path) => { + ($ty:ident, $bound:path, $vec_bound:path) => { impl Default for $ty { #[track_caller] fn default() -> Self { @@ -47,6 +47,20 @@ macro_rules! read_impls { self.with(|v| *v == *other) } } + + impl $ty, S> { + /// Returns the length of the inner vector. + #[track_caller] + pub fn len(&self) -> usize { + self.with(|v| v.len()) + } + + /// Returns true if the inner vector is empty. + #[track_caller] + pub fn is_empty(&self) -> bool { + self.with(|v| v.is_empty()) + } + } }; } @@ -180,7 +194,7 @@ macro_rules! write_impls { }; } -read_impls!(CopyValue, Storage); +read_impls!(CopyValue, Storage, Storage>); impl>> CopyValue, S> { /// Read a value from the inner vector. @@ -245,7 +259,7 @@ impl>> CopyValue, S> { } } -read_impls!(Signal, Storage>); +read_impls!(Signal, Storage>, Storage>>); impl>>> Signal, S> { /// Read a value from the inner vector. @@ -323,7 +337,11 @@ impl>>> Signal, S> { } } -read_impls!(ReadOnlySignal, Storage>); +read_impls!( + ReadOnlySignal, + Storage>, + Storage>> +); /// An iterator over the values of a `CopyValue>`. pub struct CopyValueIterator>> {