[Fiber] Move memoization to begin phase #8655

acdlite · 2016-12-30T02:11:37Z

Because memoizedProps and memoizedState are read from the instance, the
instance's input pointers (props, state, context) should be updated even
when shouldComponentUpdate causes a bail-out.

Update

The scope of this PR changed a bit. The main change is to move memoization to the begin phase, right after reconciliation.

Currently we update the memoized inputs (props, state) during the complete phase, as we go back up the tree. That means we can't reuse work until of its children have completed.

By moving memoization to the begin phase, we can do a shallow bailout, reusing a unit of work even if there's still work to do in its children.

Memoization now happens whenever a fiber's child property is updated; typically, right after reconciling. It's also updated when shouldComponentUpdate returns false, because that indicates that the given state and props are equal to the memoized state and props.

acdlite · 2016-12-30T02:35:39Z

If you can think of a more realistic test scenario let me know. The one I wrote is pretty contrived, but since this is an edge case that might be unavoidable.

sebmarkbage

This would need to tag work as "in the middle of work" because otherwise if it gets aborted half way it looks as if the whole subtree can be reused.

I'm surprised we don't have a test that covers that. We should.

acdlite · 2016-12-30T22:33:32Z

I'll write one and see if it can be solved by setting the priority level at the same time as memoization, as you suggested in the FB group.

acdlite · 2016-12-30T23:31:13Z

@sebmarkbage Struggling to come up with a failing test case. As far as I can tell, even when work is aborted half way, the subtree doesn't get reused because bailoutOnFinishedWork returns the child.

sebmarkbage · 2017-01-02T18:25:11Z

Try it with one or two more levels. The work of the direct child isn't reused because one more child needs to be worked on to test if it has priority level. Also, ensure that there are siblings since currently work can only be reused after a tree is fully completed so to abort you need to be able to abort in a later sibling.

bvaughn · 2017-01-03T23:58:46Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -103,6 +103,8 @@ module.exports = function(

    const instance = workInProgress.stateNode;
    if (typeof instance.shouldComponentUpdate === 'function') {
+      instance.props = workInProgress.memoizedProps;
+      instance.state = workInProgress.memoizedState;
      const shouldUpdate = instance.shouldComponentUpdate(newProps, newState, newContext);


Don't we have the same potential problem below for the ReactPureComponent state check?

!shallowEqual(instance.state, newState)

Shouldn't this function always use memoizedProps and memoizedState? (Can we also just remove the redundant oldProps parameter?)

sebmarkbage · 2017-01-04T00:20:52Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+        const instance = workInProgress.stateNode;
+        workInProgress.memoizedState = instance.state;
+        workInProgress.memoizedProps = instance.props;
+        workInProgress.pendingProps = null;


We had trouble keeping things in sync when we split these. It might be helpful to move this inside of updateClassComponent and similarly for the others so that it is easier to tell when this needs to happen relative to other things. I'm not 100% sure this should definitely be at the very end in all cases.

bvaughn · 2017-01-04T18:35:57Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -103,6 +103,8 @@ module.exports = function(

    const instance = workInProgress.stateNode;
    if (typeof instance.shouldComponentUpdate === 'function') {
+      instance.props = workInProgress.memoizedProps;
+      instance.state = workInProgress.memoizedState;
      const shouldUpdate = instance.shouldComponentUpdate(newProps, newState, newContext);


Shouldn't we also do this reset before componentWillReceiveProps? and componentWillUpdate?

I think this argues for putting the reset at the start of updateClassInstance- so the other 2 lifecycle methods aren't called with stale props. Also, this way, even if the user doesn't define shouldComponentUpdate the instance attributes will still be synced before render is called.

acdlite · 2017-01-05T00:30:21Z

Found a few more problems. The test I wrote is wrong. I think relying so much on instance.props and instance.state is masking issues that we didn't notice before. Will look at it some more tomorrow.

acdlite · 2017-01-08T01:29:53Z

Ok, I clean up my commits and this is now ready for review.

I couldn't find a clean way to separate updating the instance pointers from moving memoization to the begin phase, so I've left them as part of the same commit. The diff isn't that big so hopefully it's not hard to review.

@sebmarkbage, I wrote a test to confirm that we don't drop work in the subtree. I tried to break this for a while and I'm pretty confident that it works.

acdlite · 2017-01-08T01:30:45Z

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

+        // So its direct children cannot be reused when we resume at
+        // low priority. I think this would be fixed by changing
+        // pendingWorkPriority and progressedPriority to be the priority of
+        // the children only, not including the fiber itself.


I started working on a fix for this, which I'll submit as a separate PR, since it's outside the scope of this one.

Currently we update the memoized inputs (props, state) during the complete phase, as we go back up the tree. That means we can't reuse work until of its children have completed. By moving memoization to the begin phase, we can do a shallow bailout, reusing a unit of work even if there's still work to do in its children. Memoization now happens whenever a fiber's `child` property is updated; typically, right after reconciling. It's also updated when `shouldComponentUpdate` returns false, because that indicates that the given state and props are equal to the memoized state and props.

Includes a test that confirms that work that is bailed out before completing can be reused without dropping the entire subtree.

sebmarkbage · 2017-01-12T20:26:05Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+    if (workInProgress.tag === ClassComponent) {
+      const instance = workInProgress.stateNode;
+      workInProgress.memoizedProps = instance.props;
+      workInProgress.memoizedState = instance.state;


Why is this needed? Wouldn't this already have been memoized if a child threw?

I put the memoization after reconciling the children because it seemed to make the most sense conceptually. Perhaps I could move it earlier before any lifecycles. The tricky part about that is that some things need to read from the previous memoized values. So you'd have to pass those around and it gets a bit hard to follow. But there's likely a better way to structure all this.

I'd like to get to the point where we never read from the instance at all. This is one of two (I believe) places where we still do.

I still don't understand why this needs to be done in beginFailedWork specifically.

If anything, this should set the memoized values to null since this component is now in an invalid state. The children output doesn't correspond to the input.

I agree but that breaks an assumption we make in a few places that we always have either pending or memoized props. I figured that was outside the scope of this PR but I'll fix it here if it helps this land. Shouldn't be too difficult.

That wasn't the case before because we would always memoize in completeWork, even if it failed in beginWork.

Haha if I remove those invariants everything seems to still work. I guess I thought it'd be a bit more complicated than that. I'll go ahead and push that :)

Edit: Never mind, this actually does break some things

Hmm although what should get passed to shouldComponentUpdate the next time if we set memoizedProps to null? An empty object?

I guess in that case we shouldn't call shouldComponentUpdate at all since it's as if it's mounting for the first time.

I think that something is broken here with this error handling logic but I understand now that you're only preserving existing behavior by moving it in here. I think that's fine for the first iteration.

sebmarkbage · 2017-01-12T20:51:09Z

The heavy use of abstractions like markUpdate, markUpdateIfAlreadyInProgress, resetInputPointers, memoizeProps, memoizeState is an indication that we probably need to reorganize some of this so that those steps can be consolidated but we can do that later if this works out.

sebmarkbage · 2017-01-12T20:51:55Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+  } = ReactFiberClassComponent(
+    scheduleUpdate,
+    getPriorityContext,
+    memoizeProps,


Is this an abstraction worthy the pain of passing around? :)

Haha I almost did it so I could log when I was happening :D And at one point it was a bit more involved.

I agree this could be restructured in a better way but when I'm unsure I prefer to just find something that works and passes the tests and then clean it up later.

*mostly, not almost

For example if I rewrote all the stuff in ReactFiberClassComponent, and then you told me the overall approach was wrong, I'd have wasted a bunch of time. And it would be harder to review. Easier to rewrite after overall approach is figured out.

sebmarkbage

I was surprised that this just works.

I was afraid that we would be trying to reuse incomplete work however if we're trying to reuse a previously progressed tree, then cloneChildFibers will go into the non-clone path which will preserve the pendingProps and priority flag.

I don't feel like I totally understand this but I don't see any obvious major flaws.

acdlite · 2017-01-12T23:24:33Z

@sebmarkbage My understanding is: right now bailoutOnAlreadyFinishedWork returns the child, so it's not possible to lose the side-effects in the subtree. If we want to return null instead and do a full bailout, we need to ensure that the subtree has completed, so we don't lose side-effects. For that will need to flip a bit in the effect tag (or equivalent solution) as you pointed out.

Related: #8716

acdlite · 2017-01-12T23:35:01Z

Once the CI passes I'll merge this then open a follow-up PR to restructure and clean-up the weird abstractions.

acdlite requested a review from bvaughn December 30, 2016 02:11

facebook-github-bot added the GH Review: review-needed label Dec 30, 2016

acdlite changed the title ~~[Fiber] Memoize preempted work~~ [Fiber] Memoize work even if sCU causes a bail-out Dec 30, 2016

facebook-github-bot added the CLA Signed label Dec 30, 2016

acdlite force-pushed the fibermemoizepremptedwork branch from ad0141c to 0375fbd Compare December 30, 2016 02:32

sebmarkbage mentioned this pull request Dec 30, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

sebmarkbage added GH Review: needs-revision and removed GH Review: review-needed labels Dec 30, 2016

sebmarkbage requested changes Dec 30, 2016

View reviewed changes

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jan 3, 2017

bvaughn mentioned this pull request Jan 3, 2017

Pass prevContext param to componentDidUpdate #8631

Merged

bvaughn reviewed Jan 3, 2017

View reviewed changes

sebmarkbage reviewed Jan 4, 2017

View reviewed changes

bvaughn reviewed Jan 4, 2017

View reviewed changes

acdlite force-pushed the fibermemoizepremptedwork branch 5 times, most recently from ef96a54 to 7cace1b Compare January 8, 2017 01:26

acdlite changed the title ~~[Fiber] Memoize work even if sCU causes a bail-out~~ [Fiber] Move memoization to begin phase Jan 8, 2017

acdlite commented Jan 8, 2017

View reviewed changes

acdlite force-pushed the fibermemoizepremptedwork branch 2 times, most recently from df65ba6 to 026f7d3 Compare January 9, 2017 00:05

acdlite mentioned this pull request Jan 9, 2017

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

Closed

acdlite added 2 commits January 12, 2017 11:55

Confirm that a shallow bailout does not drop work in the child

d17b9a1

Includes a test that confirms that work that is bailed out before completing can be reused without dropping the entire subtree.

acdlite force-pushed the fibermemoizepremptedwork branch from 026f7d3 to e0f8c51 Compare January 12, 2017 20:02

acdlite mentioned this pull request Jan 12, 2017

Add failing test for state merging when sCU is false #8764

Closed

Add test for state merging when sCU is false

54ebcbc

acdlite force-pushed the fibermemoizepremptedwork branch from e0f8c51 to 54ebcbc Compare January 12, 2017 20:04

sebmarkbage reviewed Jan 12, 2017

View reviewed changes

sebmarkbage approved these changes Jan 12, 2017

View reviewed changes

acdlite added GH Review: accepted and removed GH Review: review-needed labels Jan 12, 2017

acdlite merged commit 199db63 into facebook:master Jan 13, 2017

acdlite deleted the fibermemoizepremptedwork branch January 13, 2017 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fiber] Move memoization to begin phase #8655

[Fiber] Move memoization to begin phase #8655

acdlite commented Dec 30, 2016 •

edited

Loading

acdlite commented Dec 30, 2016

sebmarkbage left a comment

acdlite commented Dec 30, 2016

acdlite commented Dec 30, 2016

sebmarkbage commented Jan 2, 2017

bvaughn Jan 3, 2017

sebmarkbage Jan 4, 2017

bvaughn Jan 4, 2017

acdlite commented Jan 5, 2017

acdlite commented Jan 8, 2017

acdlite Jan 8, 2017 •

edited

Loading

sebmarkbage Jan 12, 2017

acdlite Jan 12, 2017

acdlite Jan 12, 2017

sebmarkbage Jan 12, 2017

acdlite Jan 12, 2017

acdlite Jan 12, 2017

acdlite Jan 12, 2017 •

edited

Loading

acdlite Jan 12, 2017

acdlite Jan 12, 2017

sebmarkbage Jan 12, 2017

sebmarkbage commented Jan 12, 2017

sebmarkbage Jan 12, 2017

acdlite Jan 12, 2017

acdlite Jan 12, 2017

acdlite Jan 12, 2017

sebmarkbage left a comment

acdlite commented Jan 12, 2017

acdlite commented Jan 12, 2017

[Fiber] Move memoization to begin phase #8655

[Fiber] Move memoization to begin phase #8655

Conversation

acdlite commented Dec 30, 2016 • edited Loading

Update

acdlite commented Dec 30, 2016

sebmarkbage left a comment

Choose a reason for hiding this comment

acdlite commented Dec 30, 2016

acdlite commented Dec 30, 2016

sebmarkbage commented Jan 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Jan 5, 2017

acdlite commented Jan 8, 2017

acdlite Jan 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Jan 12, 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

sebmarkbage commented Jan 12, 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

sebmarkbage left a comment

Choose a reason for hiding this comment

acdlite commented Jan 12, 2017

acdlite commented Jan 12, 2017

acdlite commented Dec 30, 2016 •

edited

Loading

acdlite Jan 8, 2017 •

edited

Loading

acdlite Jan 12, 2017 •

edited

Loading