Remove progressed work #10008

acdlite · 2017-06-19T23:29:37Z

The current implementation of resuming work is buggy. The underlying model is also flawed. Rather than attempt to fix a flawed model, we'll scrap the feature entirely and add it back later.

The triangle fuzz tester is now passing. By keeping this test green, we should have greater confidence that changes do not introduce regressions.

See #8830 for the larger plan.

sebmarkbage · 2017-06-30T23:19:23Z

src/renderers/noop/ReactNoopEntry.js

@@ -430,10 +430,10 @@ var ReactNoop = {
      if (fiber.updateQueue) {
        logUpdateQueue(fiber.updateQueue, depth);
      }
-      const childInProgress = fiber.progressedChild;
+      const childInProgress = fiber.child;
      if (childInProgress && childInProgress !== fiber.child) {


This branch is now always dead. Might be better to just comment it out so we know what it is supposed to do.

sebmarkbage · 2017-06-30T23:39:31Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

-          child = child.sibling;
-        }
-        return null;
-      }


I'm having a hard time convincing myself this isn't needed anymore.

I think it's not needed because we handle this during priority reset, here: https://github.com/acdlite/react/blob/9713bd40a6e4eb406b6f66b8a55c4bbd662d5ae3/src/renderers/shared/fiber/ReactFiberScheduler.js#L558-L564

sebmarkbage · 2017-06-30T23:41:20Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -580,7 +583,8 @@ module.exports = function(
      oldProps === newProps &&
      oldState === newState &&
      !hasContextChanged() &&
-      !(updateQueue !== null && updateQueue.hasForceUpdate)
+      !(workInProgress.updateQueue !== null &&


So this is re-reading the updateQueue, is that the purpose? Why this change?

beginUpdateQueue sometimes clones workInProgress.updateQueue, so we need to point to the latest version.

sebmarkbage · 2017-06-30T23:55:18Z

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

@@ -280,7 +280,7 @@ describe('ReactDebugFiberPerf', () => {
    expect(getFlameChart()).toMatchSnapshot();
  });

-  it('measures deprioritized work', () => {
+  xit('measures deprioritized work', () => {


Can we re-enable this?

sebmarkbage · 2017-06-30T23:59:51Z

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

-      render() {
-        ops.push('Bar');
-        return <span prop={this.state.active ? 'X' : this.props.idx} />;
+  xit(


This one is similar. We might be able to just change the test a bit?

acdlite · 2017-07-01T00:53:34Z

@sebmarkbage Re-enabled those tests

gaearon · 2017-07-01T01:26:35Z

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

@@ -382,7 +382,7 @@ describe('ReactDebugFiberPerf', () => {
    expect(getFlameChart()).toMatchSnapshot();
  });

-  it('deduplicates lifecycle names during commit to reduce overhead', () => {
+  xit('deduplicates lifecycle names during commit to reduce overhead', () => {


Why do we need to disable this? I don't think it tests specifically progressed work.

sebmarkbage

Ok, let's try it. :)

Neglected to fix this earlier because these tests were ignored.

The current implementation of resuming work is buggy. The underlying model is also flawed. Rather than attempt to fix a flawed model, we'll scrap the feature entirely and add it back later.

When we create a new work-in-progress, the existing pending props are no longer valid.

Now that we've removed the buggy implementation of resuming work, the fuzz tester passes consistently.

When an error in thrown in the begin phase, we begin work on the error boundary a second time to unmount the children. This is a special case of "resuming" work that we need to account for. The children are set to the current children so that we can delete them.

it -> xit The diff looks messier than it actually is because of Prettier.

To make sure we don't reset the priority of a down-prioritized fiber, we compare the priority we're currently rendering at to the fiber's work priority. If the work priority is lower, then we know not to reset the work priority.

This fixes a snapshot test regression introduced by facebook#10008. I had noticed this change and believe the new behavior was correct, but upon further investigation I was wrong. This reverts the snapshot test and fixes it accordingly.

This fixes a snapshot test regression introduced by #10008. I had noticed this change and believe the new behavior was correct, but upon further investigation I was wrong. This reverts the snapshot test and fixes it accordingly.

acdlite requested a review from sebmarkbage June 19, 2017 23:29

facebook-github-bot added the CLA Signed label Jun 19, 2017

acdlite mentioned this pull request Jun 19, 2017

[Umbrella] Async rendering #8830

Closed

17 tasks

acdlite force-pushed the removeprogressedwork branch 2 times, most recently from 276c65a to 4e2f445 Compare June 30, 2017 22:38

sebmarkbage reviewed Jun 30, 2017

View reviewed changes

acdlite force-pushed the removeprogressedwork branch from 4e2f445 to 9713bd4 Compare June 30, 2017 23:22

sebmarkbage reviewed Jun 30, 2017

View reviewed changes

acdlite force-pushed the removeprogressedwork branch 2 times, most recently from bff5bdc to 19eca82 Compare July 1, 2017 00:53

gaearon reviewed Jul 1, 2017

View reviewed changes

sebmarkbage approved these changes Jul 1, 2017

View reviewed changes

acdlite force-pushed the removeprogressedwork branch from 19eca82 to 4b9775e Compare July 1, 2017 01:38

acdlite added 7 commits June 30, 2017 18:39

Update fuzz tester to not use animation priority

9749a6e

Neglected to fix this earlier because these tests were ignored.

Remove ability to resume work

a0af1e1

The current implementation of resuming work is buggy. The underlying model is also flawed. Rather than attempt to fix a flawed model, we'll scrap the feature entirely and add it back later.

Clear pending props when cloning from current

264a921

When we create a new work-in-progress, the existing pending props are no longer valid.

Enable the fuzz tester

ccbb92f

Now that we've removed the buggy implementation of resuming work, the fuzz tester passes consistently.

Ignore incremental tests that assert on work reuse

de08c2a

it -> xit The diff looks messier than it actually is because of Prettier.

Down-prioritize children of hidden host components

f9af9aa

To make sure we don't reset the priority of a down-prioritized fiber, we compare the priority we're currently rendering at to the fiber's work priority. If the work priority is lower, then we know not to reset the work priority.

acdlite force-pushed the removeprogressedwork branch from 4b9775e to f9af9aa Compare July 1, 2017 01:39

acdlite merged commit f9af9aa into facebook:master Jul 1, 2017

acdlite mentioned this pull request Jul 7, 2017

Don't increase priority when cloning current children #10123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove progressed work #10008

Remove progressed work #10008

acdlite commented Jun 19, 2017

sebmarkbage Jun 30, 2017

acdlite Jun 30, 2017

sebmarkbage Jun 30, 2017

acdlite Jun 30, 2017

sebmarkbage Jun 30, 2017

acdlite Jun 30, 2017

sebmarkbage Jun 30, 2017

sebmarkbage Jun 30, 2017

acdlite commented Jul 1, 2017

gaearon Jul 1, 2017

sebmarkbage left a comment

Remove progressed work #10008

Remove progressed work #10008

Conversation

acdlite commented Jun 19, 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

Choose a reason for hiding this comment

acdlite commented Jul 1, 2017

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment