[Fiber] Separate priority for updates #8538

acdlite · 2016-12-09T05:49:57Z

Fixes a few issues with the way updates currently work.

Right now we reset the work-in-progress's update queue during the complete phase, which means updates that occur during render or in a child's begin phase (constructor, cWM, cWRP, render) are completely dropped. This isn't an ideal pattern but Stack supports it and it's fairly common. The lack of support in Fiber today is causing a nasty infinite loop bug for some components at Facebook.

To fix this, the update queue maintains a pointer to the first pending update. When an update is used during reconciliation, the pointer is set to null to indicate that the entire queue has been processed. If new updates come in before the component is committed, the pointer points to the first new update. Then in the commit phase, the processed updates are dropped, but the pending updates are kept in the queue.

Another problem is that we use the same priority field for both props and updates, and when we reset the priority field during the complete phase, we don't have a way to read the priority of the pending updates. In the first pass, I'll add a priority field to the update queue to solve this. What we really want, though, is for each individual update to have its own priority, so that when we render a component, we only process the updates that match the current priority level.

Don't clear pending updates from the queue.
Move scheduling of update/callback side-effects to the begin phase.
Add priority field to the update queue so that pending priority isn't dropped.

May do in a follow-up; not needed to fix the infinite loop bug, but necessary to get the Fiber triangle demo working:

Sort the queue by priority and only render updates that match the current priority.

acdlite · 2016-12-13T18:26:01Z

@sebmarkbage This is ready for review now. I updated it using the algorithm we discussed yesterday.

sebmarkbage · 2016-12-14T01:37:06Z

examples/fiber/index.html

+        render() {
+          var props = this.props;
+          var s = props.size * 1.3;
+          var idx = props.idx;


Seems like dead code.

sebmarkbage · 2016-12-14T01:37:53Z

examples/fiber/index.html

+          };
+          return (
+            <div style={style} onMouseEnter={() => this.enter()} onMouseLeave={() => this.leave()}>
+              {this.state.hover ? 'X' : props.text}


It would actually be nice to use to use something from props here since that shows that we're able to reuse previous props for a new condition. Maybe '"' + props.text + '"' or something?

sebmarkbage · 2016-12-14T01:42:29Z

src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js

+
+    ReactNoop.render(<Foo />);
+    ReactNoop.flush();
+    expect(Object.keys(state)).toEqual(['a', 'b', 'c']);


We shouldn't rely on object iteration order. This has failed us in the past, and in fact, we have talked about enforcing that all keys are always defined to enforce that the state object has a consistent hidden class.

Can you rewrite this to test that there are actually two renders and that the first one only have part of the state and the second one has all three?

This same goes for all the other tests in this file that use the same hack.

Dang and I thought this was so clever too :D

sebmarkbage · 2016-12-14T01:46:58Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+    scheduleUpdateAtPriority(fiber, TaskPriority);
+  }
+
+  function scheduleSetState(fiber : Fiber, partialState : any) {


Can we fine a way to move all of these out of the scheduler? These are super specific to the class API and as we add new APIs we don't want to bloat the scheduler which already has too many unrelated concepts.

I had them outside at first but it was awkward because it required exposing access the current priority context, which seems leaky. Either the class module needs to be aware of the priority context or the scheduler needs to be aware of the update queue.

sebmarkbage · 2016-12-14T01:49:09Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -460,7 +488,11 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C
      ReactFiberInstrumentation.debugTool.onWillBeginWork(workInProgress);
    }
    // See if beginning this work spawns more work.
+
+    priorityContextBeforeReconciliation = priorityContext;


This is in the hot path. Any reason this can't be outside the while loop?

Hmm well it needs to change whenever nextPriorityLevel changes. Maybe in findNextUnitOfWork? I'll see.

sebmarkbage · 2016-12-14T01:49:27Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -479,10 +511,13 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C

    ReactCurrentOwner.current = null;


While you're at it, this also doesn't belong in the hot path for the same reason.

Will address in separate PR

sebmarkbage · 2016-12-14T01:52:23Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -355,6 +376,13 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C

  function resetWorkPriority(workInProgress : Fiber) {
    let newPriority = NoWork;
+
+    // Check for pending update priority


Maybe add a comment that we usually expect this to be null so this shouldn't be a perf issue.

acdlite · 2016-12-14T04:19:34Z

@sebmarkbage Updating the example now but I believe I addressed the rest of your feedback

sebmarkbage · 2016-12-14T06:02:46Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+      workInProgress.effectTag |= Update;
+    } else {
+      // If an update was already in progress, we should schedule an Update
+      // effect even though we're bailing out, so that cWU/cDU are called.


This doesn't affect CWU. Will need to come back to this to ensure I understand it.

sebmarkbage

There's a lot to go through here. Will have to continue tomorrow. It's a skill but it might be worth while working on techniques for breaking these PRs apart. Even if you have to build on top of the base refactor, it would be quicker to review the base refactor if you excluded things like the new dev warnings, and the solutions to componentDidUpdate/refs/etc. We're still behind a flag but we're getting to a point where we need to keep the code base pretty stable so that others don't get blocked. So whatever you can do to minimize the risk of a PR is helpful even if it slows you down since it might be the thing that avoids someone being blocked on a bug - which helps parallelism.

acdlite · 2016-12-14T15:47:39Z

Yup that's fair. I'll work on that in the future. Happy to pull things out to help the main bit land. Usually I've tried to avoid any extra stuff unless it's needed to get the tests to pass, or to fix a new bug that surfaced, but some of these commits can indeed be moved to separate PRs.

sebmarkbage · 2016-12-14T20:57:14Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+    return 0;
+  }
+  if (a === NoWork && b !== NoWork) {
+    return -Infinity;


We should stick to integer numbers instead of floats (Infinity is a floating point value) if possible so that we can optimize it as such.

Np that value is immediately reset anyway

Oh nvm I thought this was a different line (on my phone). Okay I'll just use like 999 or something :D

sebmarkbage · 2016-12-14T21:52:02Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

@@ -26,6 +26,8 @@ const {
  TaskPriority,
 } = require('ReactPriorityLevel');

+const { MAX_SAFE_INTEGER } = Number;


just use 255 :P

MAX_SAFE_INTEGER will also trigger the float type in the VM because it's higher than 32 or 31 bit representations which ever is the one used.

We need to be able to access both, and since the list uses forward pointers, it makes more sense to point to the one that comes first. Otherwise to get the last progressed update you have to start at the beginning of the list.

The queue maintains a pointer to the last progressed update in the list. Updates that come after that pointer are pending. The pointer is set to the end of the list during reconciliation. Pending updates are sorted by priority then insertion. Progressed updates are sorted by the order in which they were applied during reconciliation, which may not be by priority: if a component bails out before the updates are committed, in the next render, the progressed updates are applied in the same order that they were previously, even if a higher priority update comes in. Once a progressed update is flushed/committed, it's removed from the queue.

This is needed to get the triangle demo working.

Instead clear updates on the work-in-progress during the begin phase. Aborted updates are recovered by cloning from the current fiber.

This is the most common case, so we should avoid scanning the entire list to get to the end.

The update is scheduled as if the current processing update has already been processed; if it has the same or higher priority, it will be flushed in the same batch. We also print a warning.

setState inside render/cWRP should have the same priority as whatever level is currently being reconciled.

We can just check if the deadline has expired.

...rather than changing it on every unit of work.

Infinity is a floating point value.

acdlite · 2016-12-15T17:14:36Z

Just rebased, didn't change anything

sebmarkbage · 2016-12-15T21:08:19Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+  const queue2 = fiber.alternate ? ensureUpdateQueue(fiber.alternate) : null;
+
+  // Warn if an update is scheduled from inside an updater function.
+  if (__DEV__ && typeof methodName === 'string' && (queue1.isProcessing || (queue2 && queue2.isProcessing))) {


Can we make this it's own if block? The console.errors should also use the warning module.

It used to be that our dead code elimination didn't work on more conditionals. It might now.

Regardless, I'd like to know for sure that this gets stripped and that's easier to determine with a single block. If the parenthesis are wrong somewhere, and you get a || in the condition then the whole thing and all its dependencies won't be stripped.

sebmarkbage · 2016-12-15T21:09:54Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+  let insertBefore1;
+  let insertAfter2;
+  let insertBefore2;
+  for (let i = 0; queue && i < 2; i++) {


I find the intention of this loop really confusing since it is meant to terminate after only two iterations. Seems it would be easier to just explode it but I'll try to read it as is first.

It was exploded but then whenever I made changes I kept forgetting to update both versions. Maybe that's not a big deal once the code is stable, though.

acdlite · 2016-12-15T21:14:22Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+    }
+    if (update.callback) {
+      if (callbackList && callbackList.last) {
+        callbackList.last.next = update;


Oops I forgot to clone this update before adding it to the callback list

sebmarkbage · 2016-12-15T21:43:51Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+      // time we commit.
+      update2 = cloneUpdate(update1);
+    }
+    insertUpdateIntoQueue(queue2, update2, insertAfter2, insertBefore2);


insertAfter.next = update is the thing to worry about here. I think that either it is the same and we only set it twice or they're always different and copies but in different queue or one of them is null. It would be nice to colocate this logic somehow so that this assumption can be reasoned about locally. Right now you have to ensure that both of these functions line up and understand that this assumption exists.

Ok I'll remove insertUpdateIntoQueue and inline the logic and see if that helps

Or maybe I'll keep insertUpdateIntoQueue but won't use it for the case where the update isn't cloned, and instead handle that case manually.

Only reason that function exists is for first and last, it's really easy to forget to update those.

sebmarkbage · 2016-12-15T21:59:14Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+// However, if incoming update is inserted into the same position of both lists,
+// we shouldn't make a copy.
+
+function insertUpdate(fiber : Fiber, update : Update, methodName : ?string) : void {


It's not great that we have different method signatures for dev and prod and we also don't want to pass this in prod. If we're luck it just gets inlined and dropped. I don't have a better proposal. Just flagging it.

I suppose you could move the error out of this function. It's a pretty simple conditional to replicate to the methods (setState / replaceState / forceUpdate). That would be nicer.

Could perform the check separately inside addUpdate, addCallback, et al.

acdlite · 2016-12-15T22:03:47Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+      // use the original `prevState`, not the accumulated `state`
+      state = getStateFromUpdate(update, instance, prevState, props);
+      dontMutatePrevState = true;
+      isEmpty = false;


Hmm I don't think this isEmpty thing is necessary

sebmarkbage · 2016-12-15T22:06:07Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+    let partialState;
+    if (update.isReplace) {
+      // A replace should drop all previous updates in the queue, so
+      // use the original `prevState`, not the accumulated `state`


Why is any previous state needed? Why is prevState better than state? Seems arbitrary.

Naming is bad. state is the accumulated state. prevState is the initial accumulation.

Roughly what this code is doing is:

// initialState is the state before applying any updates const nextState = updates.reduce((accumulatedState, update) => { if (update.isReplace) { return update.partialState; } else { return merge(accumulatedState, update.partialState); } }, initialState);

By applying the update to the initial state rather than the accumulation, it effectively drops the previous updates. But... this actually isn't right anyway because we should only drop updates with the same priority. We already do this in addReplaceState. So this comment is wrong and we should always pass state (the accumulation).

sebmarkbage · 2016-12-15T23:48:38Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

-    node = node.next;
+
+  workInProgress.callbackList = callbackList;
+  workInProgress.memoizedState = state;


I have concerned because we don't set memoizedProps here. If we abort and then resume we might end up relying on the wrong memoizedState but I don't see any bugs yet and we probably want to move to a model where memoizedProps is also set in begin.

I think this is detritus... something I tried out but isn't actually necessary.

I'll remove

sebmarkbage · 2016-12-15T23:51:45Z

src/renderers/shared/fiber/ReactTypeOfSideEffect.js

+  Placement: 1,                               // 0b0000001
+  Update: 2,                                  // 0b0000010
+  PlacementAndUpdate: 3,                      // 0b0000011
+  ForceUpdate: 4,                             // 0b0000100


Let's move this back to the update queue.

acdlite · 2016-12-15T23:58:17Z

Review feedback:

Replace console.error with warning module.
Remove ForceUpdate effect and put the flag back on the queue.
Don't need to track isEmpty
Don't drop updates from the queue in replaceState and always pass the accumulated state.
Inline insertUpdateIntoQueue, maybe. Make the logic easier to follow when an incoming update is cloned versus when it is not.
Don't update memoizedState in begin phase. Later, we'll move all memoization from complete to begin.
Get rid of weird for-loops. Either explode or use a helper function so it can be inlined.
Clone update before transferring to callbackList

sebmarkbage

I have some concerns about the code style that we should address in follow ups but I think this works.

Let's just undo the effect tag thing and then we can land.

acdlite · 2016-12-16T00:04:48Z

Okay I'll remove the effect tag thing and then do the rest of the follow-up in a separate PR.

Go back to using a flag, instead. I removed it before because I thought we might want to get rid of the top-level UpdateQueue type and put the fields directly on the fiber, but since we're keeping UpdateQueue we can put hasForceUpdate on there.

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 9, 2016

acdlite force-pushed the fiberupdatequeue branch 3 times, most recently from e5c09de to 3f2d671 Compare December 9, 2016 17:54

acdlite changed the title ~~[Fiber][WIP] UpdateQueue improvements~~ [Fiber][WIP] Separate priority for updates Dec 9, 2016

acdlite force-pushed the fiberupdatequeue branch 3 times, most recently from fdfadf1 to ea76d23 Compare December 9, 2016 23:08

acdlite changed the title ~~[Fiber][WIP] Separate priority for updates~~ [Fiber] Separate priority for updates Dec 9, 2016

acdlite requested a review from sebmarkbage December 9, 2016 23:11

acdlite force-pushed the fiberupdatequeue branch 5 times, most recently from d2917af to f82a1b7 Compare December 11, 2016 21:16

sebmarkbage reviewed Dec 14, 2016

View reviewed changes

acdlite added semver-major and removed semver-major labels Dec 14, 2016

acdlite force-pushed the fiberupdatequeue branch from f977e6e to e455853 Compare December 14, 2016 04:13

sebmarkbage reviewed Dec 14, 2016

View reviewed changes

acdlite force-pushed the fiberupdatequeue branch 2 times, most recently from b63cb7a to 0878d04 Compare December 14, 2016 19:23

sebmarkbage reviewed Dec 14, 2016

View reviewed changes

acdlite force-pushed the fiberupdatequeue branch from 5717e20 to b17c30c Compare December 14, 2016 21:56

acdlite added 11 commits December 15, 2016 09:12

Add unstable_deferredUpdates

ead8ab7

This is needed to get the triangle demo working.

Don't rely on commit phase effect to clear updates

d7f89b8

Instead clear updates on the work-in-progress during the begin phase. Aborted updates are recovered by cloning from the current fiber.

Add fast path for appending updates to the end of the queue

babace0

This is the most common case, so we should avoid scanning the entire list to get to the end.

Handle setState inside an updater function

e0981b8

The update is scheduled as if the current processing update has already been processed; if it has the same or higher priority, it will be flushed in the same batch. We also print a warning.

Priority context during reconciliation

eec4312

setState inside render/cWRP should have the same priority as whatever level is currently being reconciled.

Simplify test for whether we should flush a pending commit

df9603e

We can just check if the deadline has expired.

Move priority context change to findNextUnitOfWork

e1b733f

...rather than changing it on every unit of work.

Update tests to not rely on key iteration order

496512f

Use 255 instead of infinity

3a946fe

Infinity is a floating point value.

acdlite force-pushed the fiberupdatequeue branch from b17c30c to 3a946fe Compare December 15, 2016 17:14

sebmarkbage reviewed Dec 15, 2016

View reviewed changes

acdlite commented Dec 15, 2016

View reviewed changes

sebmarkbage reviewed Dec 15, 2016

View reviewed changes

acdlite commented Dec 15, 2016

View reviewed changes

sebmarkbage reviewed Dec 15, 2016

View reviewed changes

acdlite mentioned this pull request Dec 15, 2016

[Fiber] Queue top-level updates #8584

Merged

sebmarkbage reviewed Dec 15, 2016

View reviewed changes

acdlite mentioned this pull request Dec 15, 2016

Schedule state and callback at the same time #8585

Merged

sebmarkbage approved these changes Dec 16, 2016

View reviewed changes

Remove ForceUpdate effect

b8441ba

Go back to using a flag, instead. I removed it before because I thought we might want to get rid of the top-level UpdateQueue type and put the fields directly on the fiber, but since we're keeping UpdateQueue we can put hasForceUpdate on there.

acdlite merged commit 7fca464 into facebook:master Dec 16, 2016

acdlite mentioned this pull request Dec 16, 2016

[Fiber] UpdateQueue follow-up improvements #8587

Merged

8 tasks

		@@ -479,10 +511,13 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C

		ReactCurrentOwner.current = null;

[Fiber] Separate priority for updates #8538

[Fiber] Separate priority for updates #8538

Conversation

acdlite commented Dec 9, 2016 • edited Loading

acdlite commented Dec 13, 2016

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

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 Dec 14, 2016

Choose a reason for hiding this comment

sebmarkbage left a comment • edited Loading

Choose a reason for hiding this comment

acdlite commented Dec 14, 2016

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 Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Dec 15, 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

Choose a reason for hiding this comment

sebmarkbage Dec 15, 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

acdlite Dec 15, 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 Dec 15, 2016 • edited Loading

sebmarkbage left a comment

Choose a reason for hiding this comment

acdlite commented Dec 16, 2016

acdlite commented Dec 9, 2016 •

edited

Loading

sebmarkbage left a comment •

edited

Loading

acdlite Dec 15, 2016 •

edited

Loading

sebmarkbage Dec 15, 2016 •

edited

Loading

acdlite Dec 15, 2016 •

edited

Loading

acdlite commented Dec 15, 2016 •

edited

Loading