[Fiber] Use synchronous scheduling by default #8127

acdlite · 2016-10-27T19:09:06Z

~~Work in progress~~ Ready for review

Implement synchronous work
Add host config to opt into synchronous scheduling
Batch nested updates
Turn on sync scheduling in ReactDOMFiber
Figure out which tests are failing as a result, and why
Expose unstable_batchedUpdates API

acdlite · 2016-10-28T00:51:52Z

Just to be clear, I'm still working on this. Don't quite know what I'm doing yet :D

sebmarkbage · 2016-10-28T06:07:30Z

src/renderers/noop/ReactNoop.js

@@ -210,6 +211,10 @@ var ReactNoop = {
    NoopRenderer.performWithPriority(AnimationPriority, fn);
  },

+  setDefaultPriority(priorityLevel: PriorityLevel) {
+    NoopRenderer.setDefaultPriority(priorityLevel);
+  },


Can we just make this low pri by default in ReactNoop and synchronous in ReactDOMFiber? So we don't have to expose this as an API?

acdlite · 2016-10-28T18:33:55Z

Rebased on top of Dan's error boundary work

gaearon · 2016-10-28T22:46:50Z

It would be great if you could add a test in ErrorBoundaries-test for setState inside batchedUpdates, like this:

batchedUpdates(() => {
  component.setState({ ... });
  throw something;
});

It needs to still perform the work despite throwing.

Not related to error boundaries per se but we have all tests about errors there now so makes sense to add it there.

gaearon · 2016-10-28T23:42:07Z

Work in progress

It will be current once you commit!
(I just can't resist terrible Fiber puns.)

acdlite · 2016-10-29T05:20:12Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -47,19 +46,19 @@ module.exports = function(scheduleUpdate : (fiber: Fiber, priorityLevel : Priori
      const updateQueue = fiber.updateQueue ?
        addToQueue(fiber.updateQueue, partialState) :
        createUpdateQueue(partialState);
-      scheduleUpdateQueue(fiber, updateQueue, LowPriority);
+      scheduleUpdateQueue(fiber, updateQueue);


I think I found the problem... If scheduleUpdateQueue is sync (and not batched) then the update is flushed before enqueueCallback is ever called. Then enqueueCallback will add a callback to the queue but it won't ever get called until the next time the fiber is committed.

gaearon · 2016-10-29T10:30:26Z

src/renderers/shared/fiber/ReactFiberScheduler.js

-  // The default priority to use for updates.
-  let defaultPriority : PriorityLevel = LowPriority;
+  // The priority level to use when scheduling an update.
+  let priorityContext : (PriorityLevel | null) = null;


AFAIK having a variable switch between null and a number prevents some optimizations in V8. Can we use something like -1 for this?

You could stil keep the distinction in type system if you don't make it a part of the PriorityContext union and explicitly say PriorityContext | NoPriorityContext. Or something like it.

Good call. I think I'll just get rid of defaultPriorityContext entirely. I added it when I thought we'd need a way to change the default on demand, but now that it's determined by the host config it's not necessary.

gaearon · 2016-10-29T10:36:35Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+
+  function performAndHandleErrors<A>(fn: (a: A) => void, a: A) {
+    try {
+      fn(a);


Do we try to avoid first class functions unless we're dealing with browser or user code? I'm not sure, just curious what @sebmarkbage wants.

Hmm idk. Doesn't seem much different than passing performAnimationWork to requestAnimationFrame.

And it shouldn't lead to deep stacks because nested sync updates are batched

Ideally we should avoid first-class functions because I'd like everything to be able to take advantage of inline expansion - and be easily portable to C. It also helps newcomers to follow code that you don't have to jump back and forth. Not sure if we'll even use first-class functions for scheduling in React Native because we're not tied to rAF/rIC. Might just move that part to the DOM renderer.

It'd probably be better to go in the opposite direction and unify deferred/animation work and simply branch in performWork before the loop.

gaearon · 2016-10-29T10:38:26Z

src/renderers/shared/stack/reconciler/__tests__/ReactCompositeComponentState-test.js

+      ];
+    } else {
+      // There's a bug in the stack reconciler where setState callbacks inside
+      // componentWillMount aren't flushed properly


Oh nice, we are fixing this?

Yes we should fix this in the stack reconciler. I'll open an issue for it once I understand it better. (Not very familiar with how stack works.) @sebmarkbage has more details.

The extent of my knowledge is: under certain circumstances in componentWillMount, enqueued callbacks aren't flushed, so they stick around until some other update causes them to flush.

acdlite · 2016-10-31T05:48:45Z

Ready for review!

acdlite · 2016-10-31T06:00:34Z

@gaearon I'll look at the error boundary stuff once we get #8153 merged.

sebmarkbage · 2016-10-31T17:35:29Z

src/isomorphic/classic/class/ReactClass.js

@@ -728,6 +728,11 @@ var ReactClassMixin = {
   * type signature and the only use case for this, is to avoid that.
   */
  replaceState: function(newState, callback) {
+    if (this.updater.isFiberUpdater) {


Can you explain why Fiber needs to be special here and take a callback argument instead of a separate enqueueCallback call?

If we're going down that route, we should just do the same for the stack reconciler instead.

If we're changing Component internals may be better to do this before 15.4.0 which semi-separates classic stuff from reconciler.

Took me a while to figure out. If the updater schedules them separately, and the update is synchronous, then the work is already done by the time the callback is scheduled, so the callback doesn't get called until the next flush. This is what I was trying to describe on Friday that is similar to the componentWillMount bug we found.

In incremental mode, the flow goes like this:

(in updater) Schedule update

(in updater) Schedule callback

Perform work

In sync mode, it goes like this

(in updater) Schedule update

Because the update is synchronous, the work is performed immediately

(in updater) Schedule callback

I'm actually not sure how Stack deals with this, come to think of it, but this seemed like the most straightforward solution.

I think the way stack works is that enqueueCallback enqueues a second synchronous flush, which doesn't really do anything except calling the callback.

Why does this strategy that Stack uses (two separate synchronous reconciliations) work for Fiber? My worry would be that this is covering up another bug.

There was a bug because enqueueUpdate only adding the callback to the queue; it didn't schedule an update on the fiber. This was unobservable previously because the callback was added to the same update queue node as the state update.

I can get it to work by adding that to enqueueUpdate, but it felt wrong that you need an extra flush for a callback that is conceptually linked to the update.

Oops, meant enqueueCallback. On my phone so can't edit.

I split them out in Stack because they look like they are conceptually linked but they're actually not since the callback isn't fired after a particular state update but after all the state updates. It's an API designed without consideration for batching.

At some point I considered making the enqueueCallback API public instead of the argument to setState.

We only have to do an extra flush now because they're synchronous but as soon as we get rid of that it won't cause an extra flush. They'all always be batched.

We shouldn't even need to traverse the parents in schedule update since we've already scheduled this node's priority.

sebmarkbage · 2016-10-31T17:41:11Z

src/renderers/shared/stack/reconciler/__tests__/ReactCompositeComponentState-test.js

-      ['componentWillUnmount', 'blue'],
-    ].join('\n'));
+    let expected;
+    if (ReactDOMFeatureFlags.useFiber) {


Can we localize this branch to the two places that differ? ...(ReactDOMFeatureFlags.useFiber ? ['foo'] : []) and ...(!ReactDOMFeatureFlags.useFiber ? ['foo'] : [])

It is hard to follow what the difference is here.

acdlite · 2016-10-31T18:20:04Z

@sebmarkbage Changed performAndHandleErrors to branch on the priority level rather than use a first-class function

sebmarkbage · 2016-11-02T08:31:27Z

Accepting pending the comments I mentioned above.

gaearon · 2016-11-02T10:21:56Z

Thanks a lot folks!

acdlite · 2016-11-02T17:49:36Z

Merging once the build passes

gaearon · 2016-11-02T17:51:16Z

scripts/fiber/tests-passing.txt

@@ -692,6 +691,7 @@ src/renderers/dom/stack/client/__tests__/ReactMount-test.js
 * should warn if mounting into dirty rendered markup
 * should not warn if mounting into non-empty node
 * should warn when mounting into document.body
+* passes the correct callback context


Just learned about this a week ago. This is the weirdest part of React API 😄

gaearon · 2016-11-02T17:54:59Z

sebmarkbage · 2016-11-02T18:55:13Z

So, there is a use case for the existing behavior of the callback. It allows you to do work after your parent's componentDidMount. Which is what we do for things like controlled components. It'll be interesting to see if this new behavior will break something critical.

gaearon · 2016-11-02T19:34:14Z

I fixed up lint. Test probably failed due to my mistake in master which I have fixed there.

acdlite

Thanks :) at lunch and that was driving me crazy

I don't think this actually changes any behavior because of the way findNextUnitOfWork works, but I think this is easier to follow.

A bit of restructuring so that setState uses whatever the current priority context is, like top-level render already does. Renamed defaultPriority to priorityContext, and added a new variable called defaultPriorityContext. Having two separate variables allows the default to be changed without interfering with the current context.

I'll turn this on in ReactDOMFiber once I figure out batchedUpdates.

Almost working...

Without this fix, in non-batched mode, the update is scheduled first and synchronously flushed before the callback is added to the queue. The callback isn't called until the next flush.

Implements batchedUpdates and exposes as unstable_batchedUpdates. All nested synchronous updates are automatically batched.

Turns out this isn't necessary. Simpler to keep it as one field.

Instead we'll branch on the priority level, like in scheduleWork.

Covers some bugs I encountered while working on this feature. Introduces a syncUpdates API to ReactNoop. Is this something we'd want to expose to the reconciler?

This solves the problem I had with enqueueSetState and enqueueCallback being separate.

facebook-github-bot added the GH Review: review-needed label Oct 27, 2016

acdlite changed the title ~~[Fiber ] Use synchronous scheduling by default~~ [Fiber] Use synchronous scheduling by default Oct 27, 2016

sebmarkbage mentioned this pull request Oct 27, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

facebook-github-bot added the CLA Signed label Oct 27, 2016

gaearon mentioned this pull request Oct 27, 2016

[Fiber] Full Error Boundaries #8095

Merged

acdlite assigned sebmarkbage Oct 27, 2016

sebmarkbage reviewed Oct 28, 2016

View reviewed changes

acdlite force-pushed the fibersynchronouswork branch from 73ddf40 to e4a160b Compare October 28, 2016 18:33

acdlite force-pushed the fibersynchronouswork branch 3 times, most recently from b065e71 to 55235cb Compare October 28, 2016 22:16

acdlite force-pushed the fibersynchronouswork branch 3 times, most recently from 6b01166 to ffc832e Compare October 29, 2016 04:24

acdlite commented Oct 29, 2016

View reviewed changes

acdlite force-pushed the fibersynchronouswork branch from 8a5c59d to dfe8378 Compare October 29, 2016 06:03

gaearon reviewed Oct 29, 2016

View reviewed changes

acdlite force-pushed the fibersynchronouswork branch 2 times, most recently from 2994868 to b44da09 Compare October 31, 2016 05:35

sebmarkbage reviewed Oct 31, 2016

View reviewed changes

gaearon mentioned this pull request Oct 31, 2016

[Fiber] Remove recursion from error handling #8153

Closed

gaearon mentioned this pull request Nov 2, 2016

[Fiber] Error handling #8181

Closed

4 tasks

acdlite force-pushed the fibersynchronouswork branch 2 times, most recently from 91401b2 to d3ee9c6 Compare November 2, 2016 17:46

gaearon reviewed Nov 2, 2016

View reviewed changes

acdlite commented Nov 2, 2016

View reviewed changes

acdlite added 13 commits November 2, 2016 12:40

Ensure first iteration of performAnimationWork loop has right priority

ab92097

I don't think this actually changes any behavior because of the way findNextUnitOfWork works, but I think this is easier to follow.

Synchronous work

a07f5ee

Add config to enable sync scheduling by default

78aef38

I'll turn this on in ReactDOMFiber once I figure out batchedUpdates.

Batch nested updates when in sync mode

ecf468e

Almost working...

Enqueue update and callback simultaneously

d153b1e

Without this fix, in non-batched mode, the update is scheduled first and synchronously flushed before the callback is added to the queue. The callback isn't called until the next flush.

unstable_batchedUpdates

b41883f

Implements batchedUpdates and exposes as unstable_batchedUpdates. All nested synchronous updates are automatically batched.

Don't need defaultPriorityContext

2e5e88c

Turns out this isn't necessary. Simpler to keep it as one field.

unstable_batchedUpdates should return value of callback

26731ea

At the end of a batch, perform any sync work that was scheduled

3121e05

Remove first-class function

da45010

Instead we'll branch on the priority level, like in scheduleWork.

batchedUpdates tests

2182b69

Covers some bugs I encountered while working on this feature. Introduces a syncUpdates API to ReactNoop. Is this something we'd want to expose to the reconciler?

New effect type: Callback

c3ab541

This solves the problem I had with enqueueSetState and enqueueCallback being separate.

acdlite force-pushed the fibersynchronouswork branch from 9b9378f to 40fec61 Compare November 2, 2016 19:41

Fix lint by removing unneeded type import

95084be

acdlite force-pushed the fibersynchronouswork branch from 40fec61 to 95084be Compare November 2, 2016 19:45

acdlite merged commit 75f4ab2 into master Nov 2, 2016

gaearon deleted the fibersynchronouswork branch November 2, 2016 19:58

acdlite mentioned this pull request Nov 3, 2016

[Fiber] "Task" priority for error boundaries and batched updates #8193

Merged

[Fiber] Use synchronous scheduling by default #8127

[Fiber] Use synchronous scheduling by default #8127

Conversation

acdlite commented Oct 27, 2016 • edited Loading

acdlite commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Oct 28, 2016

gaearon commented Oct 28, 2016

gaearon commented Oct 28, 2016

acdlite Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Oct 31, 2016

acdlite commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Oct 31, 2016

sebmarkbage commented Nov 2, 2016

gaearon commented Nov 2, 2016

acdlite commented Nov 2, 2016

Choose a reason for hiding this comment

gaearon commented Nov 2, 2016

sebmarkbage commented Nov 2, 2016 • edited Loading

gaearon commented Nov 2, 2016

acdlite left a comment

Choose a reason for hiding this comment

acdlite commented Oct 27, 2016 •

edited

Loading

acdlite Oct 29, 2016 •

edited

Loading

acdlite Oct 29, 2016 •

edited

Loading

sebmarkbage Oct 31, 2016 •

edited

Loading

sebmarkbage commented Nov 2, 2016 •

edited

Loading