From 6d8cd151b5893084e9ebd638203968ac1700eaaa Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 18 Oct 2016 17:01:15 -0700 Subject: [PATCH 1/3] Use the memoized props/state from the workInProgress We don't want to use the "current" props/state because if we have started work, then pause and continue then we'll have the newer props/state on it already. If it is not a resume, it should be the same as current. --- .../shared/fiber/ReactFiberBeginWork.js | 21 +++-- .../fiber/__tests__/ReactIncremental-test.js | 92 +++++++++++++++++++ 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index a1fc0b998674..b354cb18eb99 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -160,15 +160,24 @@ module.exports = function(config : HostConfig, s // Account for the possibly of missing pending props by falling back to the // memoized props. var props = workInProgress.pendingProps; - if (!props && current) { - props = current.memoizedProps; + if (!props) { + // If there isn't any new props, then we'll reuse the memoized props. + // This could be from already completed work. + props = workInProgress.memoizedProps; + if (!props) { + throw new Error('There should always be pending or memoized props.'); + } } + // Compute the state using the memoized state and the update queue. var updateQueue = workInProgress.updateQueue; - var previousState = current ? current.memoizedState : null; - var state = updateQueue ? - mergeUpdateQueue(updateQueue, previousState, props) : - previousState; + var previousState = workInProgress.memoizedState; + var state; + if (updateQueue) { + state = mergeUpdateQueue(updateQueue, previousState, props); + } else { + state = previousState; + } var instance = workInProgress.stateNode; if (!instance) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index 6d4db53996b9..aa6cc245541b 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -835,4 +835,96 @@ describe('ReactIncremental', () => { ReactNoop.flush(); expect(ops).toEqual(['Foo', 'Bar', 'Baz', 'Bar', 'Baz']); }); + + it('can call sCU while resuming a partly mounted component', () => { + var ops = []; + + class Bar extends React.Component { + state = { y: 'A' }; + shouldComponentUpdate(newProps, newState) { + return this.props.x !== newProps.x || + this.state.y !== newState.y; + } + render() { + ops.push('Bar:' + this.props.x); + return ; + } + } + + function Foo(props) { + ops.push('Foo'); + return [ + , + , + , + ]; + } + + ReactNoop.render(); + ReactNoop.flushDeferredPri(30); + expect(ops).toEqual(['Foo', 'Bar:A', 'Bar:B']); + + ops = []; + + ReactNoop.render(); + ReactNoop.flushDeferredPri(40); + expect(ops).toEqual(['Foo', 'Bar:B', 'Bar:C']); + }); + + it('gets new props when setting state on a partly updated component', () => { + var ops = []; + var instances = []; + + class Bar extends React.Component { + state = { y: 'A' }; + constructor() { + super(); + instances.push(this); + } + performAction() { + this.setState({ + y: 'B', + }); + } + render() { + ops.push('Bar:' + this.props.x + '-' + this.props.step); + return ; + } + } + + function Baz() { + // This component is used as a sibling to Foo so that we can fully + // complete Foo, without committing. + ops.push('Baz'); + return
; + } + + function Foo(props) { + ops.push('Foo'); + return [ + , + , + ]; + } + + ReactNoop.render(
); + ReactNoop.flush(); + + ops = []; + + // Flush part way through with new props, fully completing the first Bar. + // However, it doesn't commit yet. + ReactNoop.render(
); + ReactNoop.flushDeferredPri(45); + expect(ops).toEqual(['Foo', 'Bar:A-1', 'Bar:B-1', 'Baz']); + + // Make an update to the same Bar. + instances[0].performAction(); + + ops = []; + + ReactNoop.flush(); + expect(ops).toEqual(['Bar:A-1', 'Baz', 'Baz']); + }); + }); From cb2bc0d8f7ac5d9c92cc1c96f15fcdeabeba27e3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 18 Oct 2016 15:41:49 -0700 Subject: [PATCH 2/3] Deprioritize setState within a deprioritized tree Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority. This ensures that we don't let a deprioritized subtree get reconciled at a higher priority. --- .../shared/fiber/ReactFiberBeginWork.js | 20 ++++ .../ReactIncrementalSideEffects-test.js | 107 ++++++++++++++++++ 2 files changed, 127 insertions(+) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index b354cb18eb99..b75fe2e67be4 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -309,6 +309,26 @@ module.exports = function(config : HostConfig, s function bailoutOnAlreadyFinishedWork(current, workInProgress : Fiber) : ?Fiber { const priorityLevel = workInProgress.pendingWorkPriority; + if (workInProgress.tag === HostComponent && + workInProgress.memoizedProps.hidden && + workInProgress.pendingWorkPriority !== OffscreenPriority) { + // This subtree still has work, but it should be deprioritized so we need + // to bail out and not do any work yet. + // TODO: It would be better if this tree got its correct priority set + // during scheduleUpdate instead because otherwise we'll start a higher + // priority reconciliation first before we can get down here. However, + // that is a bit tricky since workInProgress and current can have + // different "hidden" settings. + let child = workInProgress.progressedChild; + while (child) { + // To ensure that this subtree gets its priority reset, the children + // need to be reset. + child.pendingWorkPriority = OffscreenPriority; + child = child.sibling; + } + return null; + } + // TODO: We should ideally be able to bail out early if the children have no // more work to do. However, since we don't have a separation of this // Fiber's priority and its children yet - we don't know without doing lots diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 9982a078e80c..4e48cefe59c7 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -601,6 +601,113 @@ describe('ReactIncrementalSideEffects', () => { expect(ops).toEqual(['Baz', 'Bar', 'Baz', 'Bar', 'Bar']); }); + it('deprioritizes setStates that happens within a deprioritized tree', () => { + var ops = []; + + var barInstances = []; + + class Bar extends React.Component { + constructor() { + super(); + this.state = { active: false }; + barInstances.push(this); + } + activate() { + this.setState({ active: true }); + } + render() { + ops.push('Bar'); + return ; + } + } + function Foo(props) { + ops.push('Foo'); + return ( +
+ + +
+ ); + } + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + div( + span(0), + div( + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']); + + ops = []; + + ReactNoop.render(); + ReactNoop.flushDeferredPri(70); + expect(ReactNoop.root.children).toEqual([ + div( + // Updated. + span(1), + div( + // Still not updated. + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Foo', 'Bar', 'Bar']); + ops = []; + + barInstances[0].activate(); + + // This should not be enough time to render the content of all the hidden + // items. Including the set state since that is deprioritized. + // TODO: The cycles it takes to do this could be lowered with further + // optimizations. + ReactNoop.flushDeferredPri(60); + expect(ReactNoop.root.children).toEqual([ + div( + // Updated. + span(1), + div( + // Still not updated. + span(0), + span(0), + span(0) + ) + ), + ]); + + expect(ops).toEqual(['Bar']); + ops = []; + + // However, once we render fully, we will have enough time to finish it all + // at once. + ReactNoop.flush(); + expect(ReactNoop.root.children).toEqual([ + div( + span(1), + div( + // Now we had enough time to finish the spans. + span('X'), + span(0), + span(0), + ) + ), + ]); + + expect(ops).toEqual(['Bar', 'Bar']); + }); // TODO: Test that side-effects are not cut off when a work in progress node // moves to "current" without flushing due to having lower priority. Does this // even happen? Maybe a child doesn't get processed because it is lower prio? From 23857e8fb7b8303632357f216181c99bc066caed Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 19 Oct 2016 10:28:39 -0700 Subject: [PATCH 3/3] Bump idx in unit test This proves that this number is actually deprioritized. --- .../fiber/__tests__/ReactIncrementalSideEffects-test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 4e48cefe59c7..81a1e1c09321 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -650,7 +650,7 @@ describe('ReactIncrementalSideEffects', () => { ops = []; - ReactNoop.render(); + ReactNoop.render(); ReactNoop.flushDeferredPri(70); expect(ReactNoop.root.children).toEqual([ div( @@ -700,8 +700,8 @@ describe('ReactIncrementalSideEffects', () => { div( // Now we had enough time to finish the spans. span('X'), - span(0), - span(0), + span(1), + span(1), ) ), ]);