From 8b0be68eee8b3a05c0b4d64e9a01059af7aa8237 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 1 Sep 2016 21:04:44 -0700 Subject: [PATCH] Progressed work --- src/renderers/noop/ReactNoop.js | 16 +- src/renderers/shared/fiber/ReactChildFiber.js | 11 +- src/renderers/shared/fiber/ReactFiber.js | 34 +++- .../shared/fiber/ReactFiberBeginWork.js | 173 ++++++++---------- .../shared/fiber/ReactFiberCompleteWork.js | 8 +- .../shared/fiber/ReactFiberScheduler.js | 36 ++-- .../ReactIncrementalSideEffects-test.js | 79 ++++++++ 7 files changed, 214 insertions(+), 143 deletions(-) diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index 82e64b5357..390c4f6ac3 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -170,16 +170,12 @@ var ReactNoop = { ' '.repeat(depth) + '- ' + (fiber.type ? fiber.type.name || fiber.type : '[root]'), '[' + fiber.pendingWorkPriority + (fiber.pendingProps ? '*' : '') + ']' ); - const childInProgress = fiber.childInProgress; - if (childInProgress) { - if (childInProgress === fiber.child) { - console.log(' '.repeat(depth + 1) + 'ERROR: IN PROGRESS == CURRENT'); - } else { - console.log(' '.repeat(depth + 1) + 'IN PROGRESS'); - logFiber(childInProgress, depth + 1); - if (fiber.child) { - console.log(' '.repeat(depth + 1) + 'CURRENT'); - } + const childInProgress = fiber.progressedChild; + if (childInProgress && childInProgress !== fiber.child) { + console.log(' '.repeat(depth + 1) + 'IN PROGRESS: ' + fiber.progressedPriority); + logFiber(childInProgress, depth + 1); + if (fiber.child) { + console.log(' '.repeat(depth + 1) + 'CURRENT'); } } if (fiber.child) { diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 8a3ad31d41..699b0e7883 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -67,7 +67,7 @@ function ChildReconciler(shouldClone) { clone.pendingWorkPriority = priority; } clone.pendingProps = element.props; - clone.child = existingChild.child; + // clone.child = existingChild.child; clone.sibling = null; clone.return = returnFiber; previousSibling.sibling = clone; @@ -139,7 +139,7 @@ function ChildReconciler(shouldClone) { clone.pendingWorkPriority = priority; } clone.pendingProps = element.props; - clone.child = existingChild.child; + // clone.child = existingChild.child; clone.sibling = null; clone.return = returnFiber; return clone; @@ -246,9 +246,6 @@ exports.cloneChildFibers = function(workInProgress : Fiber) { // If the children of the alternate fiber is a different set, then we don't // need to clone. We need to reset the return fiber though since we'll // traverse down into them. - // TODO: I don't think it is actually possible for them to be anything but - // equal at this point because this fiber was just cloned. Can we skip this - // check? Similar question about the return fiber. let child = workInProgress.child; while (child) { child.return = workInProgress; @@ -265,7 +262,7 @@ exports.cloneChildFibers = function(workInProgress : Fiber) { // than the next sibling. At that point we should add tests that catches // this. - const currentChild = current.child; + const currentChild = workInProgress.child; if (!currentChild) { return; } @@ -274,4 +271,4 @@ exports.cloneChildFibers = function(workInProgress : Fiber) { currentChild.pendingWorkPriority ); cloneSiblings(currentChild, workInProgress.child, workInProgress); -} +}; diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 453ee56d5f..967c6f78b0 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -92,6 +92,17 @@ export type Fiber = Instance & { // This will be used to quickly determine if a subtree has no pending changes. pendingWorkPriority: PriorityLevel, + // This value represents the priority level that was last used to process this + // component. This indicates whether it is better to continue from the + // progressed work or if it is better to continue from the current state. + progressedPriority: PriorityLevel, + + // If work bails out on a Fiber that already had some work started at a lower + // priority, then we need to store the progressed work somewhere. This holds + // the started child set until we need to get back to working on it. It may + // or may not be the same as the "current" child. + progressedChild: ?Fiber, + // This is a pooled version of a Fiber. Every fiber that gets updated will // eventually have a pair. There are cases when we can clean up pairs to save // memory if we need to. @@ -147,6 +158,8 @@ var createFiber = function(tag : TypeOfWork, key : null | string) : Fiber { lastEffect: null, pendingWorkPriority: NoWork, + progressedPriority: NoWork, + progressedChild: null, alternate: null, @@ -158,7 +171,16 @@ function shouldConstruct(Component) { } // This is used to create an alternate fiber to do work on. +// TODO: Rename to createWorkInProgressFiber or something like that. exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fiber { + // We clone to get a work in progress. That means that this fiber is the + // current. To make it safe to reuse that fiber later on as work in progress + // we need to reset its work in progress flag now. We don't have an + // opportunity to do this earlier since we don't traverse the tree when + // the work in progress tree becomes the current tree. + // fiber.progressedPriority = NoWork; + // fiber.progressedChild = null; + // We use a double buffering pooling technique because we know that we'll only // ever need at most two versions of a tree. We pool the "other" unused node // that we're free to reuse. This is lazily created to avoid allocating extra @@ -167,12 +189,12 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi let alt = fiber.alternate; if (alt) { alt.stateNode = fiber.stateNode; - alt.child = fiber.child; - alt.sibling = fiber.sibling; + alt.sibling = fiber.sibling; // This should always be overridden. TODO: null alt.ref = fiber.ref; - alt.pendingProps = fiber.pendingProps; + alt.pendingProps = fiber.pendingProps; // TODO: Pass as argument. alt.pendingWorkPriority = priorityLevel; + alt.child = fiber.child; alt.memoizedProps = fiber.memoizedProps; alt.output = fiber.output; @@ -190,15 +212,19 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi alt.type = fiber.type; alt.stateNode = fiber.stateNode; alt.child = fiber.child; - alt.sibling = fiber.sibling; + alt.sibling = fiber.sibling; // This should always be overridden. TODO: null alt.ref = fiber.ref; // pendingProps is here for symmetry but is unnecessary in practice for now. + // TODO: Pass in the new pendingProps as an argument maybe? alt.pendingProps = fiber.pendingProps; alt.pendingWorkPriority = priorityLevel; alt.memoizedProps = fiber.memoizedProps; alt.output = fiber.output; + alt.progressedChild = fiber.progressedChild; + alt.progressedPriority = fiber.progressedPriority; + alt.alternate = fiber; fiber.alternate = alt; return alt; diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index ddcf88f32a..feabc7838b 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -15,6 +15,7 @@ import type { ReactCoroutine } from 'ReactCoroutine'; import type { Fiber } from 'ReactFiber'; import type { HostConfig } from 'ReactFiberReconciler'; +import type { PriorityLevel } from 'ReactPriorityLevel'; var { reconcileChildFibers, @@ -39,8 +40,24 @@ var { module.exports = function(config : HostConfig) { + function markChildAsProgressed(current, workInProgress, priorityLevel) { + // We now have clones. Let's store them as the currently progressed work. + workInProgress.progressedChild = workInProgress.child; + workInProgress.progressedPriority = priorityLevel; + if (current) { + // We also store it on the current. When the alternate swaps in we can + // continue from this point. + current.progressedChild = workInProgress.progressedChild; + current.progressedPriority = workInProgress.progressedPriority; + } + } + function reconcileChildren(current, workInProgress, nextChildren) { const priorityLevel = workInProgress.pendingWorkPriority; + reconcileChildrenAtPriority(current, workInProgress, nextChildren, priorityLevel); + } + + function reconcileChildrenAtPriority(current, workInProgress, nextChildren, priorityLevel) { // At this point any memoization is no longer valid since we'll have changed // the children. workInProgress.memoizedProps = null; @@ -66,6 +83,7 @@ module.exports = function(config : HostConfig) { priorityLevel ); } + markChildAsProgressed(current, workInProgress, priorityLevel); } function updateFunctionalComponent(current, workInProgress) { @@ -94,20 +112,13 @@ module.exports = function(config : HostConfig) { var ctor = workInProgress.type; workInProgress.stateNode = instance = new ctor(props); } else if (typeof instance.shouldComponentUpdate === 'function') { - if (current && current.memoizedProps) { - // Revert to the last flushed props, incase we aborted an update. - instance.props = current.memoizedProps; - if (!instance.shouldComponentUpdate(props)) { - return bailoutOnCurrent(current, workInProgress); - } - } - if (workInProgress.memoizedProps) { + if (workInProgress.memoizedProps !== null) { // Reset the props, in case this is a ping-pong case rather than a // completed update case. For the completed update case, the instance // props will already be the memoizedProps. instance.props = workInProgress.memoizedProps; if (!instance.shouldComponentUpdate(props)) { - // return bailoutOnAlreadyFinishedWork(current, workInProgress); + return bailoutOnAlreadyFinishedWork(current, workInProgress); } } } @@ -120,14 +131,29 @@ module.exports = function(config : HostConfig) { } function updateHostComponent(current, workInProgress) { + const nextChildren = workInProgress.pendingProps.children; if (workInProgress.pendingProps.hidden && workInProgress.pendingWorkPriority !== OffscreenPriority) { - // If this host component is hidden, we can bail out and ignore this. - // We'll rerender it later at the lower priority. - workInProgress.pendingWorkPriority = OffscreenPriority; - return bailoutOnLowPriority(current, workInProgress); + // If this host component is hidden, we can bail out on the children. + // We'll rerender the children later at the lower priority. + + // It is unfortunate that we have to do the reconciliation of these + // children already since that will add them to the tree even though + // they are not actually done yet. If this is a large set it is also + // confusing that this takes time to do right now instead of later. + + if (workInProgress.progressedPriority === OffscreenPriority) { + // If we already made some progress on the offscreen priority before, + // then we should continue from where we left off. + workInProgress.child = workInProgress.progressedChild; + } + + // Reconcile the children and stash them for later work. + reconcileChildrenAtPriority(current, workInProgress, nextChildren, OffscreenPriority); + workInProgress.child = current ? current.child : null; + // Abort and don't process children yet. + return null; } else { - var nextChildren = workInProgress.pendingProps.children; reconcileChildren(current, workInProgress, nextChildren); return workInProgress.child; } @@ -163,31 +189,7 @@ module.exports = function(config : HostConfig) { reconcileChildren(current, workInProgress, coroutine.children); } - function reuseChildren(returnFiber : Fiber, firstChild : Fiber) { - // TODO on the TODO: Is this not necessary anymore because I moved the - // priority reset? - // TODO: None of this should be necessary if structured better. - // The returnFiber pointer only needs to be updated when we walk into this child - // which we don't do right now. If the pending work priority indicated only - // if a child has work rather than if the node has work, then we would know - // by a single lookup on workInProgress rather than having to go through - // each child. - let child = firstChild; - do { - // Update the returnFiber of the child to the newest fiber. - child.return = returnFiber; - // Retain the priority if there's any work left to do in the children. - /*if (child.pendingWorkPriority !== NoWork && - (returnFiber.pendingWorkPriority === NoWork || - returnFiber.pendingWorkPriority > child.pendingWorkPriority)) { - returnFiber.pendingWorkPriority = child.pendingWorkPriority; - }*/ - if (!child.pendingProps && !child.memoizedProps) { - throw new Error('Should have memoized props by now'); - } - } while (child = child.sibling); - } - + /* function reuseChildrenEffects(returnFiber : Fiber, firstChild : Fiber) { let child = firstChild; do { @@ -204,56 +206,28 @@ module.exports = function(config : HostConfig) { } } while (child = child.sibling); } - -/* - function bailoutOnCurrent(current : Fiber, workInProgress : Fiber) : ?Fiber { - // The most likely scenario is that the previous copy of the tree contains - // the same props as the new one. In that case, we can just copy the output - // and children from that node. - workInProgress.memoizedProps = workInProgress.pendingProps; - workInProgress.output = current.output; - const priorityLevel = workInProgress.pendingWorkPriority; - // workInProgress.pendingProps = null; - workInProgress.stateNode = current.stateNode; - - workInProgress.nextEffect = null; - workInProgress.firstEffect = null; - workInProgress.lastEffect = null; - - workInProgress.child = current.child; - - cloneChildFibers(workInProgress); - - // TODO: Maybe bailout with null if the children priority flag indicate - // that there is no nested work. - return workInProgress.child; - } -*/ + */ function bailoutOnAlreadyFinishedWork(current, workInProgress : Fiber) : ?Fiber { - // If we started this work before, and finished it, or if we're in a - // ping-pong update scenario, this version could already be what we're - // looking for. In that case, we should be able to just bail out. const priorityLevel = workInProgress.pendingWorkPriority; - // workInProgress.pendingProps = null; - workInProgress.firstEffect = null; - workInProgress.nextEffect = null; - workInProgress.lastEffect = 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 + // of the same work we do anyway. Once we have that separation we can just + // bail out here if the children has no more work at this priority level. + // if (workInProgress.priorityOfChildren <= priorityLevel) { + // // If there are side-effects in these children that have not yet been + // // committed we need to ensure that they get properly transferred up. + // if (current && current.child !== workInProgress.child) { + // reuseChildrenEffects(workInProgress, child); + // } + // return null; + // } - const child = workInProgress.child; - if (child) { - // Ensure that the effects of reused work are preserved. - reuseChildrenEffects(workInProgress, child); - // If we bail out but still has work with the current priority in this - // subtree, we need to go find it right now. If we don't, we won't flush - // it until the next tick. - reuseChildren(workInProgress, child); - // TODO: Maybe bailout with null if the children priority flag indicate - // that there is no nested work. - return workInProgress.child; - } - return null; + cloneChildFibers(workInProgress); + markChildAsProgressed(current, workInProgress, priorityLevel); + return workInProgress.child; } function bailoutOnLowPriority(current, workInProgress) { @@ -265,26 +239,22 @@ module.exports = function(config : HostConfig) { return null; } - function beginWork(current : ?Fiber, workInProgress : Fiber, priorityLevel) : ?Fiber { - if (!workInProgress.pendingProps) { - throw new Error('should have pending props here'); - } - + function beginWork(current : ?Fiber, workInProgress : Fiber, priorityLevel : PriorityLevel) : ?Fiber { if (workInProgress.pendingWorkPriority === NoWork || workInProgress.pendingWorkPriority > priorityLevel) { return bailoutOnLowPriority(current, workInProgress); } - // The current, flushed, state of this fiber is the alternate. - // Ideally nothing should rely on this, but relying on it here - // means that we don't need an additional field on the work in - // progress. - if (current && workInProgress.pendingProps === current.memoizedProps) { - return bailoutOnCurrent(current, workInProgress); + if (workInProgress.progressedPriority === priorityLevel) { + // If we have progressed work on this priority level already, we can + // proceed this that as the child. + workInProgress.child = workInProgress.progressedChild; } - if (workInProgress.memoizedProps && - workInProgress.pendingProps === workInProgress.memoizedProps) { + if (workInProgress.pendingProps === null || ( + workInProgress.memoizedProps !== null && + workInProgress.pendingProps === workInProgress.memoizedProps + )) { return bailoutOnAlreadyFinishedWork(current, workInProgress); } @@ -302,7 +272,8 @@ module.exports = function(config : HostConfig) { if (workInProgress.child) { return beginWork( workInProgress.child.alternate, - workInProgress.child + workInProgress.child, + priorityLevel ); } return null; @@ -322,7 +293,8 @@ module.exports = function(config : HostConfig) { if (workInProgress.child) { return beginWork( workInProgress.child.alternate, - workInProgress.child + workInProgress.child, + priorityLevel ); } return workInProgress.child; @@ -332,7 +304,8 @@ module.exports = function(config : HostConfig) { if (workInProgress.sibling) { return beginWork( workInProgress.sibling.alternate, - workInProgress.sibling + workInProgress.sibling, + priorityLevel ); } return null; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index 53421116b4..14e95a0d73 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -162,10 +162,16 @@ module.exports = function(config : HostConfig) { // This returns true if there was something to update. markForPreEffect(workInProgress); } + // TODO: Is this actually ever going to change? Why set it every time? workInProgress.output = instance; } else { if (!newProps) { - throw new Error('We must have new props for new mounts.'); + if (workInProgress.stateNode === null) { + throw new Error('We must have new props for new mounts.'); + } else { + // This can happen when we abort work. + return null; + } } const instance = createInstance(workInProgress.type, newProps, children); // TODO: This seems like unnecessary duplication. diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 96da456351..057f9f7455 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -25,9 +25,6 @@ var { cloneFiber } = require('ReactFiber'); var { NoWork, - HighPriority, - LowPriority, - OffscreenPriority, } = require('ReactPriorityLevel'); var timeHeuristicForUnitOfWork = 1; @@ -105,7 +102,10 @@ module.exports = function(config : HostConfig) { function resetWorkPriority(workInProgress : Fiber) { let newPriority = NoWork; - let child = workInProgress.child; + // progressedChild is going to be the child set with the highest priority. + // Either it is the same as child, or it just bailed out because it choose + // not to do the work. + let child = workInProgress.progressedChild; while (child) { // Ensure that remaining work priority bubbles up. if (child.pendingWorkPriority !== NoWork && @@ -125,25 +125,13 @@ module.exports = function(config : HostConfig) { // means that we don't need an additional field on the work in // progress. const current = workInProgress.alternate; - let next = null; + const next = completeWork(current, workInProgress); - // If this bailed at a lower priority. - // TODO: This branch is currently needed if a particular type of component - // ends up being a priority lowering. We should probably know that already - // before entering begin work. - if (workInProgress.pendingWorkPriority === NoWork || - workInProgress.pendingWorkPriority > nextPriorityLevel) { - // This fiber was ignored. We need to fall through to the next fiber - // and leave the pending props and work untouched on this fiber. - } else { - next = completeWork(current, workInProgress); + resetWorkPriority(workInProgress); - resetWorkPriority(workInProgress); - - // The work is now done. We don't need this anymore. This flags - // to the system not to redo any work here. - workInProgress.pendingProps = null; - } + // The work is now done. We don't need this anymore. This flags + // to the system not to redo any work here. + workInProgress.pendingProps = null; const returnFiber = workInProgress.return; @@ -175,6 +163,12 @@ module.exports = function(config : HostConfig) { } else { // If we're at the root, there's no more work to do. We can flush it. const root : FiberRoot = (workInProgress.stateNode : any); + if (root.current === workInProgress) { + throw new Error( + 'Cannot commit the same tree as before. This is probably a bug ' + + 'related to the return field.' + ); + } root.current = workInProgress; // TODO: We can be smarter here and only look for more work in the // "next" scheduled work since we've already scanned passed. That diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index fe8c2f2d71..63983720df 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js @@ -270,6 +270,85 @@ describe('ReactIncrementalSideEffects', () => { ]); }); + it('can defer side-effects and resume them later on', function() { + class Bar extends React.Component { + shouldComponentUpdate(nextProps) { + return this.props.idx !== nextProps; + } + render() { + return ; + } + } + function Foo(props) { + return ( +
+ + +
+ ); + } + ReactNoop.render(); + ReactNoop.flushLowPri(40 + 25); + expect(ReactNoop.root.children).toEqual([ + div( + span(0), + div(/*the spans are down-prioritized and not rendered yet*/) + ), + ]); + ReactNoop.render(); + ReactNoop.flushLowPri(35 + 25); + expect(ReactNoop.root.children).toEqual([ + div( + span(1), + div(/*still not rendered yet*/) + ), + ]); + ReactNoop.flushLowPri(30 + 25); + expect(ReactNoop.root.children).toEqual([ + div( + span(1), + div( + // Now we had enough time to finish the spans. + span(0), + span(1) + ) + ), + ]); + var innerSpanA = ReactNoop.root.children[0].children[1].children[1]; + ReactNoop.render(); + ReactNoop.flushLowPri(30 + 25); + expect(ReactNoop.root.children).toEqual([ + div( + span(2), + div( + // Still same old numbers. + span(0), + span(1) + ) + ), + ]); + ReactNoop.flushLowPri(30); + expect(ReactNoop.root.children).toEqual([ + div( + span(2), + div( + // New numbers. + span(1), + span(2) + ) + ), + ]); + + var innerSpanB = ReactNoop.root.children[0].children[1].children[1]; + // This should have been an update to an existing instance, not recreation. + // We verify that by ensuring that the child instance was the same as + // before. + expect(innerSpanA).toBe(innerSpanB); + }); + // 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