matthewp / haunted

React's Hooks API implemented for web components 👻
BSD 2-Clause "Simplified" License
2.6k stars 92 forks source link

Something I am not quite getting in update schedules #33

Open askbeka opened 5 years ago

askbeka commented 5 years ago

I want to clarify motivation behind current implementation of updates in components. As I understand getting template result is done in RAF for batching then next RAF is scheduled for commit or DOM updates, then next RAF for effect hooks.

There are few things that I think might cause problems, but may be that I am not seeing the whole picture:

In my imagination all the prop updates need to be batched and then single run to read whole tree and only after one RAF to update whole tree, pretty much how I imagined React to work before fiber.

And I would prefer webcomponents to be rendering syncronously without scheduling since they are leaf nodes, unless there are some exceptional needs.

And virtual components, ones that wrap many of those leafs, need to have these scheduling mechanism, but they have to work in sync, don't schedule another RAF if you already scheduled one.

From docs about effects in React page, seems like RAF is the closes place to run them, after commit. So no questions about that:)

matthewp commented 5 years ago

Yeah, so like I said in that other thread not much thought was put into the scheduler. I just knew that I wanted reads and writes to be in their own tasks because that's been a good practice going back to fastdom. https://github.com/wilsonpage/fastdom

Then, later, I realized effects needed to happen a frame after commit. That's really all the thought that went into it.

I think what we should do is render the entire tree in one raf, then do all of the commits in the next, and then effects in another.

In the future ideally we can implement things like priorities. And make sure each read/write frame maintains 60fps.

I can work on this soon. Is this problem blocking your work on Context or just something you've noticed using Haunted?

askbeka commented 5 years ago

Thank you

No it is not blocking, just something I was curious about. I thought that I have missed on some news or insight in browser render optimisation:)

On Sat, 10 Nov 2018, 02:56 Matthew Phillips <notifications@github.com wrote:

Yeah, so like I said in that other thread not much thought was put into the scheduler. I just knew that I wanted reads and writes to be in their own tasks because that's been a good practice going back to fastdom. https://github.com/wilsonpage/fastdom

Then, later, I realized effects needed to happen a frame after commit. That's really all the thought that went into it.

I think what we should do is render the entire tree in one raf, then do all of the commits in the next, and then effects in another.

In the future ideally we can implement things like priorities. And make sure each read/write frame maintains 60fps.

I can work on this soon. Is this problem blocking your work on Context or just something you've noticed using Haunted?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/matthewp/haunted/issues/33#issuecomment-437549041, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3ImmG9K_fjel9D8oTP-euUtHBS10eyks5utjIxgaJpZM4YXsdt .

blikblum commented 5 years ago

Regarding usage of requestAnimationFrame (raf), is there any of the concerns pointed in this article or in this older React raf library relevant here?

I did some tests with inputs and did not notice anything wrong.

Maybe it would be worth ask @polymer/lit-element folks why they use microtasks instead of raf to do async render.

askbeka commented 5 years ago

Thats maybe because browser vendors moved input updates into render cycle after raf. Polymer uses microtaks for batching prop updates. There is no mechanism to synchronise components. Eventloop runs microtasks untill none left unlike tasks blicking the eventloop from doing anything else, this way render is batched too.

On Sat, 10 Nov 2018, 12:32 Luiz Américo <notifications@github.com wrote:

Regarding usage of requestAnimationFrame (raf), is there any of the concerns pointed in this article https://github.com/evancz/react-angular-ember-elm-performance-comparison/blob/master/readme.md or in this older React raf library https://github.com/petehunt/react-raf-batching/issues/8 relevant here?

I did some tests with inputs and did not notice anything wrong.

Maybe it would be worth ask @polymer/lit-element folks why they use microtasks instead of raf to do async render.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/matthewp/haunted/issues/33#issuecomment-437577473, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3Imnhi835JVE0p2TKEzCl-7ZHlH3sXks5utrk4gaJpZM4YXsdt .

jpray commented 5 years ago

I'd be up for constructing some performance and "making sure things happen in an expected way" tests. @askbeka, any guidance on what a good test case would include to show the potential downsides of the current approach?

jpray commented 5 years ago

I constructed a pen that illustrates the issue: https://codesandbox.io/s/1z53k10vj7

jpray commented 5 years ago

I tried swapping out the RAF for a microtask and it did make everything render in a single frame, but when working with a crazy big number of children and mutations there is, of course, a tradeoff with FPS dropping below 60. Is it an all or nothing decision, or is there a possible hybrid approach?

jpray commented 5 years ago

Probably switching it to a microtask is the way to go. DBMonster benchmark still performs well with that. I'll upload a couple perf test pages today or tomorrow to demonstrate...

jpray commented 5 years ago

https://jpray.github.io/haunted-tests/

@matthewp, what do you think?

askbeka commented 5 years ago

One thing to note. Users should use promise from webcomponents polyfill and do not load their own. Since webcomponents polyfill adds mutation observer for older browsers instead of default behaviour of settimeout. https://github.com/webcomponents/webcomponentsjs/blob/master/src/promise.js

On Sun, 17 Feb 2019, 13:24 John Daniel Pray, notifications@github.com wrote:

https://jpray.github.io/haunted-tests/

@matthewp https://github.com/matthewp, what do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/matthewp/haunted/issues/33#issuecomment-464450530, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3Imq4ZYRHxWZkPFdEv3awShGAT790Lks5vOUnigaJpZM4YXsdt .

matthewp commented 5 years ago

@jpray you are amazing. Thanks for creating these perf comparisons. I think we should possibly move on this. Do you have a commit for this change? I assume you are doing it similar to preact?

One problem I see is that in the Promise.resolve version it stops rendering while scrolling. That makes sense because Promise.resolve is essentially synchronous but with the ability for prioritized events (like scrolling) to interject. So it can only go while scrolling is not happening.

@developit, if you're listening, how do you handle in Preact rendering being stopped during scrolling?

@jpray I wonder if ideally we could switch rendering modes some how.. use rAF during priority events but Promise.resolve() when the browser is "idle". For now it's probably safe to make the switch though, since you've already done the work.

jpray commented 5 years ago

Opened https://github.com/matthewp/haunted/pull/67 with preact microtask implementation (Thanks Preact!)

@matthewp, I wasn't able to produce what you reported about updates not happening when scrolling. The closest thing I could see was that on the nested children example, it takes >1 sec to render the page so if you scroll down quickly you'll get a jagged feeling as the next child pops in every second rAF. Do you have different steps to reproduce?

I chewed on switching between a microtask and a rAF based on some criteria, but couldn't come up with anything clean.

One other related question I had while looking at the code was the thinking behind the commit phase and the effects using the same scheduler instance (write), but the update phase has its own instance (read)? https://github.com/matthewp/haunted/blob/master/src/core.js#L47 I am not proposing any changes here, just trying to get my mind around it. Thanks...