Skip to content

Conversation

@bakape
Copy link
Contributor

@bakape bakape commented Sep 19, 2021

Description

  • Deduplicated non-first component render() and rendered() calls.
    • Deduplicated expensive view() calls - they have been moved to the render lifecycle event.
  • Reduced indirection and branching in component lifecycle event handlers.
  • Reduced amount of scheduler calls.
  • Decreased ComponentState memory footprint.
  • Optimized scheduler task priorities.
  • Added scheduler task batching, where possible.

Benchmark results:
Screenshot_20210919_165716

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have adjusted tests

@mc1098
Copy link
Contributor

mc1098 commented Sep 20, 2021

@intendednull 👋 RE: #1946 I wonder whether you could point your project to this change and see if the issue still persists? I've had a sneaking feeling that it might be due to the scheduler and some of the changes of priority here could possible prevent the issue. I don't have a way to replicate the issue on master which is why I'm asking 🙏. Thank you ❤️!

For me the PR isn't blocked on whether #1946 is fixed, but rather it would be nice to know if this has provided the fix so that the issue can be linked and @bakape can receive a well earned reward 🎉

@bakape
Copy link
Contributor Author

bakape commented Sep 20, 2021

If I understand the issued correctly, me moving the view() call from the update event to the render event would indeed solve #1946. Of course, testing is required to make sure.

@bakape
Copy link
Contributor Author

bakape commented Sep 20, 2021

As a side note, the gains in benchmarks are not really from render deduplication but other lifecycle and scheduler optimizations. The former only comes into play with more complex graph-like update flows.

@mc1098 mc1098 added the A-yew Area: The main yew crate label Sep 20, 2021
@intendednull
Copy link
Contributor

intendednull commented Sep 21, 2021

No luck, issue is present here as well. @mc1098 I think you're right about it being related to the scheduler. At the bottom of this stack trace we see the scheduler is running because an agent message was added (yewdux::service::ServiceBridge<H,SCOPE>::send_service::h1dcf1ea32f5e566), and due to #1954 changes, it executes immediately. So if there are already pending runnables, they'd be executed too. Is it possible a component lifecycle runnable is executing before it was designed to?

panicked at 'VComp is not mounted', /home/intendednull/.cargo/git/checkouts/yew-f43266d198c566fb/83a6f1e/packages/yew/src/virtual_dom/vnode.rs:60:57

Stack:

Error
    at imports.wbg.__wbg_new_59cb74e423758ede (http://localhost:8080/index-f90a2bac3f7d43b5.js:346:19)
    at console_error_panic_hook::Error::new::h35ae2c0762a88a49 (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[19855]:0x43bbd0)
    at console_error_panic_hook::hook_impl::h53e881a07904b706 (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[1807]:0x1d1820)
    at console_error_panic_hook::hook::h7a222228e698e443 (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[22371]:0x45dc37)
    at core::ops::function::Fn::call::hfd2f4b5c3386fe22 (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[18265]:0x422deb)
    at std::panicking::rust_panic_with_hook::h9852a65ac22b441a (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[4231]:0x283208)
    at std::panicking::begin_panic_handler::{{closure}}::hb262f7ea62636fdc (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[8706]:0x346d15)
    at std::sys_common::backtrace::__rust_end_short_backtrace::hbeb6eae20506157b (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[26176]:0x481f9f)
    at rust_begin_unwind (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[20785]:0x448e67)
    at core::panicking::panic_fmt::h1ba034fb58bf295c (http://localhost:8080/index-f90a2bac3f7d43b5_bg.wasm:wasm-function[23198]:0x467c97)

imports.wbg.__wbg_error_4bb6c2a97407129a	@	index-f90a2bac3f7d43b5.js:340
console_error_panic_hook::error::he85ebddb9bd41c88	@	index-f90a2bac3f7d43b5_bg.wasm:0x2fa694
console_error_panic_hook::hook_impl::h53e881a07904b706	@	index-f90a2bac3f7d43b5_bg.wasm:0x1d1919
console_error_panic_hook::hook::h7a222228e698e443	@	index-f90a2bac3f7d43b5_bg.wasm:0x45dc37
core::ops::function::Fn::call::hfd2f4b5c3386fe22	@	index-f90a2bac3f7d43b5_bg.wasm:0x422deb
std::panicking::rust_panic_with_hook::h9852a65ac22b441a	@	index-f90a2bac3f7d43b5_bg.wasm:0x283208
std::panicking::begin_panic_handler::{{closure}}::hb262f7ea62636fdc	@	index-f90a2bac3f7d43b5_bg.wasm:0x346d15
std::sys_common::backtrace::__rust_end_short_backtrace::hbeb6eae20506157b	@	index-f90a2bac3f7d43b5_bg.wasm:0x481f9f
rust_begin_unwind	@	index-f90a2bac3f7d43b5_bg.wasm:0x448e67
core::panicking::panic_fmt::h1ba034fb58bf295c	@	index-f90a2bac3f7d43b5_bg.wasm:0x467c97
core::panicking::panic_display::h0b0fc4d9da8b83e3	@	index-f90a2bac3f7d43b5_bg.wasm:0x41c72b
core::option::expect_failed::h83d8066bdd552df6	@	index-f90a2bac3f7d43b5_bg.wasm:0x474676
core::option::Option<T>::expect::h2a1b0b53dd3e3fa4	@	index-f90a2bac3f7d43b5_bg.wasm:0x3936fa
yew::virtual_dom::vnode::VNode::first_node::h4b6f6eec3263b00b	@	index-f90a2bac3f7d43b5_bg.wasm:0x192006
<yew::virtual_dom::vtag::VTag as yew::virtual_dom::VDiff>::apply::h9eb20bb013692b1c	@	index-f90a2bac3f7d43b5_bg.wasm:0x65d2c
<yew::virtual_dom::vnode::VNode as yew::virtual_dom::VDiff>::apply::h5fb253a66cdac875	@	index-f90a2bac3f7d43b5_bg.wasm:0x9915d
yew::virtual_dom::vlist::VList::apply_unkeyed::{{closure}}::h638eb67ab5c74778	@	index-f90a2bac3f7d43b5_bg.wasm:0x207193
yew::virtual_dom::vlist::VList::apply_unkeyed::h4d2b265535aa8e12	@	index-f90a2bac3f7d43b5_bg.wasm:0x8a4da
<yew::virtual_dom::vlist::VList as yew::virtual_dom::VDiff>::apply::h6e9cf17ee6429f81	@	index-f90a2bac3f7d43b5_bg.wasm:0x94616
<yew::virtual_dom::vtag::VTag as yew::virtual_dom::VDiff>::apply::h9eb20bb013692b1c	@	index-f90a2bac3f7d43b5_bg.wasm:0x666bc
<yew::virtual_dom::vnode::VNode as yew::virtual_dom::VDiff>::apply::h5fb253a66cdac875	@	index-f90a2bac3f7d43b5_bg.wasm:0x9915d
<yew::html::component::lifecycle::RenderRunner<COMP> as yew::scheduler::Runnable>::run::h7219dad7336ffd0d	@	index-f90a2bac3f7d43b5_bg.wasm:0xe23d9
yew::scheduler::start::{{closure}}::ha2dbf821f76d73f2	@	index-f90a2bac3f7d43b5_bg.wasm:0x12e74c
std::thread::local::LocalKey<T>::try_with::ha02e7f5aae6c29f7	@	index-f90a2bac3f7d43b5_bg.wasm:0x2b4fad
std::thread::local::LocalKey<T>::with::h29d21b704e850391	@	index-f90a2bac3f7d43b5_bg.wasm:0x3a93c5
yew::scheduler::start::hbc7816ab6197dbb3	@	index-f90a2bac3f7d43b5_bg.wasm:0x4821f7
yew::scheduler::push::h89c9c3fa97620461	@	index-f90a2bac3f7d43b5_bg.wasm:0x3d0071
yew_agent::link::AgentScope<AGN>::send::hefe0ebd30fafff8a	@	index-f90a2bac3f7d43b5_bg.wasm:0x2c9aca
<yew_agent::local::context::ContextBridge<AGN> as yew_agent::Bridge<AGN>>::send::h0be3730765c74a6e	@	index-f90a2bac3f7d43b5_bg.wasm:0x26bfaa
yewdux::service::ServiceBridge<H,SCOPE>::send_service::h1dcf1ea32f5e566

Am becoming more convinced my issue is a little different than originally posted in #1946.

@mc1098
Copy link
Contributor

mc1098 commented Sep 21, 2021

No luck, issue is present here as well.

Shame :) but thank you so much @intendednull for trying ❤️

Is it possible a component lifecycle runnable is executing before it was designed to?

That was my thinking, we probably should include some logging/tracing in debug so that we can actually see what lifecycle events are happening and when. This would be a different PR ofc.

Am becoming more convinced my issue is a little different than originally posted in #1946.

I'm not sure tbh 🙃

mc1098
mc1098 previously approved these changes Sep 21, 2021
Copy link
Contributor

@mc1098 mc1098 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me :)

I appreciate all the comments, especially for each step in fill_queue 👍

@bakape
Copy link
Contributor Author

bakape commented Sep 21, 2021 via email

@mergify mergify bot dismissed mc1098’s stale review September 21, 2021 10:13

Pull request has been modified.

@bakape
Copy link
Contributor Author

bakape commented Sep 21, 2021

@intendednull Try now! Panic message should be more helpful. Make sure to build it in debug mode!

@intendednull
Copy link
Contributor

@bakape okay ya! now I'm getting panicked at 'no node_ref; VComp should be mounted after: []

@bakape
Copy link
Contributor Author

bakape commented Sep 21, 2021 via email

mc1098
mc1098 previously approved these changes Sep 22, 2021
Copy link
Contributor

@mc1098 mc1098 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🎉
Thanks for adding event logging ❤️

@mergify mergify bot dismissed mc1098’s stale review September 22, 2021 15:00

Pull request has been modified.

@siku2 siku2 merged commit a442ce5 into yewstack:master Sep 22, 2021
@mc1098 mc1098 mentioned this pull request Sep 24, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-yew Area: The main yew crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants