From 3addf205bf6ad1333861d662ec955fcb11db9d24 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 25 Oct 2017 16:51:45 -0700 Subject: [PATCH] Remove Task priority (#11307) * Remove Task priority The concept of Task priority was originally added as a way to avoid reentrancy. Sync priority is for work that flushes synchronously, and Task is for work that flushes at the end of the event loop. But it turns out that in most cases, it's simpler to model Task and Sync as the same priority level. For example, it's never correct to flush Sync work from the update queue without flushing Task. Doing so can lead to infinite update loops. And using a separate priority level is not necessary to avoid reentrancy. We already track when work is being rendered, and exit before entering the render cycle again. That alone is sufficient. This commit removes Task priority from the codebase. Now we use the same level for both truly synchronous work and work that is deferred until the end of the event loop. This also enables us to remove DOM-specific legacy cases from the reconciler and lift them to the renderer. * Remove isUnbatched from FiberRoot Simpler to render unbatched roots immediately. * Use a cyclic linked list for root schedule Avoids the need for a separate `isScheduled` field. --- .../src/ReactFiberExpirationTime.js | 4 +- .../src/ReactFiberScheduler.js | 242 ++++++++++-------- 2 files changed, 137 insertions(+), 109 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberExpirationTime.js b/packages/react-reconciler/src/ReactFiberExpirationTime.js index 7a4e599f36cb5..64a7274051c79 100644 --- a/packages/react-reconciler/src/ReactFiberExpirationTime.js +++ b/packages/react-reconciler/src/ReactFiberExpirationTime.js @@ -14,14 +14,12 @@ export type ExpirationTime = number; const NoWork = 0; const Sync = 1; -const Task = 2; const Never = 2147483647; // Max int32: Math.pow(2, 31) - 1 const UNIT_SIZE = 10; -const MAGIC_NUMBER_OFFSET = 3; +const MAGIC_NUMBER_OFFSET = 2; exports.Sync = Sync; -exports.Task = Task; exports.NoWork = NoWork; exports.Never = Never; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 5c5e25a65b760..67a87c9a4a3f8 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -56,7 +56,6 @@ var {createWorkInProgress} = require('./ReactFiber'); var {onCommitRoot} = require('./ReactFiberDevToolsHook'); var { NoWork, - Task, Sync, Never, msToExpirationTime, @@ -1172,15 +1171,6 @@ module.exports = function( expirationTime = computeAsyncExpiration(); } } - - if ( - expirationTime === Sync && - isBatchingUpdates && - (!isUnbatchingUpdates || isCommitting) - ) { - // If we're in a batch, downgrade sync to task. - expirationTime = Task; - } return expirationTime; } @@ -1250,7 +1240,7 @@ module.exports = function( } function scheduleErrorRecovery(fiber: Fiber) { - scheduleWorkImpl(fiber, Task, true); + scheduleWorkImpl(fiber, Sync, true); } function recalculateCurrentTime(): ExpirationTime { @@ -1318,15 +1308,18 @@ module.exports = function( ); } + // Add the root to the schedule. // Check if this root is already part of the schedule. - if (root.remainingExpirationTime === NoWork) { + if (root.nextScheduledRoot === null) { // This root is not already scheduled. Add it. root.remainingExpirationTime = expirationTime; if (lastScheduledRoot === null) { firstScheduledRoot = lastScheduledRoot = root; + root.nextScheduledRoot = root; } else { lastScheduledRoot.nextScheduledRoot = root; lastScheduledRoot = root; + lastScheduledRoot.nextScheduledRoot = firstScheduledRoot; } } else { // This root is already scheduled, but its priority may have increased. @@ -1340,21 +1333,28 @@ module.exports = function( } } - // If we're not already rendering, schedule work to flush now (if it's - // sync) or later (if it's async). - if (!isRendering) { - // TODO: Remove distinction between sync and task. Maybe we can remove - // these magic numbers entirely by always comparing to the current time? - if (expirationTime === Sync) { - if (isUnbatchingUpdates) { - performWork(Sync, null); - } else { - performWork(Task, null); - } - } else if (!isCallbackScheduled) { - isCallbackScheduled = true; - scheduleDeferredCallback(flushAsyncWork); + if (isRendering) { + // Prevent reentrancy. Remaining work will be scheduled at the end of + // the currently rendering batch. + return; + } + + if (isBatchingUpdates) { + // Flush work at the end of the batch. + if (isUnbatchingUpdates) { + // ...unless we're inside unbatchedUpdates, in which case we should + // flush it now. + performWorkOnRoot(root, Sync); } + return; + } + + // TODO: Get rid of Sync and use current time? + if (expirationTime === Sync) { + performWork(Sync, null); + } else if (!isCallbackScheduled) { + isCallbackScheduled = true; + scheduleDeferredCallback(performAsyncWork); } } @@ -1362,34 +1362,60 @@ module.exports = function( let highestPriorityWork = NoWork; let highestPriorityRoot = null; - let previousScheduledRoot = null; - let root = firstScheduledRoot; - while (root !== null) { - const remainingExpirationTime = root.remainingExpirationTime; - if (remainingExpirationTime === NoWork) { - // If this root no longer has work, remove it from the scheduler. - let next = root.nextScheduledRoot; - root.nextScheduledRoot = null; - if (previousScheduledRoot === null) { - firstScheduledRoot = next; + if (lastScheduledRoot !== null) { + let previousScheduledRoot = lastScheduledRoot; + let root = firstScheduledRoot; + while (root !== null) { + const remainingExpirationTime = root.remainingExpirationTime; + if (remainingExpirationTime === NoWork) { + // This root no longer has work. Remove it from the scheduler. + + // TODO: This check is redudant, but Flow is confused by the branch + // below where we set lastScheduledRoot to null, even though we break + // from the loop right after. + invariant( + previousScheduledRoot !== null && lastScheduledRoot !== null, + 'Should have a previous and last root. This error is likely ' + + 'caused by a bug in React. Please file an issue.', + ); + if (root === root.nextScheduledRoot) { + // This is the only root in the list. + root.nextScheduledRoot = null; + firstScheduledRoot = lastScheduledRoot = null; + break; + } else if (root === firstScheduledRoot) { + // This is the first root in the list. + const next = root.nextScheduledRoot; + firstScheduledRoot = next; + lastScheduledRoot.nextScheduledRoot = next; + root.nextScheduledRoot = null; + } else if (root === lastScheduledRoot) { + // This is the last root in the list. + lastScheduledRoot = previousScheduledRoot; + lastScheduledRoot.nextScheduledRoot = firstScheduledRoot; + root.nextScheduledRoot = null; + break; + } else { + previousScheduledRoot.nextScheduledRoot = root.nextScheduledRoot; + root.nextScheduledRoot = null; + } + root = previousScheduledRoot.nextScheduledRoot; } else { - previousScheduledRoot.nextScheduledRoot = next; - } - if (next === null) { - lastScheduledRoot = null; + if ( + highestPriorityWork === NoWork || + remainingExpirationTime < highestPriorityWork + ) { + // Update the priority, if it's higher + highestPriorityWork = remainingExpirationTime; + highestPriorityRoot = root; + } + if (root === lastScheduledRoot) { + break; + } + previousScheduledRoot = root; + root = root.nextScheduledRoot; } - root = next; - continue; - } else if ( - highestPriorityWork === NoWork || - remainingExpirationTime < highestPriorityWork - ) { - // Update the priority, if it's higher - highestPriorityWork = remainingExpirationTime; - highestPriorityRoot = root; } - previousScheduledRoot = root; - root = root.nextScheduledRoot; } // If the next root is the same as the previous root, this is a nested @@ -1408,18 +1434,11 @@ module.exports = function( nextFlushedExpirationTime = highestPriorityWork; } - function flushAsyncWork(dl) { + function performAsyncWork(dl) { performWork(NoWork, dl); } function performWork(minExpirationTime: ExpirationTime, dl: Deadline | null) { - invariant( - !isRendering, - 'performWork was called recursively. This error is likely caused ' + - 'by a bug in React. Please file an issue.', - ); - - isRendering = true; deadline = dl; // Keep working on roots until there's no more work, or until the we reach @@ -1432,49 +1451,7 @@ module.exports = function( nextFlushedExpirationTime <= minExpirationTime) && !deadlineDidExpire ) { - // Check if this is async work or sync/expired work. - // TODO: Pass current time as argument to renderRoot, commitRoot - if (nextFlushedExpirationTime <= recalculateCurrentTime()) { - // Flush sync work. - let finishedWork = nextFlushedRoot.finishedWork; - if (finishedWork !== null) { - // This root is already complete. We can commit it. - nextFlushedRoot.finishedWork = null; - nextFlushedRoot.remainingExpirationTime = commitRoot(finishedWork); - } else { - nextFlushedRoot.finishedWork = null; - finishedWork = renderRoot(nextFlushedRoot, nextFlushedExpirationTime); - if (finishedWork !== null) { - // We've completed the root. Commit it. - nextFlushedRoot.remainingExpirationTime = commitRoot(finishedWork); - } - } - } else { - // Flush async work. - let finishedWork = nextFlushedRoot.finishedWork; - if (finishedWork !== null) { - // This root is already complete. We can commit it. - nextFlushedRoot.finishedWork = null; - nextFlushedRoot.remainingExpirationTime = commitRoot(finishedWork); - } else { - nextFlushedRoot.finishedWork = null; - finishedWork = renderRoot(nextFlushedRoot, nextFlushedExpirationTime); - if (finishedWork !== null) { - // We've completed the root. Check the deadline one more time - // before committing. - if (!shouldYield()) { - // Still time left. Commit the root. - nextFlushedRoot.remainingExpirationTime = commitRoot( - finishedWork, - ); - } else { - // There's no time left. Mark this root as complete. We'll come - // back and commit it later. - nextFlushedRoot.finishedWork = finishedWork; - } - } - } - } + performWorkOnRoot(nextFlushedRoot, nextFlushedExpirationTime); // Find the next highest priority work. findHighestPriorityRoot(); } @@ -1489,13 +1466,12 @@ module.exports = function( // If there's work left over, schedule a new callback. if (nextFlushedRoot !== null && !isCallbackScheduled) { isCallbackScheduled = true; - scheduleDeferredCallback(flushAsyncWork); + scheduleDeferredCallback(performAsyncWork); } // Clean-up. deadline = null; deadlineDidExpire = false; - isRendering = false; nestedUpdateCount = 0; if (hasUnhandledError) { @@ -1506,6 +1482,60 @@ module.exports = function( } } + function performWorkOnRoot(root, expirationTime) { + invariant( + !isRendering, + 'performWorkOnRoot was called recursively. This error is likely caused ' + + 'by a bug in React. Please file an issue.', + ); + + isRendering = true; + + // Check if this is async work or sync/expired work. + // TODO: Pass current time as argument to renderRoot, commitRoot + if (expirationTime <= recalculateCurrentTime()) { + // Flush sync work. + let finishedWork = root.finishedWork; + if (finishedWork !== null) { + // This root is already complete. We can commit it. + root.finishedWork = null; + root.remainingExpirationTime = commitRoot(finishedWork); + } else { + root.finishedWork = null; + finishedWork = renderRoot(root, expirationTime); + if (finishedWork !== null) { + // We've completed the root. Commit it. + root.remainingExpirationTime = commitRoot(finishedWork); + } + } + } else { + // Flush async work. + let finishedWork = root.finishedWork; + if (finishedWork !== null) { + // This root is already complete. We can commit it. + root.finishedWork = null; + root.remainingExpirationTime = commitRoot(finishedWork); + } else { + root.finishedWork = null; + finishedWork = renderRoot(root, expirationTime); + if (finishedWork !== null) { + // We've completed the root. Check the deadline one more time + // before committing. + if (!shouldYield()) { + // Still time left. Commit the root. + root.remainingExpirationTime = commitRoot(finishedWork); + } else { + // There's no time left. Mark this root as complete. We'll come + // back and commit it later. + root.finishedWork = finishedWork; + } + } + } + } + + isRendering = false; + } + // When working on async work, the reconciler asks the renderer if it should // yield execution. For DOM, we implement this with requestIdleCallback. function shouldYield() { @@ -1546,7 +1576,7 @@ module.exports = function( } finally { isBatchingUpdates = previousIsBatchingUpdates; if (!isBatchingUpdates && !isRendering) { - performWork(Task, null); + performWork(Sync, null); } } } @@ -1579,7 +1609,7 @@ module.exports = function( 'flushSync was called from inside a lifecycle method. It cannot be ' + 'called when React is already rendering.', ); - performWork(Task, null); + performWork(Sync, null); } }