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

[Umbrella] Async rendering #8830

Closed
acdlite opened this issue Jan 19, 2017 · 7 comments
Closed

[Umbrella] Async rendering #8830

acdlite opened this issue Jan 19, 2017 · 7 comments

Comments

@acdlite
Copy link
Member

@acdlite acdlite commented Jan 19, 2017

Async rendering is incomplete. There are bugs with the existing implementation and crucial features that are missing.

Specifically, the bugs are related to resuming work after it has been interupted.

#9695 was an effort to clean up the bugs without fundamentally changing the underlying model. In the course of working on that branch, we've decided that the underlying model is inherently flawed and needs to change. The tricky case is when low priority work is interrupted by a higher priority update. We want to be able to reconcile again at the higher priority without losing the low priority children, so that we can resume them later. There's no way to do this with the existing model.

So we're going to scrap the model and start again.

Scrap existing "progressed work" implementation and its bugs

This will give us a better foundation upon which to build the new model. It should also fix bugs in the triangle demo, although starvation will clearly be worse. We should aim for correctness before comprehensiveness.

  • Add fuzz tester to protect against regressions. [@acdlite] (#9952)
    • Difficult to impossible to write unit tests that provide sufficient coverage, especially ones that are resilient to implementation changes. A fuzz tester provides more safety.
    • Should not make assertions on how work is reused, only on consistency.
  • Remove all existing code related to "progressed" or "forked" work. [@acdlite] (#10008)
  • Split pendingWorkPriority from the update priority so that it represents the priority of the subtree, but not the fiber it belongs to. This lets us know whether the children have remaining work.

Expiration times

The next step will be to implement expiration times so that low priority work doesn't. It's possible that expiration times alone are sufficient to generate real product wins, even without the ability to resume interrupted work.

  • Implement expiration times [@acdlite] (#10426)
    • Updates to the same fiber at the same priority level should coalesce (commit all at once).
    • "Bucket" updates by rounding expiration times. This may be sufficient the solve to coalescing problem.

Async top-level API

Keep track of next unit of work per root

  • Is this possible? What about context?
  • scheduleUpdate (setState) currently does not always reach the root, so when we receive an update on a fiber, we don't necessarily know which tree the fiber belongs to.

Flush interaction work synchronously

Expiration boundaries and blockers (shouldComponentBlock)

Still figuring out the details for how this will work

  • Components can block rendering using shouldComponentBlock(props, state)
    • When a component blocks, React searches for the nearest expiration boundary. Has similar semantics to error boundaries.
    • If/when the update expires, switches to a renderExpired tree instead.
    • Unblock using this.ping (actual name TK).

Resuming interrupted work

Then we can move onto to addressing the problem of resuming interrupted work.

  • Implement resuming in the basic case, where the fiber was not touched since the last time we worked on it.
    • progressedPriority should represent the priority that the parent last reconciled at.
  • child is a set of all children, present and future (and maybe some in the past)?
    • Still figuring out the details.
    • How will re-orders work?

Other items

  • Resume mount bug: null is passed as props to constructor [@acdlite] (#9576)
  • Resume mount bug: creating a new instance on resume causes refs (or callbacks in user-space) to close over the wrong instance. Fix by reusing the original instance. [@acdlite] (#9608)
  • Image load event may fire before it's been mounted into the DOM. Figure out a way to defer this event (and other applicable ones) until after mount.
  • Ensure error boundaries work in incremental mode, e.g. in a hidden subtree.
  • Don't commit during requestIdleCallback. Wait until the next animation frame, then flush animation work using the last completed priority level. If the work overlaps, we may be able to reuse it. (This item may need to wait until we switch to expiration times.)
  • Ensure work expires even if there are no rIC or rAF events.
  • Solve stale event listeners: When a component receives an interaction event, flush updates in parents and "simulate" a render to recreate event handler before calling it.
  • Defer event dispatching in a proper and fast way. See #9742 and #9580 for context.
  • Solve the case where you want to show some fallback content only if the primary content takes too long but not if it is fast. Such as rendering a spinner if an async render takes too long. What if when the general expiration time elapsed for an async tree, we started calling an alternate renderExpired tree instead? That way we can render the tree with a spinner, only if it took too long to render. This is Suspense.
  • Figure out story around unit-testing async components. We probably just want to force sync mode. What API should we provide to Enzyme/TestUtils to do this?
@sebmarkbage sebmarkbage mentioned this issue Jan 19, 2017
130 of 155 tasks complete
@leoasis
Copy link

@leoasis leoasis commented Jan 20, 2017

Exciting list! When I read https://developers.google.com/web/updates/2015/08/using-requestidlecallback a paragraph caught especially my attention regarding how to use requestIdleCallback and requestAnimationFrame:

The best practice is to only make DOM changes inside of a requestAnimationFrame callback, since it is scheduled by the browser with that type of work in mind. That means that our code will need to use a document fragment, which can then be appended in the next requestAnimationFrame callback. If you are using a VDOM library, you would use requestIdleCallback to make changes, but you would apply the DOM patches in the next requestAnimationFrame callback, not the idle callback.

And I was hoping to see if that idea was planned to be potentially included in React if you had the same findings in your measurements. Seeing the third bullet in the bigger items list just answered my question 😄

@acdlite
Copy link
Member Author

@acdlite acdlite commented Feb 22, 2017

@amjoshi91 Friendly heads up: that item is outdated and isn't quite right.

Generally, the items in this issue fairly complex (even the smaller ones) and will be hard for an outside contributor to complete. It will be easier to contribute once more of the core pieces are in place. We'll do our best to identify those opportunities as they come up. :)

EDIT: Comment I responded to was deleted ¯\_(ツ)_/¯

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Feb 22, 2017

Does async work poorly if you don't have shouldComponentUpdate everywhere because the reuse mechanism isn't as good? Maybe the other mechanisms cover it.

@acdlite
Copy link
Member Author

@acdlite acdlite commented Feb 22, 2017

@sebmarkbage I was thinking that, too. It's interesting because the accepted best practice today is to not use shouldComponentUpdate until you really need it, because the time it takes to compare all the props could be wasteful. But that trade-off might adjust once we have time-slicing.

@acdlite acdlite changed the title [Fiber] Core algorithm improvements [Fiber] Umbrella for issues related to time-slicing (async scheduling) Mar 6, 2017
@acdlite acdlite changed the title [Fiber] Umbrella for issues related to time-slicing (async scheduling) [Umbrella] Async rendering Jun 13, 2017
@JobLeonard
Copy link

@JobLeonard JobLeonard commented Sep 22, 2017

Hey, I have a maybe dumb question about the polyfill for requestIdleCallback and I think this is the most appropriate place to ask.

Looking at the source for ReactDOMFrameScheduling, I see that you are using a polyfill that is limited to the scope of ReactDOMFrameScheduling, and is more-or-less optimised for it.

My worry is: if I use a polyfill for requestIdleCallback in my own code, could that interfere with the test in line 56, leading to a sub-optimal polyfill being used by React?

} else if (typeof requestIdleCallback !== 'function') {

In short: is there anything I should think about when using a polyfill for requestIdleCallback for my own code?

@sebmarkbage
Copy link
Member

@sebmarkbage sebmarkbage commented Sep 22, 2017

@JobLeonard Good observation. Yes. Most requestIdleCallback polyfills are way too conservative. Meaning they don't fire often enough. requestIdleCallback should fill up all the available CPU cycles between frames and React will rely on that. If you use a conservative polyfill, you'll not get enough callbacks and your application performance will suffer as a result. This only matters in async mode. We might want to have a DEV only detection to see if there's a non-native polyfill and warn about that.

@acdlite Do we have an item on here to ensure that setTimeout is used for expiration if requestIdleCallback never fires?

@gaearon
Copy link
Member

@gaearon gaearon commented Nov 28, 2017

More recent issue #11566

@NE-SmallTown NE-SmallTown mentioned this issue Jul 24, 2018
4 of 22 tasks complete
@acdlite acdlite closed this Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants