From b6dc2a2230e638ab228315926568ebf4ece7baf7 Mon Sep 17 00:00:00 2001 From: Evan Almloff Date: Wed, 19 Jun 2024 03:49:25 +0200 Subject: [PATCH] Deduplicate reactive scope updates/Reset subscriptions on reruns/fix use memo double update (#2506) * deduplicate effect updates * only subscribe to signals read in the current run of reactive scopes * subscribe to memo reads after recomputing the value --- Cargo.lock | 1 + packages/core-macro/Cargo.toml | 1 + .../tests/values_memoize_in_place.rs | 23 +- packages/{signals => core}/docs/reactivity.md | 0 packages/core/src/generational_box.rs | 10 +- packages/core/src/lib.rs | 6 +- packages/core/src/reactive_context.rs | 298 ++++++++++++++++++ packages/core/src/scope_arena.rs | 11 +- packages/core/src/scope_context.rs | 12 + packages/core/src/scopes.rs | 4 +- packages/hooks/src/use_effect.rs | 17 +- packages/hooks/src/use_resource.rs | 2 +- packages/hooks/tests/memo.rs | 80 ++++- packages/signals/src/lib.rs | 3 - packages/signals/src/memo.rs | 46 +-- packages/signals/src/reactive_context.rs | 181 ----------- packages/signals/src/set_compare.rs | 4 +- packages/signals/src/signal.rs | 10 +- 18 files changed, 459 insertions(+), 250 deletions(-) rename packages/{signals => core}/docs/reactivity.md (100%) create mode 100644 packages/core/src/reactive_context.rs delete mode 100644 packages/signals/src/reactive_context.rs diff --git a/Cargo.lock b/Cargo.lock index f6c6b10f3..1b2b863dc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2323,6 +2323,7 @@ dependencies = [ "quote", "rustversion", "syn 2.0.60", + "tokio", "trybuild", ] diff --git a/packages/core-macro/Cargo.toml b/packages/core-macro/Cargo.toml index 7c341b0b3..481d0aef4 100644 --- a/packages/core-macro/Cargo.toml +++ b/packages/core-macro/Cargo.toml @@ -24,6 +24,7 @@ prettyplease = "0.2.15" [dev-dependencies] dioxus = { workspace = true } rustversion = "1.0" +tokio = { workspace = true, features = ["full", "time"] } trybuild = "1.0" [features] diff --git a/packages/core-macro/tests/values_memoize_in_place.rs b/packages/core-macro/tests/values_memoize_in_place.rs index 6af4b019f..3f7d6f314 100644 --- a/packages/core-macro/tests/values_memoize_in_place.rs +++ b/packages/core-macro/tests/values_memoize_in_place.rs @@ -2,8 +2,8 @@ use dioxus::prelude::*; use dioxus_core::ElementId; use std::rc::Rc; -#[test] -fn values_memoize_in_place() { +#[tokio::test] +async fn values_memoize_in_place() { thread_local! { static DROP_COUNT: std::cell::RefCell = const { std::cell::RefCell::new(0) }; } @@ -20,9 +20,14 @@ fn values_memoize_in_place() { let mut count = use_signal(|| 0); let x = CountsDrop; - if generation() < 15 { - count += 1; - } + use_hook(|| { + spawn(async move { + for _ in 0..15 { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + count += 1; + } + }); + }); rsx! { TakesEventHandler { @@ -33,7 +38,6 @@ fn values_memoize_in_place() { }, children: count() / 2 } - TakesSignal { sig: count(), children: count() / 2 } } } @@ -50,12 +54,16 @@ fn values_memoize_in_place() { ElementId(1), true, ); + tokio::select! { + _ = tokio::time::sleep(std::time::Duration::from_millis(20)) => {}, + _ = dom.wait_for_work() => {} + } dom.render_immediate(&mut dioxus_core::NoOpMutations); } dom.render_immediate(&mut dioxus_core::NoOpMutations); // As we rerun the app, the drop count should be 15 one for each render of the app component let drop_count = DROP_COUNT.with(|c| *c.borrow()); - assert_eq!(drop_count, 15); + assert_eq!(drop_count, 16); } // We move over event handlers in place. Make sure we do that in a way that doesn't destroy the original event handler @@ -98,6 +106,7 @@ fn cloning_event_handler_components_work() { #[component] fn TakesEventHandler(click: EventHandler, children: usize) -> Element { + println!("children is{children}"); let first_render_click = use_hook(move || click); if generation() > 0 { // Make sure the event handler is memoized in place and never gets dropped diff --git a/packages/signals/docs/reactivity.md b/packages/core/docs/reactivity.md similarity index 100% rename from packages/signals/docs/reactivity.md rename to packages/core/docs/reactivity.md diff --git a/packages/core/src/generational_box.rs b/packages/core/src/generational_box.rs index 2d7f67280..5a5c485b2 100644 --- a/packages/core/src/generational_box.rs +++ b/packages/core/src/generational_box.rs @@ -9,7 +9,7 @@ use std::{ use generational_box::{AnyStorage, Owner, SyncStorage, UnsyncStorage}; -use crate::{innerlude::current_scope_id, ScopeId}; +use crate::{innerlude::current_scope_id, Runtime, ScopeId}; /// Run a closure with the given owner. /// @@ -91,12 +91,6 @@ pub fn current_owner() -> Owner { impl ScopeId { /// Get the owner for the current scope. pub fn owner(self) -> Owner { - match self.has_context() { - Some(rt) => rt, - None => { - let owner = S::owner(); - self.provide_context(owner) - } - } + Runtime::with_scope(self, |cx| cx.owner::()).expect("to be in a dioxus runtime") } } diff --git a/packages/core/src/lib.rs b/packages/core/src/lib.rs index cfdf0eb08..861fc8af8 100644 --- a/packages/core/src/lib.rs +++ b/packages/core/src/lib.rs @@ -15,6 +15,7 @@ mod global_context; mod mutations; mod nodes; mod properties; +mod reactive_context; mod render_signal; mod runtime; mod scheduler; @@ -42,6 +43,7 @@ pub(crate) mod innerlude { pub use crate::mutations::*; pub use crate::nodes::*; pub use crate::properties::*; + pub use crate::reactive_context::*; pub use crate::runtime::{Runtime, RuntimeGuard}; pub use crate::scheduler::*; pub use crate::scopes::*; @@ -79,7 +81,7 @@ pub mod prelude { use_hook_with_cleanup, wait_for_next_render, with_owner, AnyValue, Attribute, Callback, Component, ComponentFunction, Element, ErrorBoundary, Event, EventHandler, Fragment, HasAttributes, IntoAttributeValue, IntoDynNode, OptionStringFromMarker, Properties, - Runtime, RuntimeGuard, ScopeId, ScopeState, SuperFrom, SuperInto, Task, Template, - TemplateAttribute, TemplateNode, Throw, VNode, VNodeInner, VirtualDom, + ReactiveContext, Runtime, RuntimeGuard, ScopeId, ScopeState, SuperFrom, SuperInto, Task, + Template, TemplateAttribute, TemplateNode, Throw, VNode, VNodeInner, VirtualDom, }; } diff --git a/packages/core/src/reactive_context.rs b/packages/core/src/reactive_context.rs new file mode 100644 index 000000000..5675f6528 --- /dev/null +++ b/packages/core/src/reactive_context.rs @@ -0,0 +1,298 @@ +use crate::{ + prelude::{current_scope_id, ScopeId}, + scope_context::Scope, + tasks::SchedulerMsg, + Runtime, +}; +use futures_channel::mpsc::UnboundedReceiver; +use generational_box::{GenerationalBox, SyncStorage}; +use std::{ + cell::RefCell, + collections::HashSet, + hash::Hash, + sync::{Arc, Mutex}, +}; + +#[doc = include_str!("../docs/reactivity.md")] +#[derive(Clone, Copy)] +pub struct ReactiveContext { + scope: ScopeId, + inner: GenerationalBox, +} + +impl PartialEq for ReactiveContext { + fn eq(&self, other: &Self) -> bool { + self.inner.ptr_eq(&other.inner) + } +} + +impl Eq for ReactiveContext {} + +thread_local! { + static CURRENT: RefCell> = const { RefCell::new(vec![]) }; +} + +impl std::fmt::Display for ReactiveContext { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + #[cfg(debug_assertions)] + { + if let Ok(read) = self.inner.try_read() { + if let Some(scope) = read.scope { + return write!(f, "ReactiveContext(for scope: {:?})", scope); + } + return write!(f, "ReactiveContext created at {}", read.origin); + } + } + write!(f, "ReactiveContext") + } +} + +impl ReactiveContext { + /// Create a new reactive context + #[track_caller] + pub fn new() -> (Self, UnboundedReceiver<()>) { + Self::new_with_origin(std::panic::Location::caller()) + } + + /// Create a new reactive context with a location for debugging purposes + /// This is useful for reactive contexts created within closures + pub fn new_with_origin( + origin: &'static std::panic::Location<'static>, + ) -> (Self, UnboundedReceiver<()>) { + let (tx, rx) = futures_channel::mpsc::unbounded(); + let callback = move || { + // If there is already an update queued, we don't need to queue another + if !tx.is_empty() { + return; + } + let _ = tx.unbounded_send(()); + }; + let _self = Self::new_with_callback(callback, current_scope_id().unwrap(), origin); + (_self, rx) + } + + /// Create a new reactive context that may update a scope. When any signal that this context subscribes to changes, the callback will be run + pub fn new_with_callback( + callback: impl FnMut() + Send + Sync + 'static, + scope: ScopeId, + #[allow(unused)] origin: &'static std::panic::Location<'static>, + ) -> Self { + let inner = Inner { + self_: None, + update: Box::new(callback), + subscribers: Default::default(), + #[cfg(debug_assertions)] + origin, + #[cfg(debug_assertions)] + scope: None, + }; + + let owner = scope.owner(); + + let self_ = Self { + scope, + inner: owner.insert(inner), + }; + + self_.inner.write().self_ = Some(self_); + + self_ + } + + /// Get the current reactive context from the nearest reactive hook or scope + pub fn current() -> Option { + CURRENT.with(|current| current.borrow().last().cloned()) + } + + /// Create a reactive context for a scope id + pub(crate) fn new_for_scope(scope: &Scope, runtime: &Runtime) -> Self { + let id = scope.id; + let sender = runtime.sender.clone(); + let update_scope = move || { + tracing::trace!("Marking scope {:?} as dirty", id); + sender.unbounded_send(SchedulerMsg::Immediate(id)).unwrap(); + }; + + // Otherwise, create a new context at the current scope + let inner = Inner { + self_: None, + update: Box::new(update_scope), + subscribers: Default::default(), + #[cfg(debug_assertions)] + origin: std::panic::Location::caller(), + #[cfg(debug_assertions)] + scope: Some(id), + }; + + let owner = scope.owner(); + + let self_ = Self { + scope: id, + inner: owner.insert(inner), + }; + + self_.inner.write().self_ = Some(self_); + + self_ + } + + /// Clear all subscribers to this context + pub fn clear_subscribers(&self) { + let old_subscribers = std::mem::take(&mut self.inner.write().subscribers); + for subscriber in old_subscribers { + subscriber.0.lock().unwrap().remove(self); + } + } + + /// Update the subscribers + pub(crate) fn update_subscribers(&self) { + let subscribers = &self.inner.read().subscribers; + for subscriber in subscribers.iter() { + subscriber.0.lock().unwrap().insert(*self); + } + } + + /// Reset the reactive context and then run the callback in the context. This can be used to create custom reactive hooks like `use_memo`. + /// + /// ```rust, no_run + /// # use dioxus::prelude::*; + /// # use futures_util::StreamExt; + /// fn use_simplified_memo(mut closure: impl FnMut() -> i32 + 'static) -> Signal { + /// use_hook(|| { + /// // Create a new reactive context and channel that will recieve a value every time a value the reactive context subscribes to changes + /// let (reactive_context, mut changed) = ReactiveContext::new(); + /// // Compute the value of the memo inside the reactive context. This will subscribe the reactive context to any values you read inside the closure + /// let value = reactive_context.reset_and_run_in(&mut closure); + /// // Create a new signal with the value of the memo + /// let mut signal = Signal::new(value); + /// // Create a task that reruns the closure when the reactive context changes + /// spawn(async move { + /// while changed.next().await.is_some() { + /// // Since we reset the reactive context as we run the closure, our memo will only subscribe to the new values that are read in the closure + /// let new_value = reactive_context.run_in(&mut closure); + /// if new_value != value { + /// signal.set(new_value); + /// } + /// } + /// }); + /// signal + /// }) + /// } + /// + /// let mut boolean = use_signal(|| false); + /// let mut count = use_signal(|| 0); + /// // Because we use `reset_and_run_in` instead of just `run_in`, our memo will only subscribe to the signals that are read this run of the closure (initially just the boolean) + /// let memo = use_simplified_memo(move || if boolean() { count() } else { 0 }); + /// println!("{memo}"); + /// // Because the count signal is not read in this run of the closure, the memo will not rerun + /// count += 1; + /// println!("{memo}"); + /// // Because the boolean signal is read in this run of the closure, the memo will rerun + /// boolean.toggle(); + /// println!("{memo}"); + /// // If we toggle the boolean again, and the memo unsubscribes from the count signal + /// boolean.toggle(); + /// println!("{memo}"); + /// ``` + pub fn reset_and_run_in(&self, f: impl FnOnce() -> O) -> O { + self.clear_subscribers(); + self.run_in(f) + } + + /// Run this function in the context of this reactive context + /// + /// This will set the current reactive context to this context for the duration of the function. + /// You can then get information about the current subscriptions. + pub fn run_in(&self, f: impl FnOnce() -> O) -> O { + CURRENT.with(|current| current.borrow_mut().push(*self)); + let out = f(); + CURRENT.with(|current| current.borrow_mut().pop()); + self.update_subscribers(); + out + } + + /// Marks this reactive context as dirty + /// + /// If there's a scope associated with this context, then it will be marked as dirty too + /// + /// Returns true if the context was marked as dirty, or false if the context has been dropped + pub fn mark_dirty(&self) -> bool { + if let Ok(mut self_write) = self.inner.try_write() { + #[cfg(debug_assertions)] + { + tracing::trace!( + "Marking reactive context created at {} as dirty", + self_write.origin + ); + } + + (self_write.update)(); + + true + } else { + false + } + } + + /// Subscribe to this context. The reactive context will automatically remove itself from the subscriptions when it is reset. + pub fn subscribe(&self, subscriptions: Arc>>) { + subscriptions.lock().unwrap().insert(*self); + self.inner + .write() + .subscribers + .insert(PointerHash(subscriptions)); + } + + /// Get the scope that inner CopyValue is associated with + pub fn origin_scope(&self) -> ScopeId { + self.scope + } +} + +impl Hash for ReactiveContext { + fn hash(&self, state: &mut H) { + self.inner.id().hash(state); + } +} + +struct PointerHash(Arc); + +impl Hash for PointerHash { + fn hash(&self, state: &mut H) { + std::sync::Arc::::as_ptr(&self.0).hash(state); + } +} + +impl PartialEq for PointerHash { + fn eq(&self, other: &Self) -> bool { + std::sync::Arc::::as_ptr(&self.0) == std::sync::Arc::::as_ptr(&other.0) + } +} + +impl Eq for PointerHash {} + +impl Clone for PointerHash { + fn clone(&self) -> Self { + Self(self.0.clone()) + } +} + +type SubscriberMap = Mutex>; + +struct Inner { + self_: Option, + + // Futures will call .changed().await + update: Box, + + // Subscribers to this context + subscribers: HashSet>, + + // Debug information for signal subscriptions + #[cfg(debug_assertions)] + origin: &'static std::panic::Location<'static>, + + #[cfg(debug_assertions)] + // The scope that this reactive context is associated with + scope: Option, +} diff --git a/packages/core/src/scope_arena.rs b/packages/core/src/scope_arena.rs index 86cbd1a3e..6fe57147a 100644 --- a/packages/core/src/scope_arena.rs +++ b/packages/core/src/scope_arena.rs @@ -1,4 +1,5 @@ use crate::innerlude::ScopeOrder; +use crate::reactive_context::ReactiveContext; use crate::{ any_props::{AnyProps, BoxedAnyProps}, innerlude::ScopeState, @@ -17,16 +18,18 @@ impl VirtualDom { let entry = self.scopes.vacant_entry(); let id = ScopeId(entry.key()); + let scope_runtime = Scope::new(name, id, parent_id, height); + let reactive_context = ReactiveContext::new_for_scope(&scope_runtime, &self.runtime); + self.runtime.create_scope(scope_runtime); + let scope = entry.insert(ScopeState { runtime: self.runtime.clone(), context_id: id, props, last_rendered_node: Default::default(), + reactive_context, }); - self.runtime - .create_scope(Scope::new(name, id, parent_id, height)); - scope } @@ -52,7 +55,7 @@ impl VirtualDom { let props: &dyn AnyProps = &*scope.props; let span = tracing::trace_span!("render", scope = %scope.state().name); - span.in_scope(|| props.render()) + span.in_scope(|| scope.reactive_context.reset_and_run_in(|| props.render())) }; let context = scope.state(); diff --git a/packages/core/src/scope_context.rs b/packages/core/src/scope_context.rs index a22424109..3f9134d62 100644 --- a/packages/core/src/scope_context.rs +++ b/packages/core/src/scope_context.rs @@ -1,4 +1,5 @@ use crate::{innerlude::SchedulerMsg, Element, Runtime, ScopeId, Task}; +use generational_box::{AnyStorage, Owner}; use rustc_hash::FxHashSet; use std::{ any::Any, @@ -91,6 +92,17 @@ impl Scope { }) } + /// Get the owner for the current scope. + pub fn owner(&self) -> Owner { + match self.has_context() { + Some(rt) => rt, + None => { + let owner = S::owner(); + self.provide_context(owner) + } + } + } + /// Return any context of type T if it exists on this scope pub fn has_context(&self) -> Option { self.shared_contexts diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 266cce3eb..c9e0592ab 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -1,5 +1,6 @@ use crate::{ - any_props::BoxedAnyProps, nodes::RenderReturn, runtime::Runtime, scope_context::Scope, + any_props::BoxedAnyProps, nodes::RenderReturn, reactive_context::ReactiveContext, + runtime::Runtime, scope_context::Scope, }; use std::{cell::Ref, rc::Rc}; @@ -53,6 +54,7 @@ pub struct ScopeState { pub(crate) context_id: ScopeId, pub(crate) last_rendered_node: Option, pub(crate) props: BoxedAnyProps, + pub(crate) reactive_context: ReactiveContext, } impl Drop for ScopeState { diff --git a/packages/hooks/src/use_effect.rs b/packages/hooks/src/use_effect.rs index f5cacdd60..cf911f63e 100644 --- a/packages/hooks/src/use_effect.rs +++ b/packages/hooks/src/use_effect.rs @@ -1,5 +1,6 @@ +use std::{cell::Cell, rc::Rc}; + use dioxus_core::prelude::*; -use dioxus_signals::ReactiveContext; use futures_util::StreamExt; use crate::use_callback; @@ -15,12 +16,24 @@ pub fn use_effect(callback: impl FnMut() + 'static) -> Effect { use_hook(|| { // Inside the effect, we track any reads so that we can rerun the effect if a value the effect reads changes let (rc, mut changed) = ReactiveContext::new_with_origin(location); + + // Deduplicate queued effects + let effect_queued = Rc::new(Cell::new(false)); + // Spawn a task that will run the effect when: // 1) The component is first run // 2) The effect is rerun due to an async read at any time // 3) The effect is rerun in the same tick that the component is rerun: we need to wait for the component to rerun before we can run the effect again let queue_effect_for_next_render = move || { - queue_effect(move || rc.run_in(&*callback)); + if effect_queued.get() { + return; + } + effect_queued.set(true); + let effect_queued = effect_queued.clone(); + queue_effect(move || { + rc.reset_and_run_in(&*callback); + effect_queued.set(false); + }); }; queue_effect_for_next_render(); diff --git a/packages/hooks/src/use_resource.rs b/packages/hooks/src/use_resource.rs index 4329b7362..55ccde05c 100644 --- a/packages/hooks/src/use_resource.rs +++ b/packages/hooks/src/use_resource.rs @@ -30,7 +30,7 @@ where let cb = use_callback(move || { // Create the user's task - let fut = rc.run_in(&mut future); + let fut = rc.reset_and_run_in(&mut future); // Spawn a wrapper task that polls the inner future and watch its dependencies spawn(async move { diff --git a/packages/hooks/tests/memo.rs b/packages/hooks/tests/memo.rs index 77e59f3d5..2d50ee47f 100644 --- a/packages/hooks/tests/memo.rs +++ b/packages/hooks/tests/memo.rs @@ -50,21 +50,73 @@ async fn memo_updates() { } } - let mut dom = VirtualDom::new(app); + let race = async move { + let mut dom = VirtualDom::new(app); - dom.rebuild_in_place(); - let mut signal = VEC_SIGNAL.with(|cell| (*cell.borrow()).unwrap()); - // Wait for the signal to update - for _ in 0..2 { - dom.wait_for_work().await; - dom.render_immediate(&mut dioxus::dioxus_core::NoOpMutations); + dom.rebuild_in_place(); + let mut signal = VEC_SIGNAL.with(|cell| (*cell.borrow()).unwrap()); + // Wait for the signal to update + for _ in 0..2 { + dom.wait_for_work().await; + dom.render_immediate(&mut dioxus::dioxus_core::NoOpMutations); + } + assert_eq!(signal(), vec![0, 1, 2, 3, 4, 5]); + // Remove each element from the vec + for _ in 0..6 { + signal.pop(); + dom.wait_for_work().await; + dom.render_immediate(&mut dioxus::dioxus_core::NoOpMutations); + println!("Signal: {signal:?}"); + } + }; + + tokio::select! { + _ = race => {}, + _ = tokio::time::sleep(std::time::Duration::from_millis(1000)) => panic!("timed out") + }; +} + +#[tokio::test] +async fn use_memo_only_triggers_one_update() { + use dioxus::prelude::*; + use std::cell::RefCell; + + thread_local! { + static VEC_SIGNAL: RefCell> = const { RefCell::new(Vec::new()) }; } - assert_eq!(signal(), vec![0, 1, 2, 3, 4, 5]); - // Remove each element from the vec - for _ in 0..6 { - signal.pop(); - dom.wait_for_work().await; - dom.render_immediate(&mut dioxus::dioxus_core::NoOpMutations); - println!("Signal: {signal:?}"); + + fn app() -> Element { + let mut count = use_signal(|| 0); + + let memorized = use_memo(move || dbg!(count() * 2)); + + use_memo(move || { + println!("reading doubled"); + let doubled = memorized(); + VEC_SIGNAL.with_borrow_mut(|v| v.push(doubled)) + }); + + // Writing to count many times in a row should not cause the memo to update other subscribers multiple times + use_hook(move || { + for _ in 0..10 { + count += 1; + // Reading the memo each time will trigger the memo to rerun immediately, but the VEC_SIGNAL should still only rerun once + println!("doubled {memorized}"); + } + }); + + rsx! {} } + + let mut dom = VirtualDom::new(app); + dom.rebuild_in_place(); + + tokio::select! { + _ = dom.wait_for_work() => {}, + _ = tokio::time::sleep(std::time::Duration::from_millis(100)) => {} + }; + + dom.render_immediate(&mut dioxus::dioxus_core::NoOpMutations); + + assert_eq!(VEC_SIGNAL.with(|v| v.borrow().clone()), vec![0, 20]); } diff --git a/packages/signals/src/lib.rs b/packages/signals/src/lib.rs index 9668d3d9c..dd2f4aae9 100644 --- a/packages/signals/src/lib.rs +++ b/packages/signals/src/lib.rs @@ -39,6 +39,3 @@ pub use write::*; mod props; pub use props::*; - -mod reactive_context; -pub use reactive_context::*; diff --git a/packages/signals/src/memo.rs b/packages/signals/src/memo.rs index 853262606..13a42af7b 100644 --- a/packages/signals/src/memo.rs +++ b/packages/signals/src/memo.rs @@ -1,6 +1,6 @@ use crate::read_impls; use crate::write::Writable; -use crate::{read::Readable, ReactiveContext, ReadableRef, Signal}; +use crate::{read::Readable, ReadableRef, Signal}; use crate::{CopyValue, ReadOnlySignal}; use std::{ cell::RefCell, @@ -10,7 +10,7 @@ use std::{ use dioxus_core::prelude::*; use futures_util::StreamExt; -use generational_box::UnsyncStorage; +use generational_box::{AnyStorage, UnsyncStorage}; struct UpdateInformation { dirty: Arc, @@ -53,7 +53,7 @@ impl Memo { where T: PartialEq, { - let dirty = Arc::new(AtomicBool::new(true)); + let dirty = Arc::new(AtomicBool::new(false)); let (tx, mut rx) = futures_channel::mpsc::unbounded(); let callback = { @@ -67,7 +67,7 @@ impl Memo { ReactiveContext::new_with_callback(callback, current_scope_id().unwrap(), location); // Create a new signal in that context, wiring up its dependencies and subscribers - let mut recompute = move || rc.run_in(&mut f); + let mut recompute = move || rc.reset_and_run_in(&mut f); let value = recompute(); let recompute = RefCell::new(Box::new(recompute) as Box T>); let update = CopyValue::new(UpdateInformation { @@ -134,24 +134,30 @@ where fn try_read_unchecked( &self, ) -> Result, generational_box::BorrowError> { - let read = self.inner.try_read_unchecked(); - match read { - Ok(r) => { - let needs_update = self - .update - .read() - .dirty - .swap(false, std::sync::atomic::Ordering::Relaxed); - if needs_update { - drop(r); - self.recompute(); - self.inner.try_read_unchecked() - } else { - Ok(r) - } + // Read the inner generational box instead of the signal so we have more fine grained control over exactly when the subscription happens + let read = self.inner.inner.try_read_unchecked()?; + + let needs_update = self + .update + .read() + .dirty + .swap(false, std::sync::atomic::Ordering::Relaxed); + let result = if needs_update { + drop(read); + // We shouldn't be subscribed to the value here so we don't trigger the scope we are currently in to rerun even though that scope got the latest value because we synchronously update the value: https://github.com/DioxusLabs/dioxus/issues/2416 + self.recompute(); + self.inner.inner.try_read_unchecked() + } else { + Ok(read) + }; + // Subscribe to the current scope before returning the value + if let Ok(read) = &result { + if let Some(reactive_context) = ReactiveContext::current() { + tracing::trace!("Subscribing to the reactive context {}", reactive_context); + reactive_context.subscribe(read.subscribers.clone()); } - Err(e) => Err(e), } + result.map(|read| ::map(read, |v| &v.value)) } /// Get the current value of the signal. **Unlike read, this will not subscribe the current scope to the signal which can cause parts of your UI to not update.** diff --git a/packages/signals/src/reactive_context.rs b/packages/signals/src/reactive_context.rs deleted file mode 100644 index 7a2fbf50e..000000000 --- a/packages/signals/src/reactive_context.rs +++ /dev/null @@ -1,181 +0,0 @@ -use dioxus_core::prelude::{ - current_scope_id, has_context, provide_context, schedule_update_any, ScopeId, -}; -use futures_channel::mpsc::UnboundedReceiver; -use generational_box::SyncStorage; -use std::{cell::RefCell, hash::Hash}; - -use crate::{CopyValue, Writable}; - -#[doc = include_str!("../docs/reactivity.md")] -#[derive(Clone, Copy, PartialEq, Eq)] -pub struct ReactiveContext { - inner: CopyValue, -} - -thread_local! { - static CURRENT: RefCell> = const { RefCell::new(vec![]) }; -} - -impl std::fmt::Display for ReactiveContext { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - #[cfg(debug_assertions)] - { - use crate::Readable; - if let Ok(read) = self.inner.try_read() { - if let Some(scope) = read.scope { - return write!(f, "ReactiveContext(for scope: {:?})", scope); - } - return write!(f, "ReactiveContext created at {}", read.origin); - } - } - write!(f, "ReactiveContext") - } -} - -impl ReactiveContext { - /// Create a new reactive context - #[track_caller] - pub fn new() -> (Self, UnboundedReceiver<()>) { - Self::new_with_origin(std::panic::Location::caller()) - } - - /// Create a new reactive context with a location for debugging purposes - /// This is useful for reactive contexts created within closures - pub fn new_with_origin( - origin: &'static std::panic::Location<'static>, - ) -> (Self, UnboundedReceiver<()>) { - let (tx, rx) = futures_channel::mpsc::unbounded(); - let callback = move || { - let _ = tx.unbounded_send(()); - }; - let _self = Self::new_with_callback(callback, current_scope_id().unwrap(), origin); - (_self, rx) - } - - /// Create a new reactive context that may update a scope. When any signal that this context subscribes to changes, the callback will be run - pub fn new_with_callback( - callback: impl FnMut() + Send + Sync + 'static, - scope: ScopeId, - #[allow(unused)] origin: &'static std::panic::Location<'static>, - ) -> Self { - let inner = Inner { - self_: None, - update: Box::new(callback), - #[cfg(debug_assertions)] - origin, - #[cfg(debug_assertions)] - scope: None, - }; - - let mut self_ = Self { - inner: CopyValue::new_maybe_sync_in_scope(inner, scope), - }; - - self_.inner.write().self_ = Some(self_); - - self_ - } - - /// Get the current reactive context - /// - /// If this was set manually, then that value will be returned. - /// - /// If there's no current reactive context, then a new one will be created for the current scope and returned. - pub fn current() -> Option { - let cur = CURRENT.with(|current| current.borrow().last().cloned()); - - // If we're already inside a reactive context, then return that - if let Some(cur) = cur { - return Some(cur); - } - - // If we're rendering, then try and use the reactive context attached to this component - if !dioxus_core::vdom_is_rendering() { - return None; - } - if let Some(cx) = has_context() { - return Some(cx); - } - let update_any = schedule_update_any(); - let scope_id = current_scope_id().unwrap(); - let update_scope = move || { - tracing::trace!("Marking scope {:?} as dirty", scope_id); - update_any(scope_id) - }; - - // Otherwise, create a new context at the current scope - #[allow(unused_mut)] - let mut reactive_context = ReactiveContext::new_with_callback( - update_scope, - scope_id, - std::panic::Location::caller(), - ); - #[cfg(debug_assertions)] - { - // Associate the reactive context with the current scope for debugging - reactive_context.inner.write().scope = Some(scope_id); - } - Some(provide_context(reactive_context)) - } - - /// Run this function in the context of this reactive context - /// - /// This will set the current reactive context to this context for the duration of the function. - /// You can then get information about the current subscriptions. - pub fn run_in(&self, f: impl FnOnce() -> O) -> O { - CURRENT.with(|current| current.borrow_mut().push(*self)); - let out = f(); - CURRENT.with(|current| current.borrow_mut().pop()); - out - } - - /// Marks this reactive context as dirty - /// - /// If there's a scope associated with this context, then it will be marked as dirty too - /// - /// Returns true if the context was marked as dirty, or false if the context has been dropped - pub fn mark_dirty(&self) -> bool { - if let Ok(mut self_write) = self.inner.try_write_unchecked() { - #[cfg(debug_assertions)] - { - tracing::trace!( - "Marking reactive context created at {} as dirty", - self_write.origin - ); - } - - (self_write.update)(); - - true - } else { - false - } - } - - /// Get the scope that inner CopyValue is associated with - pub fn origin_scope(&self) -> ScopeId { - self.inner.origin_scope() - } -} - -impl Hash for ReactiveContext { - fn hash(&self, state: &mut H) { - self.inner.id().hash(state); - } -} - -struct Inner { - self_: Option, - - // Futures will call .changed().await - update: Box, - - // Debug information for signal subscriptions - #[cfg(debug_assertions)] - origin: &'static std::panic::Location<'static>, - - #[cfg(debug_assertions)] - // The scope that this reactive context is associated with - scope: Option, -} diff --git a/packages/signals/src/set_compare.rs b/packages/signals/src/set_compare.rs index d8ddca912..63fdf3d46 100644 --- a/packages/signals/src/set_compare.rs +++ b/packages/signals/src/set_compare.rs @@ -1,4 +1,4 @@ -use crate::{write::Writable, ReactiveContext}; +use crate::write::Writable; use std::hash::Hash; use crate::read::Readable; @@ -57,7 +57,7 @@ impl>> SetCompare { spawn(async move { loop { // Recompute the value - rc.run_in(&mut recompute); + rc.reset_and_run_in(&mut recompute); // Wait for context to change let _ = changed.next().await; diff --git a/packages/signals/src/signal.rs b/packages/signals/src/signal.rs index 82316bb3e..abe3f7afa 100644 --- a/packages/signals/src/signal.rs +++ b/packages/signals/src/signal.rs @@ -1,9 +1,9 @@ use crate::{default_impl, fmt_impls, write_impls}; -use crate::{read::*, write::*, CopyValue, GlobalMemo, GlobalSignal, ReactiveContext, ReadableRef}; +use crate::{read::*, write::*, CopyValue, GlobalMemo, GlobalSignal, ReadableRef}; use crate::{Memo, WritableRef}; -use dioxus_core::IntoDynNode; -use dioxus_core::{prelude::IntoAttributeValue, ScopeId}; +use dioxus_core::prelude::*; use generational_box::{AnyStorage, Storage, SyncStorage, UnsyncStorage}; +use std::sync::Arc; use std::{ any::Any, collections::HashSet, @@ -28,7 +28,7 @@ pub type SyncSignal = Signal; /// The data stored for tracking in a signal. pub struct SignalData { - pub(crate) subscribers: Mutex>, + pub(crate) subscribers: Arc>>, pub(crate) value: T, } @@ -355,7 +355,7 @@ impl>> Readable for Signal { if let Some(reactive_context) = ReactiveContext::current() { tracing::trace!("Subscribing to the reactive context {}", reactive_context); - inner.subscribers.lock().unwrap().insert(reactive_context); + reactive_context.subscribe(inner.subscribers.clone()); } Ok(S::map(inner, |v| &v.value))