An ever Increasing Component ID (#2537)

* Bring changes to this branch.

* Child components always render after parents.

* Cleanup residual Portal references.

* Fix after merge.

* Fix trybuild.

* Opt for usize.

* Strip Generics.

* take instead of replace, first instead of front.

packages/yew/Cargo.toml

@@ -79,7 +79,7 @@ trybuild = "1"
 [features]
 ssr = ["futures", "html-escape"]
 csr = []
-doc_test = ["csr"]
+doc_test = ["csr", "ssr"]
 wasm_test = ["csr"]
 default = []
 
@@ -87,5 +87,5 @@ default = []
 tokio = { version = "1.15.0", features = ["full"] }
 
 [package.metadata.docs.rs]
-features = ["doc_test", "ssr"]
+features = ["doc_test"]
 rustdoc-args = ["--cfg", "documenting"]

packages/yew/src/html/component/lifecycle.rs

@@ -153,9 +153,7 @@ pub(crate) struct ComponentState {
 
     suspension: Option<Suspension>,
 
-    // Used for debug logging
+    pub(crate) comp_id: usize,
-    #[cfg(debug_assertions)]
-    pub(crate) vcomp_id: usize,
 }
 
 impl ComponentState {
@@ -164,8 +162,7 @@ impl ComponentState {
         scope: Scope<COMP>,
         props: Rc<COMP::Properties>,
     ) -> Self {
-        #[cfg(debug_assertions)]
+        let comp_id = scope.id;
-        let vcomp_id = scope.vcomp_id;
         let context = Context { scope, props };
 
         let inner = Box::new(CompStateInner {
@@ -181,8 +178,7 @@ impl ComponentState {
             #[cfg(feature = "csr")]
             has_rendered: false,
 
-            #[cfg(debug_assertions)]
+            comp_id,
-            vcomp_id,
         }
     }
 
@@ -208,7 +204,7 @@ impl<COMP: BaseComponent> Runnable for CreateRunner<COMP> {
         let mut current_state = self.scope.state.borrow_mut();
         if current_state.is_none() {
             #[cfg(debug_assertions)]
-            super::log_event(self.scope.vcomp_id, "create");
+            super::log_event(self.scope.id, "create");
 
             *current_state = Some(ComponentState::new(
                 self.initial_render_state,
@@ -240,6 +236,7 @@ impl Runnable for UpdateRunner {
         if let Some(state) = self.state.borrow_mut().as_mut() {
             let schedule_render = match self.event {
                 UpdateEvent::Message => state.inner.flush_messages(),
+
                 #[cfg(feature = "csr")]
                 UpdateEvent::Properties(props, next_node_ref, next_sibling) => {
                     match state.render_state {
@@ -297,16 +294,16 @@ impl Runnable for UpdateRunner {
 
             #[cfg(debug_assertions)]
             super::log_event(
-                state.vcomp_id,
+                state.comp_id,
                 format!("update(schedule_render={})", schedule_render),
             );
 
             if schedule_render {
                 scheduler::push_component_render(
-                    self.state.as_ptr() as usize,
+                    state.comp_id,
-                    RenderRunner {
+                    Box::new(RenderRunner {
                         state: self.state.clone(),
-                    },
+                    }),
                 );
                 // Only run from the scheduler, so no need to call `scheduler::start()`
             }
@@ -325,7 +322,7 @@ impl Runnable for DestroyRunner {
     fn run(self: Box<Self>) {
         if let Some(mut state) = self.state.borrow_mut().take() {
             #[cfg(debug_assertions)]
-            super::log_event(state.vcomp_id, "destroy");
+            super::log_event(state.comp_id, "destroy");
 
             state.inner.destroy();
 
@@ -357,7 +354,7 @@ impl Runnable for RenderRunner {
     fn run(self: Box<Self>) {
         if let Some(state) = self.state.borrow_mut().as_mut() {
             #[cfg(debug_assertions)]
-            super::log_event(state.vcomp_id, "render");
+            super::log_event(state.comp_id, "render");
 
             match state.inner.view() {
                 Ok(m) => self.render(state, m),
@@ -373,14 +370,16 @@ impl RenderRunner {
         // suspension to parent element.
         let shared_state = self.state.clone();
 
+        let comp_id = state.comp_id;
+
         if suspension.resumed() {
             // schedule a render immediately if suspension is resumed.
 
             scheduler::push_component_render(
-                shared_state.as_ptr() as usize,
+                state.comp_id,
-                RenderRunner {
+                Box::new(RenderRunner {
                     state: shared_state,
-                },
+                }),
             );
         } else {
             // We schedule a render after current suspension is resumed.
@@ -393,10 +392,10 @@ impl RenderRunner {
 
             suspension.listen(Callback::from(move |_| {
                 scheduler::push_component_render(
-                    shared_state.as_ptr() as usize,
+                    comp_id,
-                    RenderRunner {
+                    Box::new(RenderRunner {
                         state: shared_state.clone(),
-                    },
+                    }),
                 );
                 scheduler::start();
             }));
@@ -442,11 +441,11 @@ impl RenderRunner {
                 state.has_rendered = true;
 
                 scheduler::push_component_rendered(
-                    self.state.as_ptr() as usize,
+                    state.comp_id,
-                    RenderedRunner {
+                    Box::new(RenderedRunner {
                         state: self.state.clone(),
                         first_render,
-                    },
+                    }),
                     first_render,
                 );
             }
@@ -474,7 +473,7 @@ mod feat_csr {
         fn run(self: Box<Self>) {
             if let Some(state) = self.state.borrow_mut().as_mut() {
                 #[cfg(debug_assertions)]
-                super::super::log_event(state.vcomp_id, "rendered");
+                super::super::log_event(state.comp_id, "rendered");
 
                 if state.suspension.is_none() {
                     state.inner.rendered(self.first_render);

packages/yew/src/html/component/mod.rs

@@ -14,46 +14,36 @@ pub(crate) use scope::Scoped;
 pub use scope::{AnyScope, Scope, SendAsMessage};
 use std::rc::Rc;
 
+#[cfg(debug_assertions)]
 #[cfg(any(feature = "csr", feature = "ssr"))]
 mod feat_csr_ssr {
-    #[cfg(debug_assertions)]
     thread_local! {
          static EVENT_HISTORY: std::cell::RefCell<std::collections::HashMap<usize, Vec<String>>>
             = Default::default();
-        static COMP_ID_COUNTER: AtomicUsize = AtomicUsize::new(0);
     }
 
     /// Push [Component] event to lifecycle debugging registry
-    #[cfg(debug_assertions)]
+    pub(crate) fn log_event(comp_id: usize, event: impl ToString) {
-    pub(crate) fn log_event(vcomp_id: usize, event: impl ToString) {
         EVENT_HISTORY.with(|h| {
             h.borrow_mut()
-                .entry(vcomp_id)
+                .entry(comp_id)
                 .or_default()
                 .push(event.to_string())
         });
     }
 
     /// Get [Component] event log from lifecycle debugging registry
-    #[cfg(debug_assertions)]
     #[allow(dead_code)]
-    pub(crate) fn get_event_log(vcomp_id: usize) -> Vec<String> {
+    pub(crate) fn get_event_log(comp_id: usize) -> Vec<String> {
         EVENT_HISTORY.with(|h| {
             h.borrow()
-                .get(&vcomp_id)
+                .get(&comp_id)
                 .map(|l| (*l).clone())
                 .unwrap_or_default()
         })
     }
-
-    #[cfg(debug_assertions)]
-    pub(crate) fn next_id() -> usize {
-        COMP_ID_COUNTER.with(|m| m.fetch_add(1, Ordering::Relaxed))
-    }
-
-    #[cfg(debug_assertions)]
-    use std::sync::atomic::{AtomicUsize, Ordering};
 }
+
 #[cfg(debug_assertions)]
 #[cfg(any(feature = "csr", feature = "ssr"))]
 pub(crate) use feat_csr_ssr::*;

packages/yew/src/html/component/scope.rs

@@ -8,6 +8,7 @@ use std::cell::RefCell;
 #[cfg(any(feature = "csr", feature = "ssr"))]
 use super::lifecycle::{ComponentState, UpdateEvent, UpdateRunner};
 use super::BaseComponent;
+
 use crate::callback::Callback;
 use crate::context::{ContextHandle, ContextProvider};
 use crate::html::IntoComponent;
@@ -112,8 +113,7 @@ pub struct Scope<COMP: BaseComponent> {
     #[cfg(any(feature = "csr", feature = "ssr"))]
     pub(crate) state: Shared<Option<ComponentState>>,
 
-    #[cfg(debug_assertions)]
+    pub(crate) id: usize,
-    pub(crate) vcomp_id: usize,
 }
 
 impl<COMP: BaseComponent> fmt::Debug for Scope<COMP> {
@@ -134,8 +134,7 @@ impl<COMP: BaseComponent> Clone for Scope<COMP> {
             #[cfg(any(feature = "csr", feature = "ssr"))]
             state: self.state.clone(),
 
-            #[cfg(debug_assertions)]
+            id: self.id,
-            vcomp_id: self.vcomp_id,
         }
     }
 }
@@ -211,14 +210,15 @@ mod feat_ssr {
             let state = ComponentRenderState::Ssr { sender: Some(tx) };
 
             scheduler::push_component_create(
-                CreateRunner {
+                self.id,
+                Box::new(CreateRunner {
                     initial_render_state: state,
                     props,
                     scope: self.clone(),
-                },
+                }),
-                RenderRunner {
+                Box::new(RenderRunner {
                     state: self.state.clone(),
-                },
+                }),
             );
             scheduler::start();
 
@@ -227,12 +227,12 @@ mod feat_ssr {
             let self_any_scope = AnyScope::from(self.clone());
             html.render_to_string(w, &self_any_scope).await;
 
-            scheduler::push_component_destroy(DestroyRunner {
+            scheduler::push_component_destroy(Box::new(DestroyRunner {
                 state: self.state.clone(),
 
                 #[cfg(feature = "csr")]
                 parent_to_detach: false,
-            });
+            }));
             scheduler::start();
         }
     }
@@ -269,6 +269,7 @@ mod feat_csr_ssr {
     use super::*;
     use crate::scheduler::{self, Shared};
     use std::cell::Ref;
+    use std::sync::atomic::{AtomicUsize, Ordering};
 
     #[derive(Debug)]
     pub(crate) struct MsgQueue<Msg>(Shared<Vec<Msg>>);
@@ -308,6 +309,8 @@ mod feat_csr_ssr {
         }
     }
 
+    static COMP_ID_COUNTER: AtomicUsize = AtomicUsize::new(0);
+
     impl<COMP: BaseComponent> Scope<COMP> {
         /// Crate a scope with an optional parent scope
         pub(crate) fn new(parent: Option<AnyScope>) -> Self {
@@ -325,8 +328,7 @@ mod feat_csr_ssr {
                 state,
                 parent,
 
-                #[cfg(debug_assertions)]
+                id: COMP_ID_COUNTER.fetch_add(1, Ordering::SeqCst),
-                vcomp_id: super::super::next_id(),
             }
         }
 
@@ -345,10 +347,10 @@ mod feat_csr_ssr {
         }
 
         pub(super) fn push_update(&self, event: UpdateEvent) {
-            scheduler::push_component_update(UpdateRunner {
+            scheduler::push_component_update(Box::new(UpdateRunner {
                 state: self.state.clone(),
                 event,
-            });
+            }));
             // Not guaranteed to already have the scheduler started
             scheduler::start();
         }
@@ -415,14 +417,15 @@ mod feat_csr {
             };
 
             scheduler::push_component_create(
-                CreateRunner {
+                self.id,
+                Box::new(CreateRunner {
                     initial_render_state: state,
                     props,
                     scope: self.clone(),
-                },
+                }),
-                RenderRunner {
+                Box::new(RenderRunner {
                     state: self.state.clone(),
-                },
+                }),
             );
             // Not guaranteed to already have the scheduler started
             scheduler::start();
@@ -435,7 +438,7 @@ mod feat_csr {
             next_sibling: NodeRef,
         ) {
             #[cfg(debug_assertions)]
-            super::super::log_event(self.vcomp_id, "reuse");
+            super::super::log_event(self.id, "reuse");
 
             self.push_update(UpdateEvent::Properties(props, node_ref, next_sibling));
         }
@@ -470,10 +473,10 @@ mod feat_csr {
 
         /// Process an event to destroy a component
         fn destroy(self, parent_to_detach: bool) {
-            scheduler::push_component_destroy(DestroyRunner {
+            scheduler::push_component_destroy(Box::new(DestroyRunner {
                 state: self.state,
                 parent_to_detach,
-            });
+            }));
             // Not guaranteed to already have the scheduler started
             scheduler::start();
         }
@@ -483,10 +486,10 @@ mod feat_csr {
         }
 
         fn shift_node(&self, parent: Element, next_sibling: NodeRef) {
-            scheduler::push_component_update(UpdateRunner {
+            scheduler::push_component_update(Box::new(UpdateRunner {
                 state: self.state.clone(),
                 event: UpdateEvent::Shift(parent, next_sibling),
-            })
+            }))
         }
     }
 }

packages/yew/src/scheduler.rs

@@ -1,7 +1,7 @@
 //! This module contains a scheduler.
 
 use std::cell::RefCell;
-use std::collections::VecDeque;
+use std::collections::BTreeMap;
 use std::rc::Rc;
 
 /// Alias for Rc<RefCell<T>>
@@ -24,15 +24,18 @@ struct Scheduler {
     destroy: Vec<Box<dyn Runnable>>,
     create: Vec<Box<dyn Runnable>>,
     update: Vec<Box<dyn Runnable>>,
-    render_first: VecDeque<Box<dyn Runnable>>,
 
-    #[cfg(any(feature = "ssr", feature = "csr"))]
+    /// The Binary Tree Map guarantees components with lower id (parent) is rendered first and
-    render: RenderScheduler,
+    /// no more than 1 render can be scheduled before a component is rendered.
+    ///
+    /// Parent can destroy child components but not otherwise, we can save unnecessary render by
+    /// rendering parent first.
+    render_first: BTreeMap<usize, Box<dyn Runnable>>,
+    render: BTreeMap<usize, Box<dyn Runnable>>,
 
-    /// Stacks to ensure child calls are always before parent calls
+    /// Binary Tree Map to guarantee children rendered are always called before parent calls
-    rendered_first: Vec<Box<dyn Runnable>>,
+    rendered_first: BTreeMap<usize, Box<dyn Runnable>>,
-    #[cfg(feature = "csr")]
+    rendered: BTreeMap<usize, Box<dyn Runnable>>,
-    rendered: RenderedScheduler,
 }
 
 /// Execute closure with a mutable reference to the scheduler
@@ -60,92 +63,33 @@ pub fn push(runnable: Box<dyn Runnable>) {
 #[cfg(any(feature = "ssr", feature = "csr"))]
 mod feat_csr_ssr {
     use super::*;
-
-    use std::collections::{hash_map::Entry, HashMap};
-
     /// Push a component creation, first render and first rendered [Runnable]s to be executed
     pub(crate) fn push_component_create(
-        create: impl Runnable + 'static,
+        component_id: usize,
-        first_render: impl Runnable + 'static,
+        create: Box<dyn Runnable>,
+        first_render: Box<dyn Runnable>,
     ) {
         with(|s| {
-            s.create.push(Box::new(create));
+            s.create.push(create);
-            s.render_first.push_back(Box::new(first_render));
+            s.render_first.insert(component_id, first_render);
         });
     }
 
     /// Push a component destruction [Runnable] to be executed
-    pub(crate) fn push_component_destroy(runnable: impl Runnable + 'static) {
+    pub(crate) fn push_component_destroy(runnable: Box<dyn Runnable>) {
-        with(|s| s.destroy.push(Box::new(runnable)));
+        with(|s| s.destroy.push(runnable));
     }
 
     /// Push a component render and rendered [Runnable]s to be executed
-    pub(crate) fn push_component_render(component_id: usize, render: impl Runnable + 'static) {
+    pub(crate) fn push_component_render(component_id: usize, render: Box<dyn Runnable>) {
         with(|s| {
-            s.render.schedule(component_id, Box::new(render));
+            s.render.insert(component_id, render);
         });
     }
 
     /// Push a component update [Runnable] to be executed
-    pub(crate) fn push_component_update(runnable: impl Runnable + 'static) {
+    pub(crate) fn push_component_update(runnable: Box<dyn Runnable>) {
-        with(|s| s.update.push(Box::new(runnable)));
+        with(|s| s.update.push(runnable));
-    }
-
-    /// Task to be executed for specific component
-    struct QueueTask {
-        /// Tasks in the queue to skip for this component
-        skip: usize,
-
-        /// Runnable to execute
-        runnable: Box<dyn Runnable>,
-    }
-
-    /// Scheduler for non-first component renders with deduplication
-    #[derive(Default)]
-    pub(super) struct RenderScheduler {
-        /// Task registry by component ID
-        tasks: HashMap<usize, QueueTask>,
-
-        /// Task queue by component ID
-        queue: VecDeque<usize>,
-    }
-
-    impl RenderScheduler {
-        /// Schedule render task execution
-        pub fn schedule(&mut self, component_id: usize, runnable: Box<dyn Runnable>) {
-            self.queue.push_back(component_id);
-            match self.tasks.entry(component_id) {
-                Entry::Vacant(e) => {
-                    e.insert(QueueTask { skip: 0, runnable });
-                }
-                Entry::Occupied(mut e) => {
-                    let v = e.get_mut();
-                    v.skip += 1;
-
-                    // Technically the 2 runners should be functionally identical, but might as well
-                    // overwrite it for good measure, accounting for future changes. We have it here
-                    // anyway.
-                    v.runnable = runnable;
-                }
-            }
-        }
-
-        /// Try to pop a task from the queue, if any
-        pub fn pop(&mut self) -> Option<Box<dyn Runnable>> {
-            while let Some(id) = self.queue.pop_front() {
-                match self.tasks.entry(id) {
-                    Entry::Occupied(mut e) => {
-                        let v = e.get_mut();
-                        if v.skip == 0 {
-                            return Some(e.remove().runnable);
-                        }
-                        v.skip -= 1;
-                    }
-                    Entry::Vacant(_) => (),
-                }
-            }
-            None
-        }
     }
 }
 
@@ -156,51 +100,19 @@ pub(crate) use feat_csr_ssr::*;
 mod feat_csr {
     use super::*;
 
-    use std::collections::HashMap;
-
     pub(crate) fn push_component_rendered(
         component_id: usize,
-        rendered: impl Runnable + 'static,
+        rendered: Box<dyn Runnable>,
         first_render: bool,
     ) {
         with(|s| {
-            let rendered = Box::new(rendered);
-
             if first_render {
-                s.rendered_first.push(rendered);
+                s.rendered_first.insert(component_id, rendered);
             } else {
-                s.rendered.schedule(component_id, rendered);
+                s.rendered.insert(component_id, rendered);
             }
         });
     }
-
-    /// Deduplicating scheduler for component rendered calls with deduplication
-    #[derive(Default)]
-    pub(super) struct RenderedScheduler {
-        /// Task registry by component ID
-        tasks: HashMap<usize, Box<dyn Runnable>>,
-
-        /// Task stack by component ID
-        stack: Vec<usize>,
-    }
-
-    impl RenderedScheduler {
-        /// Schedule rendered task execution
-        pub fn schedule(&mut self, component_id: usize, runnable: Box<dyn Runnable>) {
-            if self.tasks.insert(component_id, runnable).is_none() {
-                self.stack.push(component_id);
-            }
-        }
-
-        /// Drain all tasks into `dst`, if any
-        pub fn drain_into(&mut self, dst: &mut Vec<Box<dyn Runnable>>) {
-            for id in self.stack.drain(..).rev() {
-                if let Some(t) = self.tasks.remove(&id) {
-                    dst.push(t);
-                }
-            }
-        }
-    }
 }
 
 #[cfg(feature = "csr")]
@@ -278,12 +190,26 @@ impl Scheduler {
         // Create events can be batched, as they are typically just for object creation
         to_run.append(&mut self.create);
 
+        // These typically do nothing and don't spawn any other events - can be batched.
+        // Should be run only after all first renders have finished.
+        if !to_run.is_empty() {
+            return;
+        }
+
         // First render must never be skipped and takes priority over main, because it may need
         // to init `NodeRef`s
         //
         // Should be processed one at time, because they can spawn more create and rendered events
         // for their children.
-        if let Some(r) = self.render_first.pop_front() {
+        //
+        // To be replaced with BTreeMap::pop_first once it is stable.
+        if let Some(r) = self
+            .render_first
+            .keys()
+            .next()
+            .cloned()
+            .and_then(|m| self.render_first.remove(&m))
+        {
             to_run.push(r);
         }
 
@@ -292,7 +218,12 @@ impl Scheduler {
         if !to_run.is_empty() {
             return;
         }
-        to_run.extend(self.rendered_first.drain(..).rev());
+
+        if !self.rendered_first.is_empty() {
+            let rendered_first = std::mem::take(&mut self.rendered_first);
+            // Children rendered lifecycle happen before parents.
+            to_run.extend(rendered_first.into_values().rev());
+        }
 
         // Updates are after the first render to ensure we always have the entire child tree
         // rendered, once an update is processed.
@@ -303,29 +234,37 @@ impl Scheduler {
         // Likely to cause duplicate renders via component updates, so placed before them
         to_run.append(&mut self.main);
 
-        #[cfg(any(feature = "ssr", feature = "csr"))]
+        // Run after all possible updates to avoid duplicate renders.
-        {
+        //
-            // Run after all possible updates to avoid duplicate renders.
+        // Should be processed one at time, because they can spawn more create and first render
-            //
+        // events for their children.
-            // Should be processed one at time, because they can spawn more create and first render
+        if !to_run.is_empty() {
-            // events for their children.
+            return;
-            if !to_run.is_empty() {
-                return;
-            }
-
-            if let Some(r) = self.render.pop() {
-                to_run.push(r);
-            }
         }
 
-        #[cfg(feature = "csr")]
+        // To be replaced with BTreeMap::pop_first once it is stable.
+        // Should be processed one at time, because they can spawn more create and rendered events
+        // for their children.
+        if let Some(r) = self
+            .render
+            .keys()
+            .next()
+            .cloned()
+            .and_then(|m| self.render.remove(&m))
         {
-            // These typically do nothing and don't spawn any other events - can be batched.
+            to_run.push(r);
-            // Should be run only after all renders have finished.
+        }
-            if !to_run.is_empty() {
+
-                return;
+        // These typically do nothing and don't spawn any other events - can be batched.
-            }
+        // Should be run only after all renders have finished.
-            self.rendered.drain_into(to_run);
+        if !to_run.is_empty() {
+            return;
+        }
+
+        if !self.rendered.is_empty() {
+            let rendered = std::mem::take(&mut self.rendered);
+            // Children rendered lifecycle happen before parents.
+            to_run.extend(rendered.into_values().rev());
         }
     }
 }

packages/yew/tests/failed_tests/base_component_impl-fail.stderr

@@ -1,12 +1,12 @@
 error[E0277]: the trait bound `Comp: yew::Component` is not satisfied
-  --> tests/failed_tests/base_component_impl-fail.rs:6:6
+   --> tests/failed_tests/base_component_impl-fail.rs:6:6
-   |
+    |
-6  | impl BaseComponent for Comp {
+6   | impl BaseComponent for Comp {
-   |      ^^^^^^^^^^^^^ the trait `yew::Component` is not implemented for `Comp`
+    |      ^^^^^^^^^^^^^ the trait `yew::Component` is not implemented for `Comp`
-   |
+    |
-   = note: required because of the requirements on the impl of `html::component::sealed::SealedBaseComponent` for `Comp`
+    = note: required because of the requirements on the impl of `html::component::sealed::SealedBaseComponent` for `Comp`
 note: required by a bound in `BaseComponent`
-  --> src/html/component/mod.rs
+   --> src/html/component/mod.rs
-   |
+    |
-   | pub trait BaseComponent: sealed::SealedBaseComponent + Sized + 'static {
+    | pub trait BaseComponent: sealed::SealedBaseComponent + Sized + 'static {
-   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `BaseComponent`
+    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `BaseComponent`