[Fiber] Change work priority to represent the priority of the child #8716

acdlite · 2017-01-09T07:02:44Z

When the children are reconciled, set the work priority of the parent to the current priority level. Do not set the work priority of the children.
In findNextUnitOfWork, we need to check both the update priority and the work priority of the root, since the work priority field no longer represents both.
In scheduleUpdate, when bubbling the priority to the root, skip the fiber that is being updated on and start with the parent.
Move resetWorkPriority from commitAllWork in the scheduler into ReactFiberCompleteWork so we can branch on the type of the component. This is needed for HostComponents, which may have leftover work priority if we bailed out on a hidden subtree.
Because a fiber has no field that represents its own priority, we cannot bailout on low priority at the beginning of beginWork. Instead we have to perform that check while performing work on the parent, before its child is returned.

acdlite · 2017-01-10T08:21:16Z

Got it mostly working! Just one remaining issue with reusing side-effects on an already completed subtree. I'll look at this tomorrow.

acdlite · 2017-01-11T01:04:55Z

@sebmarkbage This is now ready for review. (Well, if/when #8655 is approved, I mean.)

acdlite · 2017-01-11T01:07:49Z

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

@@ -831,9 +831,9 @@ describe('ReactIncrementalSideEffects', () => {
        // Updated.
        span(1),
        div(
+          span(1),


I think this change is correct. The effect is flushed sooner than before because we're reusing more work. I may be wrong.

acdlite · 2017-01-11T01:08:41Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+    if (fiber.tag === HostRoot) {
+      // This is a root, which does not have a parent, so we need to use the
+      // update queue.
+      addTopLevelUpdate(fiber, { element: null }, null, TaskPriority);


This feels a bit weird but I couldn't think of a better solution.

acdlite · 2017-01-11T01:46:17Z

Huh don't know why CircleCI is failing

gaearon · 2017-01-11T15:51:24Z

I think you forgot to re-record tests.

The file says this is still failing:

 +src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
 +* can defer side-effects and reuse them later - complex

But apparently you fixed it.

acdlite · 2017-01-11T15:54:26Z

What's weird is the file is correct locally, not on GH. Probably some stupid mistake I made. I'll check in a bit.

gaearon · 2017-01-11T15:56:33Z

Wrong branch?

Implications: - When we clone from current, set the pendingWorkPriority of the parent, not the child. - The check to bail out on low priority must occur earlier, before we beginWork on it. It turns out this only happens when the parent bails out on already finished work. So we do the check there.

Workaround for the fact that host roots (which act as an error boundary for uncaught errors) do not have a parent. Because the child will be set to null on the next commit, anyway, I think this is fine.

Must bubble up priority from a progressed subtree that was preempted by a high priority update. We know this is the case if the progressed priority is lower than the priority at which we're reconciling.

Without the change, the second update does not create higher priority work than the first one.

If we already completed a subtree, we should be able to bail out because the tree no longer has any priority.

I added this line in a previous diff because I was thought it was necessary to avoid dropping work. But it turns out that not only is it unnecessary, but it prevents us from being able to reuse already completed progressed work, because the bailout on low priority check will always fail.

This was causing effects to be dropped.

We now do this inside the scheduler, in performUnitOfWork

Neglected to do this in a previous commit. We keep these functions separate so that we don't have to perform the failed work check on every unit of work. Not sure if there's test we can write to ensure they stay in sync.

Fixes a bug, and reverts some of the changes I made in previous commits that was masking this incorrect behavior. I understand it better now. I'll rebase to clean this up. The demo is almost working but there are a few remaining bugs. Still investigating...

I introduce the ReactDOMFrameScheduling module which just exposes rIC and rAF. The polyfill works by scheduling a requestAnimationFrame, store the time for the start of the frame, then schedule a postMessage which gets scheduled after paint. The deadline is set to time + frame rate. By separating the idle call into a separate event tick we ensure that layout, paint and other browser work is counted against the available time. The frame rate is dynamically adjusted by tracking the minimum time between two rAF callbacks. This is not perfect because browsers can schedule multiple callbacks to catch up after a long frame. To compensate for this we only adjust if two consecutive periods are faster than normal. This seems to guarantee that we will hit frame deadlines and we don't end up dropping frames. However, there is still some lost time so we risk starving by not having enough idle time. Especially Firefox seems to have issues keeping up on the triangle demo However, that is also true for native rIC in Firefox. It seems like more render work is scheduled on the main thread pipeline and also that JS execution just generally has more overhead.

Not sure why I did it this way before, but it's a mistake.

acdlite · 2017-01-22T10:20:24Z

Almost have everything working. There's still a bug in the demo that needs to be fixed (probably not introduced by this PR because it exists in master, too).

Once I figure that remaining bit out I'll write more tests related to reusing child effects, since that's crucial to the demo working properly but there are many cases that aren't covered by tests.

iamdustan · 2017-01-24T15:28:00Z

src/renderers/dom/fiber/ReactDOMFrameScheduling.js

+    frameDeadline = rafTime + activeFrameTime;
+    if (!isIdleScheduled) {
+      isIdleScheduled = true;
+      window.postMessage(messageKey, '*');


Is it safe to assume this module isn’t relevant for server rendering?

Fiber DOM renderer is client-only so no, it's not relevant.

gaearon · 2017-10-04T23:27:25Z

Can we close this?

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 9, 2017

sebmarkbage mentioned this pull request Jan 9, 2017

[Fiber] Umbrella for remaining features / bugs #7925

Closed

acdlite force-pushed the fiberworkpriority branch 2 times, most recently from 9b47df1 to 0083655 Compare January 10, 2017 08:20

acdlite force-pushed the fiberworkpriority branch from 0083655 to 8b61e5b Compare January 11, 2017 01:04

acdlite changed the title ~~[WIP][Fiber] Change work priority to represent the priority of the child~~ [Fiber] Change work priority to represent the priority of the child Jan 11, 2017

acdlite commented Jan 11, 2017

View reviewed changes

acdlite force-pushed the fiberworkpriority branch from 8b61e5b to 0c240f4 Compare January 11, 2017 19:26

acdlite mentioned this pull request Jan 12, 2017

[Fiber] Move memoization to begin phase #8655

Merged

acdlite force-pushed the fiberworkpriority branch 4 times, most recently from 7087d18 to beaf215 Compare January 18, 2017 00:30

acdlite mentioned this pull request Jan 19, 2017

[Umbrella] Async rendering #8830

Closed

17 tasks

acdlite added 3 commits January 20, 2017 16:25

Ensure effects are reused when bailing out on the child

aabcaa6

When a boundary fails, set pendingWorkPriority to TaskPriority

02a32fc

Workaround for the fact that host roots (which act as an error boundary for uncaught errors) do not have a parent. Because the child will be set to null on the next commit, anyway, I think this is fine.

acdlite force-pushed the fiberworkpriority branch 2 times, most recently from addf451 to fb01f28 Compare January 21, 2017 00:30

acdlite added 3 commits January 21, 2017 11:45

Ensure that priority is not dropped from a progressed subtree

a0c62aa

Must bubble up priority from a progressed subtree that was preempted by a high priority update. We know this is the case if the progressed priority is lower than the priority at which we're reconciling.

Fix mistake in sCU test

d6a9c05

Without the change, the second update does not create higher priority work than the first one.

Add failing test for bailing out on already completed work

865c452

If we already completed a subtree, we should be able to bail out because the tree no longer has any priority.

acdlite added 6 commits January 21, 2017 11:45

Fix reuseChildrenEffects to include effects on the fiber itself

32ab4b5

This was causing effects to be dropped.

Remove outdated TODO

700296d

We now do this inside the scheduler, in performUnitOfWork

Apply performUnitOfWork changes to performFailedUnitOfWork, too

be824d1

Neglected to do this in a previous commit. We keep these functions separate so that we don't have to perform the failed work check on every unit of work. Not sure if there's test we can write to ensure they stay in sync.

Run test script

55366f8

acdlite force-pushed the fiberworkpriority branch from 7b70d5f to 2ce2a4b Compare January 22, 2017 10:06

sebmarkbage and others added 2 commits January 22, 2017 02:06

Update progressed priority when cloning

739281e

Not sure why I did it this way before, but it's a mistake.

iamdustan reviewed Jan 24, 2017

View reviewed changes

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

acdlite mentioned this pull request May 16, 2017

Refactor progressed work model #9695

Closed

21 tasks

acdlite added a commit to acdlite/react that referenced this pull request Jun 1, 2017

Add tests from facebook#8716

bb80c83

acdlite added a commit to acdlite/react that referenced this pull request Jun 1, 2017

Add tests from facebook#8716

2803474

acdlite added a commit to acdlite/react that referenced this pull request Jun 1, 2017

Add tests from facebook#8716

3e3114d

acdlite added a commit to acdlite/react that referenced this pull request Jun 1, 2017

Add tests from facebook#8716

007ea48

acdlite added a commit to acdlite/react that referenced this pull request Jun 2, 2017

Add tests from facebook#8716

0be96a1

acdlite added a commit to acdlite/react that referenced this pull request Jun 6, 2017

Add tests from facebook#8716

7571b0c

gaearon closed this Jan 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Change work priority to represent the priority of the child #8716

[Fiber] Change work priority to represent the priority of the child #8716

acdlite commented Jan 9, 2017 •

edited

Loading

acdlite commented Jan 10, 2017

acdlite commented Jan 11, 2017 •

edited

Loading

acdlite Jan 11, 2017

acdlite Jan 11, 2017

acdlite commented Jan 11, 2017

gaearon commented Jan 11, 2017 •

edited

Loading

acdlite commented Jan 11, 2017

gaearon commented Jan 11, 2017

acdlite commented Jan 22, 2017

iamdustan Jan 24, 2017

gaearon Jan 24, 2017

gaearon commented Oct 4, 2017

[Fiber] Change work priority to represent the priority of the child #8716

[Fiber] Change work priority to represent the priority of the child #8716

Conversation

acdlite commented Jan 9, 2017 • edited Loading

acdlite commented Jan 10, 2017

acdlite commented Jan 11, 2017 • edited Loading

acdlite Jan 11, 2017

Choose a reason for hiding this comment

acdlite Jan 11, 2017

Choose a reason for hiding this comment

acdlite commented Jan 11, 2017

gaearon commented Jan 11, 2017 • edited Loading

acdlite commented Jan 11, 2017

gaearon commented Jan 11, 2017

acdlite commented Jan 22, 2017

iamdustan Jan 24, 2017

Choose a reason for hiding this comment

gaearon Jan 24, 2017

Choose a reason for hiding this comment

gaearon commented Oct 4, 2017

acdlite commented Jan 9, 2017 •

edited

Loading

acdlite commented Jan 11, 2017 •

edited

Loading

gaearon commented Jan 11, 2017 •

edited

Loading