From 78a673d335dc53d203c1116a4630bc1e3fadcbb0 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 8 Aug 2016 19:22:45 -0700 Subject: [PATCH] Move priority reset to the end and search pending work in work tree The priority level gets reset at the wrong time because I rely on mutating it at the wrong point. This moves it to the end of completed work as a second pass over the children to see what the highest priority is. This is inefficient for large sets but we can try to find a smarter way to do this later. E.g. passing it down the stack. This bug fix revealed another bug that I had flagged before that we're finding work to do in the "current" tree instead of the working tree. For trees that were paused, e.g. childInProgress trees, this won't work since don't have a current tree to search. Therefore I fixed findNextUnitOfWorkAtPriority to use workInProgress instead of current. --- src/renderers/shared/fiber/ReactChildFiber.js | 2 + src/renderers/shared/fiber/ReactFiber.js | 6 + .../shared/fiber/ReactFiberBeginWork.js | 34 ++--- .../shared/fiber/ReactFiberPendingWork.js | 130 ++++++++++-------- .../shared/fiber/ReactFiberReconciler.js | 2 + .../shared/fiber/ReactFiberScheduler.js | 34 +++-- 6 files changed, 117 insertions(+), 91 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index c3621c30ad2a..186a8beaa0ac 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -65,6 +65,7 @@ function ChildReconciler(shouldClone) { // Will fix reconciliation properly later. const clone = shouldClone ? cloneFiber(existingChild, priority) : existingChild; if (!shouldClone) { + // TODO: This might be lowering the priority of nested unfinished work. clone.pendingWorkPriority = priority; } clone.pendingProps = element.props; @@ -132,6 +133,7 @@ function ChildReconciler(shouldClone) { // Get the clone of the existing fiber. const clone = shouldClone ? cloneFiber(existingChild, priority) : existingChild; if (!shouldClone) { + // TODO: This might be lowering the priority of nested unfinished work. clone.pendingWorkPriority = priority; } clone.pendingProps = element.props; diff --git a/src/renderers/shared/fiber/ReactFiber.js b/src/renderers/shared/fiber/ReactFiber.js index 2bcfe3f496b2..09b6dabc0c12 100644 --- a/src/renderers/shared/fiber/ReactFiber.js +++ b/src/renderers/shared/fiber/ReactFiber.js @@ -170,6 +170,9 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi alt.pendingProps = fiber.pendingProps; alt.pendingWorkPriority = priorityLevel; + alt.memoizedProps = fiber.memoizedProps; + alt.output = fiber.output; + // Whenever we clone, we do so to get a new work in progress. // This ensures that we've reset these in the new tree. alt.nextEffect = null; @@ -191,6 +194,9 @@ exports.cloneFiber = function(fiber : Fiber, priorityLevel : PriorityLevel) : Fi alt.pendingProps = fiber.pendingProps; alt.pendingWorkPriority = priorityLevel; + alt.memoizedProps = fiber.memoizedProps; + alt.output = fiber.output; + 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 6c664784f07b..c5e63336420b 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -40,6 +40,8 @@ var { findNextUnitOfWorkAtPriority } = require('ReactFiberPendingWork'); module.exports = function(config : HostConfig) { function reconcileChildren(current, workInProgress, nextChildren) { + // TODO: Children needs to be able to reconcile in place if we are + // overriding progressed work. const priority = workInProgress.pendingWorkPriority; reconcileChildrenAtPriority(current, workInProgress, nextChildren, priority); } @@ -76,7 +78,6 @@ module.exports = function(config : HostConfig) { var props = workInProgress.pendingProps; var nextChildren = fn(props); reconcileChildren(current, workInProgress, nextChildren); - workInProgress.pendingWorkPriority = NoWork; } function updateClassComponent(current : ?Fiber, workInProgress : Fiber) { @@ -106,7 +107,6 @@ module.exports = function(config : HostConfig) { instance.props = props; var nextChildren = instance.render(); reconcileChildren(current, workInProgress, nextChildren); - workInProgress.pendingWorkPriority = NoWork; return workInProgress.childInProgress; } @@ -125,11 +125,9 @@ module.exports = function(config : HostConfig) { // becomes part of the render tree, even though it never completed. Its // `output` property is unpredictable because of it. reconcileChildrenAtPriority(current, workInProgress, nextChildren, OffscreenPriority); - workInProgress.pendingWorkPriority = OffscreenPriority; return null; } else { reconcileChildren(current, workInProgress, nextChildren); - workInProgress.pendingWorkPriority = NoWork; return workInProgress.childInProgress; } } @@ -153,7 +151,6 @@ module.exports = function(config : HostConfig) { } } reconcileChildren(current, workInProgress, value); - workInProgress.pendingWorkPriority = NoWork; } function updateCoroutineComponent(current, workInProgress) { @@ -162,10 +159,11 @@ module.exports = function(config : HostConfig) { throw new Error('Should be resolved by now'); } reconcileChildren(current, workInProgress, coroutine.children); - workInProgress.pendingWorkPriority = NoWork; } 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 @@ -210,7 +208,6 @@ module.exports = function(config : HostConfig) { workInProgress.output = current.output; const priorityLevel = workInProgress.pendingWorkPriority; workInProgress.pendingProps = null; - workInProgress.pendingWorkPriority = NoWork; workInProgress.stateNode = current.stateNode; workInProgress.childInProgress = current.childInProgress; if (current.child) { @@ -220,10 +217,10 @@ module.exports = function(config : HostConfig) { workInProgress.child = current.child; reuseChildren(workInProgress, workInProgress.child); if (workInProgress.pendingWorkPriority !== NoWork && workInProgress.pendingWorkPriority <= priorityLevel) { - // TODO: This passes the current node and reads the priority level and - // pending props from that. We want it to read our priority level and - // pending props from the work in progress. Needs restructuring. - return findNextUnitOfWorkAtPriority(current, priorityLevel); + return findNextUnitOfWorkAtPriority( + workInProgress, + workInProgress.pendingWorkPriority + ); } else { return null; } @@ -239,22 +236,13 @@ module.exports = function(config : HostConfig) { // looking for. In that case, we should be able to just bail out. const priorityLevel = workInProgress.pendingWorkPriority; workInProgress.pendingProps = null; - workInProgress.pendingWorkPriority = NoWork; workInProgress.firstEffect = null; workInProgress.nextEffect = null; workInProgress.lastEffect = null; - if (workInProgress.child) { - // On the way up here, we reset the child node to be the current one by - // cloning. However, it is really the original child that represents the - // already completed work. Therefore we have to reuse the alternate. - // But if we don't have a current, this was not cloned. This is super weird. - const child = !current ? workInProgress.child : workInProgress.child.alternate; - if (!child) { - throw new Error('We must have a current child to be able to use this.'); - } - workInProgress.child = child; + 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 @@ -299,7 +287,6 @@ module.exports = function(config : HostConfig) { reconcileChildren(current, workInProgress, workInProgress.pendingProps); // A yield component is just a placeholder, we can just run through the // next one immediately. - workInProgress.pendingWorkPriority = NoWork; if (workInProgress.childInProgress) { return beginWork( workInProgress.childInProgress.alternate, @@ -327,7 +314,6 @@ module.exports = function(config : HostConfig) { case YieldComponent: // A yield component is just a placeholder, we can just run through the // next one immediately. - workInProgress.pendingWorkPriority = NoWork; if (workInProgress.sibling) { return beginWork( workInProgress.sibling.alternate, diff --git a/src/renderers/shared/fiber/ReactFiberPendingWork.js b/src/renderers/shared/fiber/ReactFiberPendingWork.js index eb61c4595fff..fff76afec76e 100644 --- a/src/renderers/shared/fiber/ReactFiberPendingWork.js +++ b/src/renderers/shared/fiber/ReactFiberPendingWork.js @@ -34,20 +34,50 @@ function cloneSiblings(current : Fiber, workInProgress : Fiber, returnFiber : Fi workInProgress.sibling = null; } -exports.findNextUnitOfWorkAtPriority = function(currentRoot : Fiber, priorityLevel : PriorityLevel) : ?Fiber { - let current = currentRoot; - while (current) { - if (current.pendingWorkPriority !== NoWork && - current.pendingWorkPriority <= priorityLevel) { +function cloneChildrenIfNeeded(workInProgress : Fiber) { + const current = workInProgress.alternate; + if (!current || workInProgress.child !== current.child) { + // If there is no alternate, then we don't need to clone the children. + // 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; + child = child.sibling; + } + return; + } + // TODO: This used to reset the pending priority. Not sure if that is needed. + // workInProgress.pendingWorkPriority = current.pendingWorkPriority; + + // TODO: The below priority used to be set to NoWork which would've + // dropped work. This is currently unobservable but will become + // observable when the first sibling has lower priority work remaining + // than the next sibling. At that point we should add tests that catches + // this. + + const currentChild = current.child; + if (!currentChild) { + return; + } + workInProgress.child = cloneFiber( + currentChild, + currentChild.pendingWorkPriority + ); + cloneSiblings(currentChild, workInProgress.child, workInProgress); +} + +exports.findNextUnitOfWorkAtPriority = function(workRoot : Fiber, priorityLevel : PriorityLevel) : ?Fiber { + let workInProgress = workRoot; + while (workInProgress) { + if (workInProgress.pendingWorkPriority !== NoWork && + workInProgress.pendingWorkPriority <= priorityLevel) { // This node has work to do that fits our priority level criteria. - if (current.pendingProps !== null) { - // We found some work to do. We need to return the "work in progress" - // of this node which will be the alternate. - const workInProgress = current.alternate; - if (!workInProgress) { - throw new Error('Should have wip now'); - } - workInProgress.pendingProps = current.pendingProps; + if (workInProgress.pendingProps !== null) { return workInProgress; } @@ -56,71 +86,57 @@ exports.findNextUnitOfWorkAtPriority = function(currentRoot : Fiber, priorityLev // because it is the highest priority for the whole subtree. // TODO: Coroutines can have work in their stateNode which is another // type of child that needs to be searched for work. - if (current.childInProgress) { - let workInProgress = current.childInProgress; - while (workInProgress) { - workInProgress.return = current.alternate; - workInProgress = workInProgress.sibling; + if (workInProgress.childInProgress) { + let child = workInProgress.childInProgress; + while (child) { + child.return = workInProgress; + child = child.sibling; } - workInProgress = current.childInProgress; - while (workInProgress) { - // Don't bother drilling further down this tree if there is no child. - if (workInProgress.pendingWorkPriority !== NoWork && - workInProgress.pendingWorkPriority <= priorityLevel && - workInProgress.pendingProps !== null) { - return workInProgress; + child = workInProgress.childInProgress; + while (child) { + // Don't bother drilling further down this tree if there is no child + // with more content. + // TODO: Shouldn't this still drill down even though the first + // shallow level doesn't have anything pending on it. + if (child.pendingWorkPriority !== NoWork && + child.pendingWorkPriority <= priorityLevel && + child.pendingProps !== null) { + return child; } - workInProgress = workInProgress.sibling; + child = child.sibling; } - } else if (current.child) { - let currentChild = current.child; - currentChild.return = current; - // Ensure we have a work in progress copy to backtrack through. - let workInProgress = current.alternate; - if (!workInProgress) { - throw new Error('Should have wip now'); - } - workInProgress.pendingWorkPriority = current.pendingWorkPriority; - // TODO: The below priority used to be set to NoWork which would've - // dropped work. This is currently unobservable but will become - // observable when the first sibling has lower priority work remaining - // than the next sibling. At that point we should add tests that catches - // this. - workInProgress.child = cloneFiber( - currentChild, - currentChild.pendingWorkPriority - ); - cloneSiblings(currentChild, workInProgress.child, workInProgress); - current = currentChild; + } else if (workInProgress.child) { + cloneChildrenIfNeeded(workInProgress); + workInProgress = workInProgress.child; continue; } // If we match the priority but has no child and no work to do, // then we can safely reset the flag. - current.pendingWorkPriority = NoWork; + workInProgress.pendingWorkPriority = NoWork; } - if (current === currentRoot) { - if (current.pendingWorkPriority <= priorityLevel) { + if (workInProgress === workRoot) { + if (workInProgress.pendingWorkPriority <= priorityLevel) { // If this subtree had work left to do, we would have returned it by // now. This could happen if a child with pending work gets cleaned up // but we don't clear the flag then. It is safe to reset it now. - current.pendingWorkPriority = NoWork; + workInProgress.pendingWorkPriority = NoWork; } return null; } - while (!current.sibling) { - current = current.return; - if (!current) { + while (!workInProgress.sibling) { + workInProgress = workInProgress.return; + if (!workInProgress || workInProgress === workRoot) { return null; } - if (current.pendingWorkPriority <= priorityLevel) { + if (workInProgress.pendingWorkPriority <= priorityLevel) { // If this subtree had work left to do, we would have returned it by // now. This could happen if a child with pending work gets cleaned up // but we don't clear the flag then. It is safe to reset it now. - current.pendingWorkPriority = NoWork; + workInProgress.pendingWorkPriority = NoWork; } } - current.sibling.return = current.return; - current = current.sibling; + workInProgress.sibling.return = workInProgress.return; + workInProgress = workInProgress.sibling; } return null; }; diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 4ece60a0f2e9..aa39d1b48d36 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -70,6 +70,8 @@ module.exports = function(config : HostConfig) : Reconci const root = createFiberRoot(containerInfo); const container = root.current; // TODO: Use pending work/state instead of props. + // TODO: This should not override the pendingWorkPriority if there is + // higher priority work in the subtree. container.pendingProps = element; container.pendingWorkPriority = LowPriority; diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index b6e71289689e..e469d5c0ca88 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -66,22 +66,25 @@ module.exports = function(config : HostConfig) { // roots for high priority work before moving on to lower priorities. let root = nextScheduledRoot; while (root) { - cloneFiber(root.current, root.current.pendingWorkPriority); + let rootInProgress = cloneFiber( + root.current, + root.current.pendingWorkPriority + ); // Find the highest possible priority work to do. // This loop is unrolled just to satisfy Flow's enum constraint. // We could make arbitrary many idle priority levels but having // too many just means flushing changes too often. - let work = findNextUnitOfWorkAtPriority(root.current, HighPriority); + let work = findNextUnitOfWorkAtPriority(rootInProgress, HighPriority); if (work) { nextPriorityLevel = HighPriority; return work; } - work = findNextUnitOfWorkAtPriority(root.current, LowPriority); + work = findNextUnitOfWorkAtPriority(rootInProgress, LowPriority); if (work) { nextPriorityLevel = LowPriority; return work; } - work = findNextUnitOfWorkAtPriority(root.current, OffscreenPriority); + work = findNextUnitOfWorkAtPriority(rootInProgress, OffscreenPriority); if (work) { nextPriorityLevel = OffscreenPriority; return work; @@ -114,6 +117,21 @@ module.exports = function(config : HostConfig) { } } + function resetWorkPriority(workInProgress : Fiber) { + let newPriority = NoWork; + let child = workInProgress.childInProgress || workInProgress.child; + while (child) { + // Ensure that remaining work priority bubbles up. + if (child.pendingWorkPriority !== NoWork && + (newPriority === NoWork || + newPriority > child.pendingWorkPriority)) { + newPriority = child.pendingWorkPriority; + } + child = child.sibling; + } + workInProgress.pendingWorkPriority = newPriority; + } + function completeUnitOfWork(workInProgress : Fiber) : ?Fiber { while (true) { // The current, flushed, state of this fiber is the alternate. @@ -123,6 +141,8 @@ module.exports = function(config : HostConfig) { const current = workInProgress.alternate; const next = completeWork(current, 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; @@ -130,12 +150,6 @@ module.exports = function(config : HostConfig) { const returnFiber = workInProgress.return; if (returnFiber) { - // Ensure that remaining work priority bubbles up. - if (workInProgress.pendingWorkPriority !== NoWork && - (returnFiber.pendingWorkPriority === NoWork || - returnFiber.pendingWorkPriority > workInProgress.pendingWorkPriority)) { - returnFiber.pendingWorkPriority = workInProgress.pendingWorkPriority; - } // Ensure that the first and last effect of the parent corresponds // to the children's first and last effect. This probably relies on // children completing in order.