Refactor progressed work model #9695

acdlite · 2017-05-16T03:36:30Z

As the title suggests, this isn't ready for review yet, but I think the big pieces are in place.

This refactors the whole begin phase, particularly our model for progressed work. I tried to avoid doing a big refactor like this in the first pass, but eventually gave up — there are just too many inter-related things that had to change.

This also incorporates a change to make pendingWorkPriority exclude the priority of the fiber it belongs to, as in #8716. Again, I tried to keep this separate but it was too tied to everything else.

I wrote detailed comments inline as I refactored. I'll do an additional pass once it's closer to review.

If you're interested in taking a look, read through ReactFiberBeginWork.js. I think it's easier to follow conceptually. The basic algorithm is now:

Do we have progressed work since the last commit?
- If not, reset to current.
- If we do, did it render at the current priority?
  - If not, stash the progressed work and reset to current. This "forks" the child set.
  - If it did, reuse the progressed work.
Do we have pending props, state, or context?
- If so, reconcile. This will update the progressed work.
- If not, bailout. This will leave the progressed work alone.

TODO:

trueadm

Added some comments.

trueadm · 2017-05-25T22:29:27Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+      // in-progress if its the most recent fiber.
+      // TODO: It'd be nice to come up with a heuristic here that didn't require
+      // an additional fiber field.
+      !(progressedWork === workInProgress && workInProgress !== newestWork)


Can this be written differently? I think we can make it easier to interpret by not having the wrapping inverse of the inner condition.

Interesting, I wrote it that way at first but then rewrote it this way because I thought it was easier to understand haha :D

It's like choosing between De Morgan's Law and a double negative: which is more confusing??

Version 1:

!(progressedWork === workInProgress && workInProgress !== newestWork)

"Don't resume on the work-in-progress if it's not the newest work"

Version 2:

progressedWork !== workInProgress || workInProgress === newestWork

"Don't resume on the work-in-progress unless it's the newest work."

Yeah I guess version 2 is better.

trueadm · 2017-05-25T22:32:08Z

src/renderers/shared/fiber/ReactFiber.js

@@ -154,6 +154,7 @@ export type Fiber = {
  // progressed work or if it is better to continue from the current state.
  progressedPriority: PriorityLevel,
  progressedWork: ProgressedWork,
+  newestWork: Fiber,


could newestWork be lastWork?

Yeah maybe that's a bit better. Don't think either name makes it clear what it does or how it's different from progressedWork. I'll be sure we circle back to the naming before landing this PR.

Now we have another mostly synonymous term to add to the mix 🤡 current, in-progress, progressed, newest

acdlite · 2017-06-01T01:46:57Z

Got all the unit tests passing. This is now ready for review. (Edit: Found a bug, so may want to hold off before doing a thorough review. Feedback is welcome, though.)

acdlite · 2017-06-01T17:44:04Z

Just pushed a commit demonstrating a bug.

gaearon · 2017-06-03T11:28:38Z

src/renderers/shared/fiber/ReactChildFiber.js

+        invariant(
+          ownerFiber.tag === ClassComponent,
+          'Stateless function components cannot have refs.',
+        );


Should this say "cannot have string refs. Use callback refs instead"?

gaearon · 2017-06-03T11:29:21Z

src/renderers/shared/fiber/ReactChildFiber.js

+    invariant(
+      owner != null,
+      'Only a ReactOwner can have refs. You might be adding a ref to a ' +
+        "component that was not created inside a component's `render` " +


While we're at it, can we remove "ReactOwner" from the message? I don't think it has any meaning to people outside.

gaearon · 2017-06-03T11:32:44Z

src/renderers/shared/fiber/ReactChildFiber.js

+    }
+    const ref = function(value) {
+      const refs =
+        // TODO: Comparison to emptyObject always fails. Don't know why.


Let's make sure we fix this before merging. Should be something simple (eg maybe there's a resetModules call in an unfortunate place).

To the best of my knowledge, comparing object instances, even if structurally "identical" will always evaluate false.

({} === {}) // false

It will return true if they reference the same object in memory, which is what this is checking

const emptyObj = {}; const refs = emptyObj; refs === emptyObj; // true

gaearon · 2017-06-03T11:35:48Z

src/renderers/shared/fiber/ReactFiber.js

+  // A pooled ProgressedWork object, allocated once per fiber pair. Should not
+  // be accessed outside of this module.
+  _pooledProgressedWork: null | ProgressedWork,
+  newestWork: Fiber,


Could you please add comment for why this field matters?

I have a comment in beginWork but yeah I'll add one here, too. I'm planning to do another comments pass before landing.

gaearon · 2017-06-03T11:42:53Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+      invariant(
+        current !== null && current.child === workInProgress.child,
+        'Expected child to be the current child',
+      );


Let's note this is internal error and they should file a bug.

Saving that for the end :D

gaearon · 2017-06-03T11:54:29Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -911,7 +849,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

            // Complete the boundary as if it rendered null. This will unmount
            // the failed tree.
-            beginFailedWork(boundary.alternate, boundary, priorityLevel);
+            // TODO: We should unmount with task priority so that nothing else


Is this TODO part of this PR or a followup? Seems important.

I think it can be a follow up along with the other error issues we know about. Not related to this PR, just something I noticed.

Make sure we resume on progressed hidden children. I split these out into their own functions so that it doesn't distract from the normal, non-hidden code path. Will also make it easier to reuse later if/when we apply the offscreen children feature to other types of components.

There was some unnecessary abstraction in ReactFiberClassComponent that was making it difficult to maintain and, ironically, causing some duplication. I've moved most of that module into the begin phase, and consolidated the separate mount, resume mount, and update functions into a single function that's easier to reason about.

I would expect that whenever we resume on progressed work, the child set is a work-in-progress set.

Without this guard, the function throws

...rather than in reconcile and begin. Easier to keep track of.

Progressed priority could be less than work priority if there's a setState deep in the tree that's less than the current render priority but greater than the priority at which the fiber last reconciled.

ProgressedWork is a subset of Fiber. We use a separate Flow type to prevent access of extra properties, but we use a whole Fiber for monomorphism.

I had this wrong in an earlier commit. Performing work in the work-in-progress tree should never add work to the current tree. Only state updates do. So in the complete phase, we should bubble priority up from the children even if they are current, so that we don't drop pending updates. Also realized there's no reason to check if a work-in-progress is valid during a bailout. By the time we get to that function, we already determined if the work-in-progress is valid during the resume-or-reset step.

New test suite simulates the triangle demo. It renders a recursive tree and flushes it unit by unit, resetting the unit of work pointer to the root after each triangle is rendered. It also simulates the hover effect by updating a target node with high priority. The test is written in such a way that we can add fuzz testing on top of it later. For now, I've "manually" fuzz tested by trying out arbitrary values.

Makes bugs more noticeable.

Avoided doing this originally because I didn't want to add a field to the fork, and figured we could infer this in other ways. But the fork is a full Fiber for monomorphism purposes, anyway, so we already have it.

This allows for multiple nodes to be active at a time, which isn't possible in the demo because to hover over on node you must leave the other.

Most incremental tests rely on yielding at a specific point. Using an explicit API, rather than relying on fake expiration times that rely on implementation details, should make our tests more resilient and reduce false positives.

If `insertAfter` is null, the insertion position is at the start of the list. Similarly, if the `insertBefore` is null, the insertion position is at the end of the list. The starting/end position of one list is not necessarily the same as the starting/end position of another. So, only compare non-null values.

Previous version couldn't increment the counter more than once before flushing, which meant it couldn't reproduce certain errors that occur when running the triangle demo in Firefox, where starvation is worse. The new version uses a Redux-like approach, with actions.

This actually wasn't causing an error because the arguments were flipped twice: the first time to the wrong order, and the second time back to the correct order :D

This bug illustrates that our `progressedPriority` model is wrong. When deciding whether to resume work, we should compare the priority at which the parent reconciled its children, not the priority at which the fiber itself reconciled.

gaearon · 2018-01-05T15:14:55Z

I'll close as stale.

facebook-github-bot added the CLA Signed label May 16, 2017

acdlite requested a review from sebmarkbage May 16, 2017 03:37

acdlite force-pushed the beginphase branch 4 times, most recently from 65b5f78 to 93e5cb3 Compare May 25, 2017 22:24

trueadm reviewed May 25, 2017

View reviewed changes

acdlite force-pushed the beginphase branch 2 times, most recently from ef1df76 to ea7d604 Compare May 30, 2017 20:42

trueadm mentioned this pull request May 31, 2017

Fiber devtools fixes facebook/react-devtools#755

Merged

gaearon mentioned this pull request May 31, 2017

Follow up about RN Fiber changes after Fiber refactoring is merged facebook/react-devtools#757

Closed

acdlite changed the title ~~[Much WIP. Very not working.] Refactor progressed work model~~ Refactor progressed work model May 31, 2017

acdlite force-pushed the beginphase branch 4 times, most recently from 061ef03 to 4797a2c Compare June 1, 2017 01:45

acdlite force-pushed the beginphase branch from bb80c83 to 2803474 Compare June 1, 2017 17:41

acdlite force-pushed the beginphase branch 5 times, most recently from e78cc31 to ebe18b5 Compare June 2, 2017 21:26

gaearon reviewed Jun 3, 2017

View reviewed changes

acdlite added 4 commits June 5, 2017 18:30

[Much WIP. Very not working.] Refactor progressed work model

624d5b5

Re-implement begin phase for fragments

83ff51c

acdlite added 16 commits June 5, 2017 18:30

Run test script

4174480

Run prettier

9940c8a

Fix lint

53d9e0f

Add tests from facebook#8716

7571b0c

Add invariant

b9160ba

I would expect that whenever we resume on progressed work, the child set is a work-in-progress set.

shouldReuseContent should return false for text nodes

a1a2e9e

Without this guard, the function throws

Fix Flow arity errors

758aa3c

Update newestWork at end of beginWork

9df490d

...rather than in reconcile and begin. Easier to keep track of.

Only fork work if it's newer than the last commit

c76dacc

Remove invariant

4577349

Progressed priority could be less than work priority if there's a setState deep in the tree that's less than the current render priority but greater than the priority at which the fiber last reconciled.

Set instance variables before calling commit phase lifecycles

6527bb3

Use a fiber to stash progressed work

5e20db9

ProgressedWork is a subset of Fiber. We use a separate Flow type to prevent access of extra properties, but we use a whole Fiber for monomorphism.

Add extra invariants when bailing out on already progressed work

bf44b5a

Change color of dots on each tick in triangle demo

2b38dc3

Makes bugs more noticeable.

acdlite force-pushed the beginphase branch from ebe18b5 to 2b38dc3 Compare June 6, 2017 01:34

acdlite added 4 commits June 6, 2017 17:54

Add pendingWorkPriority to ProgressedWork

6e94072

Avoided doing this originally because I didn't want to add a field to the fork, and figured we could infer this in other ways. But the fork is a full Fiber for monomorphism purposes, anyway, so we already have it.

Triangle tester: Simulate multiple hovers in a single step

ec911db

This allows for multiple nodes to be active at a time, which isn't possible in the demo because to hover over on node you must leave the other.

Add ReactNoop.yieldBeforeNextUnitOfWork() API

9cec000

Most incremental tests rely on yielding at a specific point. Using an explicit API, rather than relying on fake expiration times that rely on implementation details, should make our tests more resilient and reduce false positives.

Use generator-based API for ReactNoop.yield

e1209b2

acdlite force-pushed the beginphase branch from df68585 to e1209b2 Compare June 8, 2017 20:55

acdlite added 4 commits June 8, 2017 17:44

Fix argument order

0959584

This actually wasn't causing an error because the arguments were flipped twice: the first time to the wrong order, and the second time back to the correct order :D

Introduce new failing test

32cb894

This bug illustrates that our `progressedPriority` model is wrong. When deciding whether to resume work, we should compare the priority at which the parent reconciled its children, not the priority at which the fiber itself reconciled.

acdlite mentioned this pull request Jun 13, 2017

[Umbrella] Async rendering #8830

Closed

17 tasks

acdlite mentioned this pull request Nov 15, 2017

[Umbrella] New algorithm for resuming interrupted work #11566

Open

gaearon closed this Jan 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor progressed work model #9695

Refactor progressed work model #9695

acdlite commented May 16, 2017 •

edited

Loading

trueadm left a comment

trueadm May 25, 2017

acdlite May 25, 2017

acdlite May 25, 2017

acdlite May 26, 2017

trueadm May 25, 2017

acdlite May 25, 2017

bvaughn May 26, 2017

acdlite commented Jun 1, 2017 •

edited

Loading

acdlite commented Jun 1, 2017

gaearon Jun 3, 2017

acdlite Jun 3, 2017

gaearon Jun 3, 2017

gaearon Jun 3, 2017

jeromecovington Jun 3, 2017

acdlite Jun 3, 2017

gaearon Jun 3, 2017

acdlite Jun 3, 2017 •

edited

Loading

gaearon Jun 3, 2017

acdlite Jun 3, 2017

gaearon Jun 3, 2017

acdlite Jun 3, 2017

gaearon commented Jan 5, 2018

Refactor progressed work model #9695

Refactor progressed work model #9695

Conversation

acdlite commented May 16, 2017 • edited Loading

trueadm left a 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 Jun 1, 2017 • edited Loading

acdlite commented Jun 1, 2017

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 Jun 3, 2017 • 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

gaearon commented Jan 5, 2018

acdlite commented May 16, 2017 •

edited

Loading

acdlite commented Jun 1, 2017 •

edited

Loading

acdlite Jun 3, 2017 •

edited

Loading