From 4b9775eaa9147b6ed337f867442175e80d1086a1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 30 Jun 2017 14:45:03 -0700 Subject: [PATCH] Down-prioritize children of `hidden` host components 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. --- scripts/fiber/tests-passing.txt | 5 + src/renderers/shared/fiber/ReactChildFiber.js | 4 +- src/renderers/shared/fiber/ReactFiber.js | 7 + .../shared/fiber/ReactFiberBeginWork.js | 56 ++++-- .../shared/fiber/ReactFiberCompleteWork.js | 19 +- .../shared/fiber/ReactFiberScheduler.js | 26 +-- .../fiber/__tests__/ReactIncremental-test.js | 6 +- .../__tests__/ReactIncrementalPerf-test.js | 2 +- .../ReactIncrementalSideEffects-test.js | 167 +++++++++--------- 9 files changed, 171 insertions(+), 121 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 18a5546f1ff92..242594a295fd2 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -2023,6 +2023,8 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * updates a previous render * can cancel partially rendered work and restart * should call callbacks even if updates are aborted +* can deprioritize unfinished work and resume it later +* can deprioritize a tree from without dropping work * memoizes work even if shouldComponentUpdate returns false * can update in the middle of a tree using setState * can queue multiple state updates @@ -2035,6 +2037,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * can handle if setState callback throws * merges and masks context * does not leak own context into context provider +* provides context when reusing work * reads context when setState is below the provider * reads context when setState is above the provider * maintains the correct context when providers bail out due to low priority @@ -2078,6 +2081,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js * warns on cascading renders from top-level render * does not treat setState from cWM or cWRP as cascading * captures all lifecycles +* measures deprioritized work * measures deferred work in chunks * recovers from fatal errors * recovers from caught errors @@ -2117,6 +2121,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js * can reuse side-effects after being preempted, if shouldComponentUpdate is false * can update a completed tree before it has a chance to commit * updates a child even though the old props is empty +* deprioritizes setStates that happens within a deprioritized tree * calls callback after update is flushed * calls setState callback even if component bails out * calls componentWillUnmount after a deletion, even if nested diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 95781072577bb..41bd8f3983e6f 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -1463,7 +1463,7 @@ exports.cloneChildFibers = function( let currentChild = workInProgress.child; let newChild = createWorkInProgress(currentChild, renderPriority); // TODO: Pass this as an argument, since it's easy to forget. - newChild.pendingProps = null; + newChild.pendingProps = currentChild.pendingProps; workInProgress.child = newChild; newChild.return = workInProgress; @@ -1473,7 +1473,7 @@ exports.cloneChildFibers = function( currentChild, renderPriority, ); - newChild.pendingProps = null; + newChild.pendingProps = currentChild.pendingProps; newChild.return = workInProgress; } newChild.sibling = null; diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index bfc4e17fe7745..29eeb315691e9 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -451,3 +451,10 @@ exports.createFiberFromPortal = function( }; return fiber; }; + +exports.largerPriority = function( + p1: PriorityLevel, + p2: PriorityLevel, +): PriorityLevel { + return p1 !== NoWork && (p2 === NoWork || p2 > p1) ? p1 : p2; +}; diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index bb97ba5f3b04e..da6240b91d3e1 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -49,7 +49,7 @@ var { YieldComponent, Fragment, } = ReactTypeOfWork; -var {NoWork} = require('ReactPriorityLevel'); +var {NoWork, OffscreenPriority} = require('ReactPriorityLevel'); var { PerformedWork, Placement, @@ -76,7 +76,11 @@ module.exports = function( scheduleUpdate: (fiber: Fiber, priorityLevel: PriorityLevel) => void, getPriorityContext: (fiber: Fiber, forceAsync: boolean) => PriorityLevel, ) { - const {shouldSetTextContent} = config; + const { + shouldSetTextContent, + useSyncScheduling, + shouldDeprioritizeSubtree, + } = config; const {pushHostContext, pushHostContainer} = hostContext; @@ -375,28 +379,29 @@ module.exports = function( return bailoutOnAlreadyFinishedWork(current, workInProgress); } - function updateHostComponent(current, workInProgress) { + function updateHostComponent(current, workInProgress, renderPriority) { pushHostContext(workInProgress); if (current === null) { tryToClaimNextHydratableInstance(workInProgress); } - let nextProps = workInProgress.pendingProps; const type = workInProgress.type; - const prevProps = current !== null ? current.memoizedProps : null; const memoizedProps = workInProgress.memoizedProps; + let nextProps = workInProgress.pendingProps; + if (nextProps === null) { + nextProps = memoizedProps; + invariant( + nextProps !== null, + 'We should always have pending or current props. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); + } + const prevProps = current !== null ? current.memoizedProps : null; + if (hasContextChanged()) { // Normally we can bail out on props equality but if context has changed // we don't do the bailout and we have to reuse existing props instead. - if (nextProps === null) { - nextProps = memoizedProps; - invariant( - nextProps !== null, - 'We should always have pending or current props. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); - } } else if (nextProps === null || memoizedProps === nextProps) { return bailoutOnAlreadyFinishedWork(current, workInProgress); } @@ -418,6 +423,18 @@ module.exports = function( markRef(current, workInProgress); + // Check the host config to see if the children are offscreen/hidden. + if ( + renderPriority !== OffscreenPriority && + !useSyncScheduling && + shouldDeprioritizeSubtree(type, nextProps) + ) { + // Down-prioritize the children. + workInProgress.pendingWorkPriority = OffscreenPriority; + // Bailout and come back to this fiber later at OffscreenPriority. + return null; + } + reconcileChildren(current, workInProgress, nextChildren); memoizeProps(workInProgress, nextProps); return workInProgress.child; @@ -686,10 +703,9 @@ module.exports = function( return null; } + // TODO: Delete memoizeProps/State and move to reconcile/bailout instead function memoizeProps(workInProgress: Fiber, nextProps: any) { workInProgress.memoizedProps = nextProps; - // Reset the pending props - workInProgress.pendingProps = null; } function memoizeState(workInProgress: Fiber, nextState: any) { @@ -728,7 +744,7 @@ module.exports = function( case HostRoot: return updateHostRoot(current, workInProgress, priorityLevel); case HostComponent: - return updateHostComponent(current, workInProgress); + return updateHostComponent(current, workInProgress, priorityLevel); case HostText: return updateHostText(current, workInProgress); case CoroutineHandlerPhase: @@ -793,13 +809,17 @@ module.exports = function( // Unmount the current children as if the component rendered null const nextChildren = null; - reconcileChildren(current, workInProgress, nextChildren); + reconcileChildrenAtPriority( + current, + workInProgress, + nextChildren, + priorityLevel, + ); if (workInProgress.tag === ClassComponent) { const instance = workInProgress.stateNode; workInProgress.memoizedProps = instance.props; workInProgress.memoizedState = instance.state; - workInProgress.pendingProps = null; } return workInProgress.child; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index 3aa2614b32eb2..74ddba49a777e 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -14,6 +14,7 @@ import type {ReactCoroutine} from 'ReactTypes'; import type {Fiber} from 'ReactFiber'; +import type {PriorityLevel} from 'ReactPriorityLevel'; import type {HostContext} from 'ReactFiberHostContext'; import type {HydrationContext} from 'ReactFiberHydrationContext'; import type {FiberRoot} from 'ReactFiberRoot'; @@ -23,6 +24,7 @@ var {reconcileChildFibers} = require('ReactChildFiber'); var {popContextProvider} = require('ReactFiberContext'); var ReactTypeOfWork = require('ReactTypeOfWork'); var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect'); +var ReactPriorityLevel = require('ReactPriorityLevel'); var { IndeterminateComponent, FunctionalComponent, @@ -37,6 +39,7 @@ var { Fragment, } = ReactTypeOfWork; var {Placement, Ref, Update} = ReactTypeOfSideEffect; +var {OffscreenPriority} = ReactPriorityLevel; if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); @@ -181,11 +184,24 @@ module.exports = function( function completeWork( current: Fiber | null, workInProgress: Fiber, + renderPriority: PriorityLevel, ): Fiber | null { if (__DEV__) { ReactDebugCurrentFiber.current = workInProgress; } + // Get the latest props. + let newProps = workInProgress.pendingProps; + if (newProps === null) { + newProps = workInProgress.memoizedProps; + } else if ( + workInProgress.pendingWorkPriority !== OffscreenPriority || + renderPriority === OffscreenPriority + ) { + // Reset the pending props, unless this was a down-prioritization. + workInProgress.pendingProps = null; + } + switch (workInProgress.tag) { case FunctionalComponent: return null; @@ -216,7 +232,6 @@ module.exports = function( popHostContext(workInProgress); const rootContainerInstance = getRootHostContainer(); const type = workInProgress.type; - const newProps = workInProgress.memoizedProps; if (current !== null && workInProgress.stateNode != null) { // If we have an alternate, that means this is an update and we need to // schedule a side-effect to do the updates. @@ -311,7 +326,7 @@ module.exports = function( return null; } case HostText: { - let newText = workInProgress.memoizedProps; + let newText = newProps; if (current && workInProgress.stateNode != null) { const oldText = current.memoizedProps; // If we have an alternate, that means this is an update and we need diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index e8640f619b3b5..5cf59f6a5ed4a 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -48,7 +48,7 @@ var ReactFiberHydrationContext = require('ReactFiberHydrationContext'); var {ReactCurrentOwner} = require('ReactGlobalSharedState'); var getComponentName = require('getComponentName'); -var {createWorkInProgress} = require('ReactFiber'); +var {createWorkInProgress, largerPriority} = require('ReactFiber'); var {onCommitRoot} = require('ReactFiberDevToolsHook'); var { @@ -551,7 +551,18 @@ module.exports = function( priorityContext = previousPriorityContext; } - function resetWorkPriority(workInProgress: Fiber) { + function resetWorkPriority( + workInProgress: Fiber, + renderPriority: PriorityLevel, + ) { + if ( + workInProgress.pendingWorkPriority !== NoWork && + workInProgress.pendingWorkPriority > renderPriority + ) { + // This was a down-prioritization. Don't bubble priority from children. + return; + } + // Check for pending update priority. let newPriority = getUpdatePriority(workInProgress); @@ -560,12 +571,7 @@ module.exports = function( let child = workInProgress.child; while (child !== null) { // Ensure that remaining work priority bubbles up. - if ( - child.pendingWorkPriority !== NoWork && - (newPriority === NoWork || newPriority > child.pendingWorkPriority) - ) { - newPriority = child.pendingWorkPriority; - } + newPriority = largerPriority(newPriority, child.pendingWorkPriority); child = child.sibling; } workInProgress.pendingWorkPriority = newPriority; @@ -578,12 +584,12 @@ module.exports = function( // means that we don't need an additional field on the work in // progress. const current = workInProgress.alternate; - const next = completeWork(current, workInProgress); + const next = completeWork(current, workInProgress, nextPriorityLevel); const returnFiber = workInProgress.return; const siblingFiber = workInProgress.sibling; - resetWorkPriority(workInProgress); + resetWorkPriority(workInProgress, nextPriorityLevel); if (next !== null) { if (__DEV__) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index d85ceb15b1c89..f0c2133488249 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -244,7 +244,7 @@ describe('ReactIncremental', () => { expect(inst.state).toEqual({text: 'bar', text2: 'baz'}); }); - xit('can deprioritize unfinished work and resume it later', () => { + it('can deprioritize unfinished work and resume it later', () => { var ops = []; function Bar(props) { @@ -296,7 +296,7 @@ describe('ReactIncremental', () => { expect(ops).toEqual(['Middle', 'Middle']); }); - xit('can deprioritize a tree from without dropping work', () => { + it('can deprioritize a tree from without dropping work', () => { var ops = []; function Bar(props) { @@ -1962,7 +1962,7 @@ describe('ReactIncremental', () => { ]); }); - xit('provides context when reusing work', () => { + it('provides context when reusing work', () => { var ops = []; class Intl extends React.Component { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js index 2a43e98327bf4..c2b81d19d4f5f 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalPerf-test.js @@ -280,7 +280,7 @@ describe('ReactDebugFiberPerf', () => { expect(getFlameChart()).toMatchSnapshot(); }); - xit('measures deprioritized work', () => { + it('measures deprioritized work', () => { addComment('Flush the parent'); ReactNoop.syncUpdates(() => { ReactNoop.render( diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index 3e3b063ac6e2a..c15489376c537 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -691,109 +691,106 @@ describe('ReactIncrementalSideEffects', () => { expect(ops).toEqual(['Bar', 'Baz', 'Bar', 'Bar']); }); - xit( - '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 ; - } + 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); } - function Foo(props) { - ops.push('Foo'); - return ( -
- - -
- ); + activate() { + this.setState({active: true}); + } + render() { + ops.push('Bar'); + return ; } - ReactNoop.render(); - ReactNoop.flush(); - expect(ReactNoop.getChildren()).toEqual([ - div(span(0), div(span(0), span(0), span(0))), - ]); + } + function Foo(props) { + ops.push('Foo'); + return ( +
+ + +
+ ); + } + ReactNoop.render(); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([ + div(span(0), div(span(0), span(0), span(0))), + ]); - expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']); + expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']); - ops = []; + ops = []; - ReactNoop.render(); - ReactNoop.flushDeferredPri(70 + 5); - expect(ReactNoop.getChildren()).toEqual([ + ReactNoop.render(); + ReactNoop.flushDeferredPri(70 + 5); + expect(ReactNoop.getChildren()).toEqual([ + div( + // Updated. + span(1), div( - // Updated. - span(1), - div( - // Still not updated. - span(0), - span(0), - span(0), - ), + // Still not updated. + span(0), + span(0), + span(0), ), - ]); + ), + ]); - expect(ops).toEqual(['Foo', 'Bar', 'Bar']); - ops = []; + expect(ops).toEqual(['Foo', 'Bar', 'Bar']); + ops = []; - barInstances[0].activate(); + 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 + 5); - expect(ReactNoop.getChildren()).toEqual([ + // 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 + 5); + expect(ReactNoop.getChildren()).toEqual([ + div( + // Updated. + span(1), div( - // Updated. - span(1), - div( - // Still not updated. - span(0), - span(0), - span(0), - ), + // Still not updated. + span(0), + span(0), + span(0), ), - ]); + ), + ]); - expect(ops).toEqual(['Bar']); - ops = []; + 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.getChildren()).toEqual([ + // However, once we render fully, we will have enough time to finish it all + // at once. + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([ + div( + span(1), div( + // Now we had enough time to finish the spans. + span('X'), + span(1), span(1), - div( - // Now we had enough time to finish the spans. - span('X'), - span(1), - span(1), - ), ), - ]); + ), + ]); - expect(ops).toEqual(['Bar']); - }, - ); + 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?