Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Something I am not quite getting in update schedules #33

Open
askbeka opened this issue Nov 9, 2018 · 12 comments
Open

Something I am not quite getting in update schedules #33

askbeka opened this issue Nov 9, 2018 · 12 comments
Labels
discussion Generic discussion topics

Comments

@askbeka
Copy link
Contributor

askbeka commented Nov 9, 2018

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:

  • read and commit could happen in the same phase, since we already got the results in previous RAF but haven't painted anything. Is it for splitting up the work, aren't microtasks better for batching?
  • in commit phase which is scheduled in second spin of render loop, DOM is updated and so do props of components that will start their own update schedule, so for each nesting 2 frames are needed to render one change.For example, passing down the prop from top component that has 30 levels of nested components, that also change as a result of property changes it means we get 60 frames to render whole tree which might take 1 second at best:)

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
Copy link
Owner

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
Copy link
Contributor Author

askbeka commented Nov 10, 2018 via email

@blikblum
Copy link

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
Copy link
Contributor Author

askbeka commented Nov 10, 2018 via email

@jpray
Copy link
Collaborator

jpray commented Feb 15, 2019

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
Copy link
Collaborator

jpray commented Feb 16, 2019

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

@jpray
Copy link
Collaborator

jpray commented Feb 16, 2019

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
Copy link
Collaborator

jpray commented Feb 16, 2019

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
Copy link
Collaborator

jpray commented Feb 17, 2019

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

@matthewp, what do you think?

@askbeka
Copy link
Contributor Author

askbeka commented Feb 17, 2019 via email

@matthewp
Copy link
Owner

@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
Copy link
Collaborator

jpray commented Feb 18, 2019

Opened #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...

@matthewp matthewp added the discussion Generic discussion topics label Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Generic discussion topics
Projects
None yet
Development

No branches or pull requests

4 participants