From 1721040771a25fedd889325fe2f2fa70a96ebb23 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 7 Nov 2017 12:33:35 +0000 Subject: [PATCH 1/2] Performance tool: Warn when interrupting an in-progress tree --- .../src/ReactDebugFiberPerf.js | 14 ++++++---- .../src/ReactFiberScheduler.js | 19 ++++++++----- .../__tests__/ReactIncrementalPerf-test.js | 14 ++++++++++ .../ReactIncrementalPerf-test.js.snap | 27 ++++++++++++++++--- 4 files changed, 59 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactDebugFiberPerf.js b/packages/react-reconciler/src/ReactDebugFiberPerf.js index ad04bc9cecae..4e531134430a 100644 --- a/packages/react-reconciler/src/ReactDebugFiberPerf.js +++ b/packages/react-reconciler/src/ReactDebugFiberPerf.js @@ -318,8 +318,9 @@ export function stopPhaseTimer(): void { } } -export function startWorkLoopTimer(): void { +export function startWorkLoopTimer(nextUnitOfWork: Fiber | null): void { if (enableUserTimingAPI) { + currentFiber = nextUnitOfWork; if (!supportsUserTiming) { return; } @@ -332,14 +333,17 @@ export function startWorkLoopTimer(): void { } } -export function stopWorkLoopTimer(): void { +export function stopWorkLoopTimer(didInterrupt: boolean): void { if (enableUserTimingAPI) { if (!supportsUserTiming) { return; } - const warning = commitCountInCurrentWorkLoop > 1 - ? 'There were cascading updates' - : null; + let warning = null; + if (didInterrupt) { + warning = 'Interrupted an in-progress update'; + } else if (commitCountInCurrentWorkLoop > 1) { + warning = 'There were cascading updates'; + } commitCountInCurrentWorkLoop = 0; // Pause any measurements until the next loop. pauseTimers(); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 19dfef14aa68..50fad6e0b99d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -216,6 +216,9 @@ export default function( let isCommitting: boolean = false; let isUnmounting: boolean = false; + // Used for performance tracking. + let didInterrupt: boolean = false; + function resetContextStack() { // Reset the stack reset(); @@ -752,8 +755,6 @@ export default function( root: FiberRoot, expirationTime: ExpirationTime, ): Fiber | null { - startWorkLoopTimer(); - invariant( !isWorking, 'renderRoot was called recursively. This error is likely caused ' + @@ -772,7 +773,7 @@ export default function( expirationTime !== nextRenderExpirationTime || nextUnitOfWork === null ) { - // This is a restart. Reset the stack. + // Reset the stack and start working from the root. resetContextStack(); nextRoot = root; nextRenderExpirationTime = expirationTime; @@ -783,6 +784,8 @@ export default function( ); } + startWorkLoopTimer(nextUnitOfWork); + let didError = false; let error = null; if (__DEV__) { @@ -865,12 +868,12 @@ export default function( const uncaughtError = firstUncaughtError; // We're done performing work. Time to clean up. + stopWorkLoopTimer(didInterrupt); + didInterrupt = false; isWorking = false; didFatal = false; firstUncaughtError = null; - stopWorkLoopTimer(); - if (uncaughtError !== null) { onUncaughtError(uncaughtError); } @@ -1198,7 +1201,11 @@ export default function( root === nextRoot && expirationTime <= nextRenderExpirationTime ) { - // This is an interruption. Restart the root from the top. + // Restart the root from the top. + if (nextUnitOfWork !== null) { + // This is an interruption. (Used for performance tracking.) + didInterrupt = true; + } nextRoot = null; nextUnitOfWork = null; nextRenderExpirationTime = NoWork; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.js index 5b699c9414a5..f690be2b0843 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.js @@ -498,4 +498,18 @@ describe('ReactDebugFiberPerf', () => { }); expect(getFlameChart()).toMatchSnapshot(); }); + + it('warns if an in-progress update is interrupted', () => { + function Foo() { + return ; + } + + ReactNoop.render(); + ReactNoop.flushUnitsOfWork(2); + ReactNoop.flushSync(() => { + ReactNoop.render(); + }); + ReactNoop.flush(); + expect(getFlameChart()).toMatchSnapshot(); + }); }); diff --git a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap index 4815a22fc0c0..ab3220f9f7c9 100644 --- a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap +++ b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap @@ -243,6 +243,17 @@ exports[`ReactDebugFiberPerf skips parents during setState 1`] = ` " `; +exports[`ReactDebugFiberPerf supports portals 1`] = ` +"⚛ (React Tree Reconciliation) + ⚛ Parent [mount] + ⚛ Child [mount] + +⚛ (Committing Changes) + ⚛ (Committing Host Effects: 2 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) +" +`; + exports[`ReactDebugFiberPerf supports returns 1`] = ` "⚛ (React Tree Reconciliation) ⚛ App [mount] @@ -260,14 +271,22 @@ exports[`ReactDebugFiberPerf supports returns 1`] = ` " `; -exports[`ReactDebugFiberPerf supports portals 1`] = ` +exports[`ReactDebugFiberPerf warns if an in-progress update is interrupted 1`] = ` "⚛ (React Tree Reconciliation) - ⚛ Parent [mount] - ⚛ Child [mount] + ⚛ Foo [mount] + +⛔ (React Tree Reconciliation) Warning: Interrupted an in-progress update + ⚛ Foo [mount] ⚛ (Committing Changes) - ⚛ (Committing Host Effects: 2 Total) + ⚛ (Committing Host Effects: 1 Total) ⚛ (Calling Lifecycle Methods: 0 Total) + +⚛ (React Tree Reconciliation) + +⚛ (Committing Changes) + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 1 Total) " `; From 30b5334aac7046858ff52e81b095bffd6a6db7ce Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 7 Nov 2017 16:15:51 +0000 Subject: [PATCH 2/2] Include the name of the component that caused the interruption --- packages/react-reconciler/src/ReactDebugFiberPerf.js | 11 ++++++++--- packages/react-reconciler/src/ReactFiberScheduler.js | 8 ++++---- .../__snapshots__/ReactIncrementalPerf-test.js.snap | 2 +- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactDebugFiberPerf.js b/packages/react-reconciler/src/ReactDebugFiberPerf.js index 4e531134430a..625dcda11b6e 100644 --- a/packages/react-reconciler/src/ReactDebugFiberPerf.js +++ b/packages/react-reconciler/src/ReactDebugFiberPerf.js @@ -333,14 +333,19 @@ export function startWorkLoopTimer(nextUnitOfWork: Fiber | null): void { } } -export function stopWorkLoopTimer(didInterrupt: boolean): void { +export function stopWorkLoopTimer(interruptedBy: Fiber | null): void { if (enableUserTimingAPI) { if (!supportsUserTiming) { return; } let warning = null; - if (didInterrupt) { - warning = 'Interrupted an in-progress update'; + if (interruptedBy !== null) { + if (interruptedBy.tag === HostRoot) { + warning = 'A top-level update interrupted the previous render'; + } else { + const componentName = getComponentName(interruptedBy) || 'Unknown'; + warning = `An update to ${componentName} interrupted the previous render`; + } } else if (commitCountInCurrentWorkLoop > 1) { warning = 'There were cascading updates'; } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 50fad6e0b99d..7bdc89c70711 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -217,7 +217,7 @@ export default function( let isUnmounting: boolean = false; // Used for performance tracking. - let didInterrupt: boolean = false; + let interruptedBy: Fiber | null = null; function resetContextStack() { // Reset the stack @@ -868,8 +868,8 @@ export default function( const uncaughtError = firstUncaughtError; // We're done performing work. Time to clean up. - stopWorkLoopTimer(didInterrupt); - didInterrupt = false; + stopWorkLoopTimer(interruptedBy); + interruptedBy = null; isWorking = false; didFatal = false; firstUncaughtError = null; @@ -1204,7 +1204,7 @@ export default function( // Restart the root from the top. if (nextUnitOfWork !== null) { // This is an interruption. (Used for performance tracking.) - didInterrupt = true; + interruptedBy = fiber; } nextRoot = null; nextUnitOfWork = null; diff --git a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap index ab3220f9f7c9..0bd35bd70129 100644 --- a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap +++ b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.js.snap @@ -275,7 +275,7 @@ exports[`ReactDebugFiberPerf warns if an in-progress update is interrupted 1`] = "⚛ (React Tree Reconciliation) ⚛ Foo [mount] -⛔ (React Tree Reconciliation) Warning: Interrupted an in-progress update +⛔ (React Tree Reconciliation) Warning: A top-level update interrupted the previous render ⚛ Foo [mount] ⚛ (Committing Changes)