From 84cbbf183421235a7eb9c4c75098186929bba12e Mon Sep 17 00:00:00 2001 From: bakape Date: Sun, 12 Sep 2021 10:38:50 +0300 Subject: [PATCH 1/7] yew: partial render deduplication implementation --- packages/yew/src/html/component/lifecycle.rs | 289 +++++++++---------- packages/yew/src/html/component/mod.rs | 9 + packages/yew/src/html/component/scope.rs | 66 +++-- packages/yew/src/scheduler.rs | 213 +++++++++++--- 4 files changed, 343 insertions(+), 234 deletions(-) diff --git a/packages/yew/src/html/component/lifecycle.rs b/packages/yew/src/html/component/lifecycle.rs index f625e459db2..397395d5c52 100644 --- a/packages/yew/src/html/component/lifecycle.rs +++ b/packages/yew/src/html/component/lifecycle.rs @@ -15,8 +15,6 @@ pub(crate) struct ComponentState { next_sibling: NodeRef, node_ref: NodeRef, has_rendered: bool, - pending_root: Option, - pending_updates: Vec>, } impl ComponentState { @@ -37,39 +35,11 @@ impl ComponentState { next_sibling, node_ref, has_rendered: false, - pending_root: None, - pending_updates: Vec::new(), - } - } - - fn drain_pending_updates(&mut self, state: &Shared>>) { - if !self.pending_updates.is_empty() { - scheduler::push_component_updates(self.pending_updates.drain(..).map(|update| { - Box::new(ComponentRunnable { - state: state.clone(), - event: update.into(), - }) as Box - })); } } } -/// Internal Component lifecycle event -pub(crate) enum ComponentLifecycleEvent { - Create(CreateEvent), - Update(UpdateEvent), - Render, - Rendered, - Destroy, -} - -impl From> for ComponentLifecycleEvent { - fn from(create: CreateEvent) -> Self { - Self::Create(create) - } -} - -pub(crate) struct CreateEvent { +pub(crate) struct CreateRunner { pub(crate) parent: Element, pub(crate) next_sibling: NodeRef, pub(crate) placeholder: VNode, @@ -78,15 +48,23 @@ pub(crate) struct CreateEvent { pub(crate) scope: Scope, } -impl From> for ComponentLifecycleEvent { - fn from(update: UpdateEvent) -> Self { - Self::Update(update) +impl Runnable for CreateRunner { + fn run(self: Box) { + let mut current_state = self.scope.state.borrow_mut(); + if current_state.is_none() { + *current_state = Some(ComponentState::new( + self.parent, + self.next_sibling, + self.placeholder, + self.node_ref, + self.scope.clone(), + self.props, + )); + } } } pub(crate) enum UpdateEvent { - /// First update - First, /// Wraps messages for a component. Message(COMP::Message), /// Wraps batch of messages for a component. @@ -95,91 +73,93 @@ pub(crate) enum UpdateEvent { Properties(COMP::Properties, NodeRef, NodeRef), } -pub(crate) struct ComponentRunnable { +pub(crate) struct UpdateRunner { pub(crate) state: Shared>>, - pub(crate) event: ComponentLifecycleEvent, + pub(crate) event: UpdateEvent, } -impl Runnable for ComponentRunnable { +impl Runnable for UpdateRunner { fn run(self: Box) { - let mut current_state = self.state.borrow_mut(); - match self.event { - ComponentLifecycleEvent::Create(event) => { - if current_state.is_none() { - *current_state = Some(ComponentState::new( - event.parent, - event.next_sibling, - event.placeholder, - event.node_ref, - event.scope.clone(), - event.props, - )); - } - } - ComponentLifecycleEvent::Update(event) => { - if let Some(mut state) = current_state.as_mut() { - if state.pending_root.is_some() { - state.pending_updates.push(event); - return; - } - - let should_render = match event { - UpdateEvent::First => true, - UpdateEvent::Message(message) => state.component.update(message), - UpdateEvent::MessageBatch(messages) => { - let component = &mut state.component; - messages - .into_iter() - .fold(false, |acc, msg| component.update(msg) || acc) - } - UpdateEvent::Properties(props, node_ref, next_sibling) => { - // When components are updated, a new node ref could have been passed in - state.node_ref = node_ref; - // When components are updated, their siblings were likely also updated - state.next_sibling = next_sibling; - state.component.change(props) - } - }; - - if should_render { - state.pending_root = Some(state.component.view()); - state.scope.process(ComponentLifecycleEvent::Render); - }; - } - } - ComponentLifecycleEvent::Render => { - if let Some(state) = current_state.as_mut() { - if let Some(mut new_root) = state.pending_root.take() { - std::mem::swap(&mut new_root, &mut state.root_node); - let ancestor = Some(new_root); - let new_root = &mut state.root_node; - let scope = state.scope.clone().into(); - let next_sibling = state.next_sibling.clone(); - let node = new_root.apply(&scope, &state.parent, next_sibling, ancestor); - state.node_ref.link(node); - state.scope.process(ComponentLifecycleEvent::Rendered); - } + if let Some(mut state) = self.state.borrow_mut().as_mut() { + let schedule_render = match self.event { + UpdateEvent::Message(message) => state.component.update(message), + UpdateEvent::MessageBatch(messages) => { + let component = &mut state.component; + messages + .into_iter() + .fold(false, |acc, msg| component.update(msg) || acc) } - } - ComponentLifecycleEvent::Rendered => { - if let Some(mut state) = current_state.as_mut() { - let first_render = !state.has_rendered; - state.component.rendered(first_render); - state.has_rendered = true; - state.drain_pending_updates(&self.state); - } - } - ComponentLifecycleEvent::Destroy => { - if let Some(mut state) = current_state.take() { - state.component.destroy(); - state.root_node.detach(&state.parent); - state.node_ref.set(None); + UpdateEvent::Properties(props, node_ref, next_sibling) => { + // When components are updated, a new node ref could have been passed in + state.node_ref = node_ref; + // When components are updated, their siblings were likely also updated + state.next_sibling = next_sibling; + state.component.change(props) } + }; + if schedule_render { + scheduler::push_component_render( + self.state.as_ptr() as usize, + RenderRunner { + state: self.state.clone(), + }, + RenderedRunner { + state: self.state.clone(), + }, + ); + // Only run from the scheduler, so no need to call `scheduler::start()` } } } } +pub(crate) struct DestroyRunner { + pub(crate) state: Shared>>, +} + +impl Runnable for DestroyRunner { + fn run(self: Box) { + if let Some(mut state) = self.state.borrow_mut().take() { + state.component.destroy(); + state.root_node.detach(&state.parent); + state.node_ref.set(None); + } + } +} + +pub(crate) struct RenderRunner { + pub(crate) state: Shared>>, +} + +impl Runnable for RenderRunner { + fn run(self: Box) { + if let Some(state) = self.state.borrow_mut().as_mut() { + let mut new_root = state.component.view(); + std::mem::swap(&mut new_root, &mut state.root_node); + let ancestor = Some(new_root); + let new_root = &mut state.root_node; + let scope = state.scope.clone().into(); + let next_sibling = state.next_sibling.clone(); + let node = new_root.apply(&scope, &state.parent, next_sibling, ancestor); + state.node_ref.link(node); + } + } +} + +struct RenderedRunner { + pub(crate) state: Shared>>, +} + +impl Runnable for RenderedRunner { + fn run(self: Box) { + if let Some(state) = self.state.borrow_mut().as_mut() { + let first_render = !state.has_rendered; + state.component.rendered(first_render); + state.has_rendered = true; + } + } +} + #[cfg(test)] mod tests { extern crate self as yew; @@ -301,7 +281,7 @@ mod tests { } } - fn test_lifecycle(props: Props, expected: &[String]) { + fn test_lifecycle(props: Props, expected: &[&str]) { let document = crate::utils::document(); let scope = Scope::::new(None); let el = document.create_element("div").unwrap(); @@ -322,12 +302,7 @@ mod tests { lifecycle: lifecycle.clone(), ..Props::default() }, - &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - ], + &["create", "view", "child rendered", "rendered(true)"], ); test_lifecycle( @@ -338,11 +313,11 @@ mod tests { ..Props::default() }, &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - "update(false)".to_string(), + "create", + "view", + "child rendered", + "rendered(true)", + "update(false)", ], ); @@ -353,13 +328,13 @@ mod tests { ..Props::default() }, &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - "update(true)".to_string(), - "view".to_string(), - "rendered(false)".to_string(), + "create", + "view", + "child rendered", + "rendered(true)", + "update(true)", + "view", + "rendered(false)", ], ); @@ -370,11 +345,11 @@ mod tests { ..Props::default() }, &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - "update(false)".to_string(), + "create", + "view", + "child rendered", + "rendered(true)", + "update(false)", ], ); @@ -385,11 +360,11 @@ mod tests { ..Props::default() }, &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - "update(false)".to_string(), + "create", + "view", + "child rendered", + "rendered(true)", + "update(false)", ], ); @@ -400,13 +375,13 @@ mod tests { ..Props::default() }, &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - "update(true)".to_string(), - "view".to_string(), - "rendered(false)".to_string(), + "create", + "view", + "child rendered", + "rendered(true)", + "update(true)", + "view", + "rendered(false)", ], ); @@ -419,17 +394,19 @@ mod tests { ..Props::default() }, &[ - "create".to_string(), - "view".to_string(), - "child rendered".to_string(), - "rendered(true)".to_string(), - "update(true)".to_string(), - "view".to_string(), - "rendered(false)".to_string(), - "update(true)".to_string(), - "view".to_string(), - "rendered(false)".to_string(), + "create", + "view", + "child rendered", + "rendered(true)", + "update(true)", + "view", + "rendered(false)", + "update(true)", + "view", + "rendered(false)", ], ); + + // TODO: test render deduplication } } diff --git a/packages/yew/src/html/component/mod.rs b/packages/yew/src/html/component/mod.rs index 0c2dc3c2014..1249ce8fecf 100644 --- a/packages/yew/src/html/component/mod.rs +++ b/packages/yew/src/html/component/mod.rs @@ -87,10 +87,19 @@ pub trait Component: Sized + 'static { /// Components define their visual layout using a JSX-style syntax through the use of the /// `html!` procedural macro. The full guide to using the macro can be found in [Yew's /// documentation](https://yew.rs/concepts/html). + /// + /// Note that `view()` calls do not always follow a render request from `update()` or + /// `change()`. Yew may optimize some calls out to reduce virtual DOM tree generation overhead. + /// The `create()` call is always followed by a call to `view()`. fn view(&self) -> Html; /// The `rendered` method is called after each time a Component is rendered but /// before the browser updates the page. + /// + /// Note that `rendered()` calls do not always follow a render request from `update()` or + /// `change()`. Yew may optimize some calls out to reduce virtual DOM tree generation overhead. + /// The `create()` call is always followed by a call to `view()` and later `rendered()`. + /// /// ## Examples /// ```rust ///# use yew::{Html, Component, ComponentLink, html, ShouldRender}; diff --git a/packages/yew/src/html/component/scope.rs b/packages/yew/src/html/component/scope.rs index 1916f1a94d1..7aebf30765f 100644 --- a/packages/yew/src/html/component/scope.rs +++ b/packages/yew/src/html/component/scope.rs @@ -2,7 +2,7 @@ use super::{ lifecycle::{ - ComponentLifecycleEvent, ComponentRunnable, ComponentState, CreateEvent, UpdateEvent, + ComponentState, CreateRunner, DestroyRunner, RenderRunner, UpdateEvent, UpdateRunner, }, Component, }; @@ -115,14 +115,20 @@ impl Scoped for Scope { /// Process an event to destroy a component fn destroy(&mut self) { - self.process(ComponentLifecycleEvent::Destroy); + scheduler::push_component_destroy( + self.state.as_ptr() as usize, + DestroyRunner { + state: self.state.clone(), + }, + ); + scheduler::start(); } } /// A context which allows sending messages to a component. pub struct Scope { parent: Option>, - state: Shared>>, + pub(crate) state: Shared>>, } impl fmt::Debug for Scope { @@ -177,40 +183,36 @@ impl Scope { VNode::VRef(placeholder) }; - self.schedule(UpdateEvent::First.into()); - self.process(ComponentLifecycleEvent::Create(CreateEvent { - parent, - next_sibling, - placeholder, - node_ref, - props, - scope: self.clone(), - })); + scheduler::push_component_create( + CreateRunner { + parent, + next_sibling, + placeholder, + node_ref, + props, + scope: self.clone(), + }, + RenderRunner { + state: self.state.clone(), + }, + RenderRunner { + state: self.state.clone(), + }, + ); + // Not guaranteed to already have the scheduler started + scheduler::start(); } pub(crate) fn reuse(&self, props: COMP::Properties, node_ref: NodeRef, next_sibling: NodeRef) { - self.process(UpdateEvent::Properties(props, node_ref, next_sibling).into()); - } - - pub(crate) fn process(&self, event: ComponentLifecycleEvent) { - self.schedule(event); - scheduler::start(); + self.push_update(UpdateEvent::Properties(props, node_ref, next_sibling)); } - fn schedule(&self, event: ComponentLifecycleEvent) { - use ComponentLifecycleEvent::*; - - let push = match &event { - Create(_) => scheduler::push_component_create, - Update(_) => scheduler::push_component_update, - Render => scheduler::push_component_render, - Rendered => scheduler::push_component_rendered, - Destroy => scheduler::push_component_destroy, - }; - push(Box::new(ComponentRunnable { + fn push_update(&self, event: UpdateEvent) { + scheduler::push_component_update(UpdateRunner { state: self.state.clone(), event, - })); + }); + scheduler::start(); } /// Send a message to the component. @@ -221,7 +223,7 @@ impl Scope { where T: Into, { - self.process(UpdateEvent::Message(msg.into()).into()); + self.push_update(UpdateEvent::Message(msg.into())); } /// Send a batch of messages to the component. @@ -239,7 +241,7 @@ impl Scope { return; } - self.process(UpdateEvent::MessageBatch(messages).into()); + self.push_update(UpdateEvent::MessageBatch(messages)); } /// Creates a `Callback` which will send a message to the linked diff --git a/packages/yew/src/scheduler.rs b/packages/yew/src/scheduler.rs index f9729a0e5a7..732b4697184 100644 --- a/packages/yew/src/scheduler.rs +++ b/packages/yew/src/scheduler.rs @@ -1,20 +1,12 @@ //! This module contains a scheduler. use std::cell::RefCell; -use std::collections::VecDeque; +use std::collections::{hash_map::Entry, HashMap, VecDeque}; use std::rc::Rc; /// Alias for Rc> pub type Shared = Rc>; -thread_local! { - /// This is a global scheduler suitable to schedule and run any tasks. - /// - /// Exclusivity of mutable access is controlled by only accessing it through a set of public - /// functions. - static SCHEDULER: RefCell = Default::default(); -} - /// A routine which could be run. pub trait Runnable { /// Runs a routine with a context instance. @@ -29,22 +21,32 @@ struct Scheduler { main: VecDeque>, // Component queues - destroy: VecDeque>, + destroy: VecDeque<(usize, Box)>, create: VecDeque>, update: VecDeque>, - render: VecDeque>, + render_first: VecDeque>, + render: RenderScheduler, - // Stack - rendered: Vec>, + /// Deduplicating stacks to ensure child calls are always before parent calls + rendered_first: Vec>, + rendered: RenderedScheduler, } /// Execute closure with a mutable reference to the scheduler #[inline] -fn with(f: impl FnOnce(&mut Scheduler)) { - SCHEDULER.with(|s| f(&mut *s.borrow_mut())); +fn with(f: impl FnOnce(&mut Scheduler) -> R) -> R { + thread_local! { + /// This is a global scheduler suitable to schedule and run any tasks. + /// + /// Exclusivity of mutable access is controlled by only accessing it through a set of public + /// functions. + static SCHEDULER: RefCell = Default::default(); + } + + SCHEDULER.with(|s| f(&mut *s.borrow_mut())) } -/// Push a generic Runnable to be executed +/// Push a generic [Runnable] to be executed #[inline] pub fn push(runnable: Box) { with(|s| s.main.push_back(runnable)); @@ -53,43 +55,46 @@ pub fn push(runnable: Box) { start(); } -/// Push a component creation Runnable to be executed -#[inline] -pub(crate) fn push_component_create(runnable: Box) { - with(|s| s.create.push_back(runnable)); -} - -/// Push a component destruction Runnable to be executed -#[inline] -pub(crate) fn push_component_destroy(runnable: Box) { - with(|s| s.destroy.push_back(runnable)); -} - -/// Push a component render Runnable to be executed +/// Push a component creation, first render and rendered [Runnable]s to be executed #[inline] -pub(crate) fn push_component_render(runnable: Box) { - with(|s| s.render.push_back(runnable)); +pub(crate) fn push_component_create( + create: impl Runnable + 'static, + first_render: impl Runnable + 'static, + first_rendered: impl Runnable + 'static, +) { + with(|s| { + s.create.push_back(Box::new(create)); + s.render_first.push_back(Box::new(first_render)); + s.rendered_first.push(Box::new(first_rendered)); + }); } -/// Push a component Runnable to be executed after a component is rendered +/// Push a component destruction [Runnable] to be executed #[inline] -pub(crate) fn push_component_rendered(runnable: Box) { - with(|s| s.rendered.push(runnable)); +pub(crate) fn push_component_destroy(component_id: usize, runnable: impl Runnable + 'static) { + with(|s| s.destroy.push_back((component_id, Box::new(runnable)))); } -/// Push a component update Runnable to be executed +/// Push a component render and rendered [Runnable]s to be executed #[inline] -pub(crate) fn push_component_update(runnable: Box) { - with(|s| s.update.push_back(runnable)); +pub(crate) fn push_component_render( + component_id: usize, + render: impl Runnable + 'static, + rendered: impl Runnable + 'static, +) { + with(|s| { + s.render.schedule(component_id, Box::new(render)); + s.rendered.schedule(component_id, Box::new(rendered)); + }); } -/// Push a batch of component updates to be executed +/// Push a component update [Runnable] to be executed #[inline] -pub(crate) fn push_component_updates(it: impl IntoIterator>) { - with(|s| s.update.extend(it)); +pub(crate) fn push_component_update(runnable: impl Runnable + 'static) { + with(|s| s.update.push_back(Box::new(runnable))); } -/// Execute any pending Runnables +/// Execute any pending [Runnable]s pub(crate) fn start() { thread_local! { // The lock is used to prevent recursion. If the lock cannot be acquired, it is because the @@ -99,7 +104,7 @@ pub(crate) fn start() { LOCK.with(|l| { if let Ok(_lock) = l.try_borrow_mut() { - while let Some(runnable) = SCHEDULER.with(|s| s.borrow_mut().next_runnable()) { + while let Some(runnable) = with(|s| s.next_runnable()) { runnable.run(); } } @@ -109,13 +114,127 @@ pub(crate) fn start() { impl Scheduler { /// Pop next Runnable to be executed according to Runnable type execution priority fn next_runnable(&mut self) -> Option> { - self.destroy + // Placed first to avoid as much needless work as possible, handling all the other events + if let Some((id, runnable)) = self.destroy.pop_front() { + // Potentially avoids 2 scheduler cycles on removed components + self.render.unschedule(&id); + self.rendered.unschedule(&id); + + return Some(runnable); + } + + self.create .pop_front() - .or_else(|| self.create.pop_front()) + // First render must never be skipped and takes priority over main, because it may need + // to init `NodeRef`s + .or_else(|| self.render_first.pop_front()) + .or_else(|| self.rendered_first.pop()) + // Updates are after the first render to ensure we always have the entire child tree + // rendered, once an update is processed. .or_else(|| self.update.pop_front()) - .or_else(|| self.render.pop_front()) - .or_else(|| self.rendered.pop()) + // Likely to cause duplicate renders, so placed before them .or_else(|| self.main.pop_front()) + // Most expensive, as these call `Component::view()` + .or_else(|| self.render.pop()) + // `rendered()` should be run last, when we are sure there are no more renders, to + // reduce workload. + .or_else(|| self.rendered.pop()) + } +} + +/// 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, +} + +/// Scheduler for non-first component renders with deduplication +#[derive(Default)] +struct RenderScheduler { + /// Task registry by component ID + tasks: HashMap, + + /// Task queue by component ID + queue: VecDeque, +} + +impl RenderScheduler { + /// Schedule render task execution + fn schedule(&mut self, component_id: usize, runnable: Box) { + 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 + fn pop(&mut self) -> Option> { + 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 + } + + /// Invalidate all render tasks for a given component_id + fn unschedule(&mut self, component_id: &usize) { + self.tasks.remove(component_id); + } +} + +/// Scheduler for component rendered calls with deduplication +#[derive(Default)] +struct RenderedScheduler { + /// Task registry by component ID + tasks: HashMap>, + + /// Task stack by component ID + stack: Vec, +} + +impl RenderedScheduler { + /// Schedule rendered task execution + fn schedule(&mut self, component_id: usize, runnable: Box) { + if self.tasks.insert(component_id, runnable).is_none() { + self.stack.push(component_id); + } + } + + /// Try to pop a task from the stack, if any + fn pop(&mut self) -> Option> { + while let Some(id) = self.stack.pop() { + if let Some(runnable) = self.tasks.remove(&id) { + return Some(runnable); + } + } + None + } + + /// Invalidate all rendered tasks for a given component_id + fn unschedule(&mut self, component_id: &usize) { + self.tasks.remove(component_id); } } @@ -142,3 +261,5 @@ mod tests { FLAG.with(|v| assert!(v.get())); } } + +// TODO: 100% coverage for scheduler From 8271cf165774ebefbefe16475d4d1a26f7b6f205 Mon Sep 17 00:00:00 2001 From: bakape Date: Sun, 19 Sep 2021 16:55:38 +0300 Subject: [PATCH 2/7] yew/scheduler: batching and comment fixes --- packages/yew/src/html/component/mod.rs | 10 +- packages/yew/src/html/component/scope.rs | 11 +- packages/yew/src/scheduler.rs | 129 +++++++++++++---------- 3 files changed, 84 insertions(+), 66 deletions(-) diff --git a/packages/yew/src/html/component/mod.rs b/packages/yew/src/html/component/mod.rs index e55841dbfd3..6c405d4ee3b 100644 --- a/packages/yew/src/html/component/mod.rs +++ b/packages/yew/src/html/component/mod.rs @@ -76,17 +76,17 @@ pub trait Component: Sized + 'static { /// Components define their visual layout using a JSX-style syntax through the use of the /// `html!` procedural macro. The full guide to using the macro can be found in [Yew's /// documentation](https://yew.rs/concepts/html). + /// + /// Note that `view()` calls do not always follow a render request from `update()` or + /// `changed()`. Yew may optimize some calls out to reduce virtual DOM tree generation overhead. + /// The `create()` call is always followed by a call to `view()`. fn view(&self, ctx: &Context) -> Html; /// The `rendered` method is called after each time a Component is rendered but /// before the browser updates the page. /// - /// Note that `view()` calls do not always follow a render request from `update()` or - /// `change()`. Yew may optimize some calls out to reduce virtual DOM tree generation overhead. - /// The `create()` call is always followed by a call to `view()`. - /// /// Note that `rendered()` calls do not always follow a render request from `update()` or - /// `change()`. Yew may optimize some calls out to reduce virtual DOM tree generation overhead. + /// `changed()`. Yew may optimize some calls out to reduce virtual DOM tree generation overhead. /// The `create()` call is always followed by a call to `view()` and later `rendered()`. #[allow(unused_variables)] fn rendered(&mut self, ctx: &Context, first_render: bool) {} diff --git a/packages/yew/src/html/component/scope.rs b/packages/yew/src/html/component/scope.rs index fbdd569a2af..1cff14d8566 100644 --- a/packages/yew/src/html/component/scope.rs +++ b/packages/yew/src/html/component/scope.rs @@ -117,12 +117,10 @@ impl Scoped for Scope { /// Process an event to destroy a component fn destroy(&mut self) { - scheduler::push_component_destroy( - self.state.as_ptr() as usize, - DestroyRunner { - state: self.state.clone(), - }, - ); + scheduler::push_component_destroy(DestroyRunner { + state: self.state.clone(), + }); + // Not guaranteed to already have the scheduler started scheduler::start(); } } @@ -219,6 +217,7 @@ impl Scope { state: self.state.clone(), event, }); + // Not guaranteed to already have the scheduler started scheduler::start(); } diff --git a/packages/yew/src/scheduler.rs b/packages/yew/src/scheduler.rs index c25158b77cf..1d3bf0820ad 100644 --- a/packages/yew/src/scheduler.rs +++ b/packages/yew/src/scheduler.rs @@ -18,12 +18,12 @@ pub trait Runnable { #[allow(missing_debug_implementations)] // todo struct Scheduler { // Main queue - main: VecDeque>, + main: Vec>, // Component queues - destroy: VecDeque<(usize, Box)>, - create: VecDeque>, - update: VecDeque>, + destroy: Vec>, + create: Vec>, + update: Vec>, render_first: VecDeque>, render: RenderScheduler, @@ -48,7 +48,7 @@ fn with(f: impl FnOnce(&mut Scheduler) -> R) -> R { /// Push a generic [Runnable] to be executed pub fn push(runnable: Box) { - with(|s| s.main.push_back(runnable)); + with(|s| s.main.push(runnable)); // Execute pending immediately. Necessary for runnables added outside the component lifecycle, // which would otherwise be delayed. start(); @@ -61,15 +61,15 @@ pub(crate) fn push_component_create( first_rendered: impl Runnable + 'static, ) { with(|s| { - s.create.push_back(Box::new(create)); + s.create.push(Box::new(create)); s.render_first.push_back(Box::new(first_render)); s.rendered_first.push(Box::new(first_rendered)); }); } /// Push a component destruction [Runnable] to be executed -pub(crate) fn push_component_destroy(component_id: usize, runnable: impl Runnable + 'static) { - with(|s| s.destroy.push_back((component_id, Box::new(runnable)))); +pub(crate) fn push_component_destroy(runnable: impl Runnable + 'static) { + with(|s| s.destroy.push(Box::new(runnable))); } /// Push a component render and rendered [Runnable]s to be executed @@ -86,7 +86,7 @@ pub(crate) fn push_component_render( /// Push a component update [Runnable] to be executed pub(crate) fn push_component_update(runnable: impl Runnable + 'static) { - with(|s| s.update.push_back(Box::new(runnable))); + with(|s| s.update.push(Box::new(runnable))); } /// Execute any pending [Runnable]s @@ -99,41 +99,72 @@ pub(crate) fn start() { LOCK.with(|l| { if let Ok(_lock) = l.try_borrow_mut() { - while let Some(runnable) = with(|s| s.next_runnable()) { - runnable.run(); + let mut queue = vec![]; + loop { + with(|s| s.fill_queue(&mut queue)); + if queue.is_empty() { + break; + } + for r in queue.drain(..) { + r.run(); + } } } }); } impl Scheduler { - /// Pop next Runnable to be executed according to Runnable type execution priority - fn next_runnable(&mut self) -> Option> { - // Placed first to avoid as much needless work as possible, handling all the other events - if let Some((id, runnable)) = self.destroy.pop_front() { - // Potentially avoids 2 scheduler cycles on removed components - self.render.unschedule(&id); - self.rendered.unschedule(&id); - - return Some(runnable); + /// Fill vector with tasks to be executed according to Runnable type execution priority + /// + /// This method is optimized for typical usage, where possible, but does not break on + /// non-typical usage (like scheduling renders in [crate::Component::create()] or + /// [crate::Component::rendered()] calls). + fn fill_queue(&mut self, to_run: &mut Vec>) { + // Placed first to avoid as much needless work as possible, handling all the other events. + // Drained completely, because they are the highest priority events anyway. + to_run.extend(self.destroy.drain(..)); + + // Create events can be batched, as they are typically just for object creation + to_run.extend(self.create.drain(..)); + + // 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. + to_run.extend(self.render_first.pop_front()); + + // 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; + } + to_run.extend(self.rendered_first.drain(..).rev()); + + // Updates are after the first render to ensure we always have the entire child tree + // rendered, once an update is processed. + // + // Can be batched, as they can cause only non-first renders. + to_run.extend(self.update.drain(..)); + + // Likely to cause duplicate renders via component updates, so placed before them + to_run.extend(self.main.drain(..)); + + // 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. + if !to_run.is_empty() { + return; } + to_run.extend(self.render.pop()); - self.create - .pop_front() - // First render must never be skipped and takes priority over main, because it may need - // to init `NodeRef`s - .or_else(|| self.render_first.pop_front()) - .or_else(|| self.rendered_first.pop()) - // Updates are after the first render to ensure we always have the entire child tree - // rendered, once an update is processed. - .or_else(|| self.update.pop_front()) - // Likely to cause duplicate renders, so placed before them - .or_else(|| self.main.pop_front()) - // Most expensive, as these call `Component::view()` - .or_else(|| self.render.pop()) - // `rendered()` should be run last, when we are sure there are no more renders, to - // reduce workload. - .or_else(|| self.rendered.pop()) + // These typically do nothing and don't spawn any other events - can be batched. + // Should be run only after all renders have finished. + if !to_run.is_empty() { + return; + } + self.rendered.drain_into(to_run); } } @@ -192,14 +223,9 @@ impl RenderScheduler { } None } - - /// Invalidate all render tasks for a given component_id - fn unschedule(&mut self, component_id: &usize) { - self.tasks.remove(component_id); - } } -/// Scheduler for component rendered calls with deduplication +/// Deduplicating scheduler for component rendered calls with deduplication #[derive(Default)] struct RenderedScheduler { /// Task registry by component ID @@ -217,21 +243,14 @@ impl RenderedScheduler { } } - /// Try to pop a task from the stack, if any - fn pop(&mut self) -> Option> { - if let Some(id) = self.stack.pop() { - let t = self.tasks.remove(&id); - debug_assert!(t.is_some()); - t - } else { - None + /// Drain all tasks into `dst`, if any + fn drain_into(&mut self, dst: &mut Vec>) { + for id in self.stack.drain(..).rev() { + if let Some(t) = self.tasks.remove(&id) { + dst.push(t); + } } } - - /// Invalidate all rendered tasks for a given component_id - fn unschedule(&mut self, component_id: &usize) { - self.tasks.remove(component_id); - } } #[cfg(test)] From 83a6f1e43c9653b97a4282fcd698733ad0cd1487 Mon Sep 17 00:00:00 2001 From: bakape Date: Sun, 19 Sep 2021 17:14:47 +0300 Subject: [PATCH 3/7] yew/scheduler: clippy fixes --- packages/yew/src/scheduler.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/yew/src/scheduler.rs b/packages/yew/src/scheduler.rs index 1d3bf0820ad..ed5a6d2f5d4 100644 --- a/packages/yew/src/scheduler.rs +++ b/packages/yew/src/scheduler.rs @@ -122,17 +122,19 @@ impl Scheduler { fn fill_queue(&mut self, to_run: &mut Vec>) { // Placed first to avoid as much needless work as possible, handling all the other events. // Drained completely, because they are the highest priority events anyway. - to_run.extend(self.destroy.drain(..)); + to_run.append(&mut self.destroy); // Create events can be batched, as they are typically just for object creation - to_run.extend(self.create.drain(..)); + to_run.append(&mut self.create); // 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. - to_run.extend(self.render_first.pop_front()); + if let Some(r) = self.render_first.pop_front() { + to_run.push(r); + } // These typically do nothing and don't spawn any other events - can be batched. // Should be run only after all first renders have finished. @@ -145,10 +147,10 @@ impl Scheduler { // rendered, once an update is processed. // // Can be batched, as they can cause only non-first renders. - to_run.extend(self.update.drain(..)); + to_run.append(&mut self.update); // Likely to cause duplicate renders via component updates, so placed before them - to_run.extend(self.main.drain(..)); + to_run.append(&mut self.main); // Run after all possible updates to avoid duplicate renders. // @@ -157,7 +159,9 @@ impl Scheduler { if !to_run.is_empty() { return; } - to_run.extend(self.render.pop()); + if let Some(r) = self.render.pop() { + to_run.push(r); + } // These typically do nothing and don't spawn any other events - can be batched. // Should be run only after all renders have finished. From e04bbf8d12c599b18084b4ada7d6a04a2509d021 Mon Sep 17 00:00:00 2001 From: bakape Date: Tue, 21 Sep 2021 13:13:16 +0300 Subject: [PATCH 4/7] yew/vcomp: add lifecycle debug logging --- Makefile.toml | 21 +++++- packages/yew/src/html/component/lifecycle.rs | 32 +++++++++ packages/yew/src/html/component/scope.rs | 54 +++++++++++++-- packages/yew/src/virtual_dom/vcomp.rs | 70 +++++++++++++++++++- packages/yew/src/virtual_dom/vnode.rs | 11 ++- 5 files changed, 179 insertions(+), 9 deletions(-) diff --git a/Makefile.toml b/Makefile.toml index 74db7c4e26f..3802c404b7c 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -3,6 +3,7 @@ # public tasks: # * pr-flow # * lint +# * lint-release # * tests # * benchmarks # @@ -25,7 +26,7 @@ namespace = "core" toolchain = "stable" category = "Checks" description = "Lint and test" -run_task = { name = ["lint", "tests"], fork = true } +run_task = { name = ["lint", "lint-release", "tests"], fork = true } [tasks.lint] category = "Checks" @@ -50,6 +51,24 @@ private = true workspace = true dependencies = ["core::check-format-flow", "core::clippy-flow"] +# Needed, because we have some code differences between debug and release builds +[tasks.lint-release] +category = "Checks" +workspace = true +description = "Run cargo check and Clippy in release profile" +env = { CARGO_MAKE_CLIPPY_ARGS = "-- --deny=warnings --release" } +run_task = { name = ["check-release", "clippy-release"], fork = true } + +[tasks.check-release] +private = true +command = "cargo" +args = ["check", "--release"] + +[tasks.clippy-release] +private = true +command = "cargo" +args = ["clippy", "--release", "--", "--deny=warnings"] + [tasks.tests-setup] private = true script_runner = "@duckscript" diff --git a/packages/yew/src/html/component/lifecycle.rs b/packages/yew/src/html/component/lifecycle.rs index 1d8e39200f8..86a4267ae2f 100644 --- a/packages/yew/src/html/component/lifecycle.rs +++ b/packages/yew/src/html/component/lifecycle.rs @@ -16,6 +16,10 @@ pub(crate) struct ComponentState { next_sibling: NodeRef, node_ref: NodeRef, has_rendered: bool, + + // Used for debug logging + #[cfg(debug_assertions)] + pub(crate) vcomp_id: u64, } impl ComponentState { @@ -27,6 +31,12 @@ impl ComponentState { scope: Scope, props: Rc, ) -> Self { + #[cfg(debug_assertions)] + let vcomp_id = { + use super::Scoped; + + scope.to_any().vcomp_id + }; let context = Context { scope, props }; let component = Box::new(COMP::create(&context)); @@ -38,6 +48,9 @@ impl ComponentState { next_sibling, node_ref, has_rendered: false, + + #[cfg(debug_assertions)] + vcomp_id, } } } @@ -55,6 +68,9 @@ impl Runnable for CreateRunner { fn run(self: Box) { let mut current_state = self.scope.state.borrow_mut(); if current_state.is_none() { + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event(self.scope.vcomp_id, "create"); + *current_state = Some(ComponentState::new( self.parent, self.next_sibling, @@ -105,6 +121,13 @@ impl Runnable for UpdateRunner { } } }; + + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event( + state.vcomp_id, + format!("update(schedule_render={})", schedule_render), + ); + if schedule_render { scheduler::push_component_render( self.state.as_ptr() as usize, @@ -128,6 +151,9 @@ pub(crate) struct DestroyRunner { impl Runnable for DestroyRunner { fn run(self: Box) { if let Some(mut state) = self.state.borrow_mut().take() { + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event(state.vcomp_id, "destroy"); + state.component.destroy(&state.context); state.root_node.detach(&state.parent); state.node_ref.set(None); @@ -142,6 +168,9 @@ pub(crate) struct RenderRunner { impl Runnable for RenderRunner { fn run(self: Box) { if let Some(state) = self.state.borrow_mut().as_mut() { + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event(state.vcomp_id, "render"); + let mut new_root = state.component.view(&state.context); std::mem::swap(&mut new_root, &mut state.root_node); let ancestor = Some(new_root); @@ -161,6 +190,9 @@ pub(crate) struct RenderedRunner { impl Runnable for RenderedRunner { fn run(self: Box) { if let Some(state) = self.state.borrow_mut().as_mut() { + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event(state.vcomp_id, "rendered"); + let first_render = !state.has_rendered; state.component.rendered(&state.context, first_render); state.has_rendered = true; diff --git a/packages/yew/src/html/component/scope.rs b/packages/yew/src/html/component/scope.rs index 1cff14d8566..c491640d43f 100644 --- a/packages/yew/src/html/component/scope.rs +++ b/packages/yew/src/html/component/scope.rs @@ -28,6 +28,10 @@ pub struct AnyScope { type_id: TypeId, parent: Option>, state: Rc, + + // Used for debug logging + #[cfg(debug_assertions)] + pub(crate) vcomp_id: u64, } impl From> for AnyScope { @@ -36,6 +40,9 @@ impl From> for AnyScope { type_id: TypeId::of::(), parent: scope.parent, state: scope.state, + + #[cfg(debug_assertions)] + vcomp_id: scope.vcomp_id, } } } @@ -47,6 +54,9 @@ impl AnyScope { type_id: TypeId::of::<()>(), parent: None, state: Rc::new(()), + + #[cfg(debug_assertions)] + vcomp_id: 0, } } @@ -62,12 +72,24 @@ impl AnyScope { /// Attempts to downcast into a typed scope pub fn downcast(self) -> Scope { + let state = self + .state + .downcast::>>>() + .expect("unexpected component type"); + + #[cfg(debug_assertions)] + let vcomp_id = state + .borrow() + .as_ref() + .map(|s| s.vcomp_id) + .unwrap_or_default(); + Scope { parent: self.parent, - state: self - .state - .downcast::>>>() - .expect("unexpected component type"), + state, + + #[cfg(debug_assertions)] + vcomp_id, } } @@ -129,6 +151,10 @@ impl Scoped for Scope { pub struct Scope { parent: Option>, pub(crate) state: Shared>>, + + // Used for debug logging + #[cfg(debug_assertions)] + pub(crate) vcomp_id: u64, } impl fmt::Debug for Scope { @@ -142,6 +168,9 @@ impl Clone for Scope { Scope { parent: self.parent.clone(), state: self.state.clone(), + + #[cfg(debug_assertions)] + vcomp_id: self.vcomp_id, } } } @@ -165,7 +194,17 @@ impl Scope { pub(crate) fn new(parent: Option) -> Self { let parent = parent.map(Rc::new); let state = Rc::new(RefCell::new(None)); - Scope { parent, state } + + #[cfg(debug_assertions)] + let vcomp_id = parent.as_ref().map(|p| p.vcomp_id).unwrap_or_default(); + + Scope { + state, + parent, + + #[cfg(debug_assertions)] + vcomp_id, + } } /// Mounts a component with `props` to the specified `element` in the DOM. @@ -176,6 +215,8 @@ impl Scope { node_ref: NodeRef, props: Rc, ) { + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event(self.vcomp_id, "create placeholder"); let placeholder = { let placeholder: Node = document().create_text_node("").into(); insert_node(&placeholder, &parent, next_sibling.get().as_ref()); @@ -209,6 +250,9 @@ impl Scope { node_ref: NodeRef, next_sibling: NodeRef, ) { + #[cfg(debug_assertions)] + crate::virtual_dom::vcomp::log_event(self.vcomp_id, "reuse"); + self.push_update(UpdateEvent::Properties(props, node_ref, next_sibling)); } diff --git a/packages/yew/src/virtual_dom/vcomp.rs b/packages/yew/src/virtual_dom/vcomp.rs index 2a3102266f4..ac340bcbef5 100644 --- a/packages/yew/src/virtual_dom/vcomp.rs +++ b/packages/yew/src/virtual_dom/vcomp.rs @@ -9,6 +9,34 @@ use std::ops::Deref; use std::rc::Rc; use web_sys::Element; +thread_local! { + #[cfg(debug_assertions)] + static EVENT_HISTORY: std::cell::RefCell>> + = Default::default(); +} + +/// Push [VComp] event to lifecycle debugging registry +#[cfg(debug_assertions)] +pub(crate) fn log_event(vcomp_id: u64, event: impl ToString) { + EVENT_HISTORY.with(|h| { + h.borrow_mut() + .entry(vcomp_id) + .or_default() + .push(event.to_string()) + }); +} + +/// Get [VComp] event log from lifecycle debugging registry +#[cfg(debug_assertions)] +pub(crate) fn get_event_log(vcomp_id: u64) -> Vec { + EVENT_HISTORY.with(|h| { + h.borrow() + .get(&vcomp_id) + .map(|l| (*l).clone()) + .unwrap_or_default() + }) +} + /// A virtual component. pub struct VComp { type_id: TypeId, @@ -16,6 +44,10 @@ pub struct VComp { props: Option>, pub(crate) node_ref: NodeRef, pub(crate) key: Option, + + /// Used for debug logging + #[cfg(debug_assertions)] + pub(crate) id: u64, } impl Clone for VComp { @@ -24,12 +56,18 @@ impl Clone for VComp { panic!("Mounted components are not allowed to be cloned!"); } + #[cfg(debug_assertions)] + log_event(self.id, "clone"); + Self { type_id: self.type_id, scope: None, props: self.props.as_ref().map(|m| m.copy()), node_ref: self.node_ref.clone(), key: self.key.clone(), + + #[cfg(debug_assertions)] + id: self.id, } } } @@ -97,12 +135,40 @@ impl VComp { props: Some(Box::new(PropsWrapper::::new(props))), scope: None, key, + + #[cfg(debug_assertions)] + id: { + thread_local! { + static ID_COUNTER: std::cell::RefCell = Default::default(); + } + + ID_COUNTER.with(|c| { + let c = &mut *c.borrow_mut(); + *c += 1; + *c + }) + }, } } pub(crate) fn root_vnode(&self) -> Option + '_> { self.scope.as_ref().and_then(|scope| scope.root_vnode()) } + + /// Take ownership of [Box] or panic with error message, if component is not mounted + #[inline] + fn take_scope(&mut self) -> Box { + self.scope.take().unwrap_or_else(|| { + #[cfg(not(debug_assertions))] + panic!("no scope; VComp should be mounted"); + + #[cfg(debug_assertions)] + panic!( + "no scope; VComp should be mounted after: {:?}", + get_event_log(self.id) + ); + }) + } } trait Mountable { @@ -156,7 +222,7 @@ impl Mountable for PropsWrapper { impl VDiff for VComp { fn detach(&mut self, _parent: &Element) { - self.scope.take().expect("VComp is not mounted").destroy(); + self.take_scope().destroy(); } fn apply( @@ -173,7 +239,7 @@ impl VDiff for VComp { // If the ancestor is the same type, reuse it and update its properties if self.type_id == vcomp.type_id && self.key == vcomp.key { self.node_ref.reuse(vcomp.node_ref.clone()); - let scope = vcomp.scope.take().expect("VComp is not mounted"); + let scope = vcomp.take_scope(); mountable.reuse(self.node_ref.clone(), scope.borrow(), next_sibling); self.scope = Some(scope); return vcomp.node_ref.clone(); diff --git a/packages/yew/src/virtual_dom/vnode.rs b/packages/yew/src/virtual_dom/vnode.rs index 5a3ca1c53f5..49bbd7ca378 100644 --- a/packages/yew/src/virtual_dom/vnode.rs +++ b/packages/yew/src/virtual_dom/vnode.rs @@ -57,7 +57,16 @@ impl VNode { let text_node = vtext.reference.as_ref().expect("VText is not mounted"); text_node.clone().into() } - VNode::VComp(vcomp) => vcomp.node_ref.get().expect("VComp is not mounted"), + VNode::VComp(vcomp) => vcomp.node_ref.get().unwrap_or_else(|| { + #[cfg(not(debug_assertions))] + panic!("no node_ref; VComp should be mounted"); + + #[cfg(debug_assertions)] + panic!( + "no node_ref; VComp should be mounted after: {:?}", + crate::virtual_dom::vcomp::get_event_log(vcomp.id), + ); + }), VNode::VList(vlist) => vlist.get(0).expect("VList is not mounted").first_node(), VNode::VRef(node) => node.clone(), } From 3a80240d7081449dc7901f1fd34f04721c142f26 Mon Sep 17 00:00:00 2001 From: bakape Date: Tue, 21 Sep 2021 13:33:06 +0300 Subject: [PATCH 5/7] ci: add release clippy and cargo check passes --- .github/workflows/pull-request.yml | 60 +++++++++++++++++++++++++++++- Makefile.toml | 4 +- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index 83e3d716de4..fcba9fbb4b4 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -41,6 +41,64 @@ jobs: command: clippy args: --all-targets -- -D warnings + lint-release: + name: Clippy on release profile + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + profile: minimal + components: clippy + + - uses: actions/cache@v2 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: cargo-${{ runner.os }}-${{ github.job }}-${{ hashFiles('**/Cargo.toml') }} + restore-keys: | + cargo-${{ runner.os }}-${{ github.job }}- + + - name: Run clippy + if: always() + uses: actions-rs/cargo@v1 + with: + command: clippy + args: --all-targets --release -- -D warnings + + check-release: + name: Check release builds + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + profile: minimal + components: clippy + + - uses: actions/cache@v2 + with: + path: | + ~/.cargo/registry + ~/.cargo/git + target + key: cargo-${{ runner.os }}-${{ github.job }}-${{ hashFiles('**/Cargo.toml') }} + restore-keys: | + cargo-${{ runner.os }}-${{ github.job }}- + + - name: Run cargo check + if: always() + uses: actions-rs/cargo@v1 + with: + command: cargo + args: check --all-targets --release + check_examples: name: Check Examples runs-on: ubuntu-latest @@ -105,7 +163,7 @@ jobs: run: | cd packages/yew cargo test --doc --features "doc_test wasm_test" - + - name: Run website code snippet tests run: | cd packages/website-test diff --git a/Makefile.toml b/Makefile.toml index 3802c404b7c..dfddebb0399 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -62,12 +62,12 @@ run_task = { name = ["check-release", "clippy-release"], fork = true } [tasks.check-release] private = true command = "cargo" -args = ["check", "--release"] +args = ["check", "--all-targets", "--release"] [tasks.clippy-release] private = true command = "cargo" -args = ["clippy", "--release", "--", "--deny=warnings"] +args = ["clippy", "--all-targets", "--release", "--", "--deny=warnings"] [tasks.tests-setup] private = true From 776f7ee7842116b2097f989c862d57506b97a053 Mon Sep 17 00:00:00 2001 From: bakape Date: Tue, 21 Sep 2021 13:34:48 +0300 Subject: [PATCH 6/7] ci: fix release check --- .github/workflows/pull-request.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index fcba9fbb4b4..c1ebf4cd13f 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -96,8 +96,8 @@ jobs: if: always() uses: actions-rs/cargo@v1 with: - command: cargo - args: check --all-targets --release + command: check + args: --all-targets --release check_examples: name: Check Examples From e8ad50eb40d49beae33d3f9c2ac5fc68583d166b Mon Sep 17 00:00:00 2001 From: bakape Date: Wed, 22 Sep 2021 17:49:25 +0300 Subject: [PATCH 7/7] ci: remove duplicate check calls --- .github/workflows/pull-request.yml | 29 ----------------------------- Makefile.toml | 11 ----------- 2 files changed, 40 deletions(-) diff --git a/.github/workflows/pull-request.yml b/.github/workflows/pull-request.yml index c1ebf4cd13f..d22ef376e2d 100644 --- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -70,35 +70,6 @@ jobs: command: clippy args: --all-targets --release -- -D warnings - check-release: - name: Check release builds - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - uses: actions-rs/toolchain@v1 - with: - toolchain: stable - override: true - profile: minimal - components: clippy - - - uses: actions/cache@v2 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - target - key: cargo-${{ runner.os }}-${{ github.job }}-${{ hashFiles('**/Cargo.toml') }} - restore-keys: | - cargo-${{ runner.os }}-${{ github.job }}- - - - name: Run cargo check - if: always() - uses: actions-rs/cargo@v1 - with: - command: check - args: --all-targets --release - check_examples: name: Check Examples runs-on: ubuntu-latest diff --git a/Makefile.toml b/Makefile.toml index dfddebb0399..aad8ce1dd17 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -55,17 +55,6 @@ dependencies = ["core::check-format-flow", "core::clippy-flow"] [tasks.lint-release] category = "Checks" workspace = true -description = "Run cargo check and Clippy in release profile" -env = { CARGO_MAKE_CLIPPY_ARGS = "-- --deny=warnings --release" } -run_task = { name = ["check-release", "clippy-release"], fork = true } - -[tasks.check-release] -private = true -command = "cargo" -args = ["check", "--all-targets", "--release"] - -[tasks.clippy-release] -private = true command = "cargo" args = ["clippy", "--all-targets", "--release", "--", "--deny=warnings"]