diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap index 82438ada7ca35..95f3c479b9c0f 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap @@ -1500,17 +1500,7 @@ Object { exports[`ProfilingCache should collect data for each root (including ones added or mounted after profiling started): Data for root Parent 3`] = ` Object { - "commitData": Array [ - Object { - "changeDescriptions": Map {}, - "duration": 0, - "fiberActualDurations": Map {}, - "fiberSelfDurations": Map {}, - "interactionIDs": Array [], - "priorityLevel": "Immediate", - "timestamp": 34, - }, - ], + "commitData": Array [], "displayName": "Parent", "initialTreeBaseDurations": Map { 6 => 11, @@ -1520,19 +1510,7 @@ Object { }, "interactionCommits": Map {}, "interactions": Map {}, - "operations": Array [ - Array [ - 1, - 6, - 0, - 2, - 4, - 9, - 8, - 7, - 6, - ], - ], + "operations": Array [], "rootID": 6, "snapshots": Map { 6 => Object { @@ -2066,17 +2044,7 @@ Object { "snapshots": Array [], }, Object { - "commitData": Array [ - Object { - "changeDescriptions": Array [], - "duration": 0, - "fiberActualDurations": Array [], - "fiberSelfDurations": Array [], - "interactionIDs": Array [], - "priorityLevel": "Immediate", - "timestamp": 34, - }, - ], + "commitData": Array [], "displayName": "Parent", "initialTreeBaseDurations": Array [ Array [ @@ -2098,19 +2066,7 @@ Object { ], "interactionCommits": Array [], "interactions": Array [], - "operations": Array [ - Array [ - 1, - 6, - 0, - 2, - 4, - 9, - 8, - 7, - 6, - ], - ], + "operations": Array [], "rootID": 6, "snapshots": Array [ Array [ diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 8b3f4fe0546e5..db5d45e3f0eb4 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1023,28 +1023,13 @@ export function attach( ) { // If we aren't profiling, we can just bail out here. // No use sending an empty update over the bridge. - if (!isProfiling) { - return; - } - - const current = root.current; - const alternate = current.alternate; - - // Certain types of updates bail out at the root without doing any actual render work. - // React should probably not call the DevTools commit hook in this case, - // but if it does- we can detect it and filter them out from the profiler. - // NOTE: Keep this logic in sync with the one in handleCommitFiberRoot() - const didBailoutAtRoot = - alternate !== null && - alternate.expirationTime === 0 && - alternate.childExpirationTime === 0; - + // // The Profiler stores metadata for each commit and reconstructs the app tree per commit using: // (1) an initial tree snapshot and // (2) the operations array for each commit // Because of this, it's important that the operations and metadata arrays align, - // So the logic that skips metadata for bailout commits should also apply to filter operations. - if (didBailoutAtRoot) { + // So it's important not to ommit even empty operations while profiing is active. + if (!isProfiling) { return; } } @@ -1388,6 +1373,8 @@ export function attach( if (isProfiling) { const {alternate} = fiber; + // It's important to update treeBaseDuration even if the current Fiber did not render, + // becuase it's possible that one of its descednants did. if ( alternate == null || treeBaseDuration !== alternate.treeBaseDuration @@ -1775,15 +1762,6 @@ export function attach( const current = root.current; const alternate = current.alternate; - // Certain types of updates bail out at the root without doing any actual render work. - // React should probably not call the DevTools commit hook in this case, - // but if it does- we can detect it and filter them out from the profiler. - // NOTE: Keep this logic in sync with the one in flushPendingEvents() - const didBailoutAtRoot = - alternate !== null && - alternate.expirationTime === 0 && - alternate.childExpirationTime === 0; - currentRootID = getFiberID(getPrimaryFiber(current)); // Before the traversals, remember to start tracking @@ -1800,7 +1778,7 @@ export function attach( // where some v16 renderers support profiling and others don't. const isProfilingSupported = root.memoizedInteractions != null; - if (isProfiling && isProfilingSupported && !didBailoutAtRoot) { + if (isProfiling && isProfilingSupported) { // If profiling is active, store commit time and duration, and the current interactions. // The frontend may request this information after profiling has stopped. currentCommitProfilingMetadata = { @@ -1844,7 +1822,7 @@ export function attach( mountFiberRecursively(current, null, false, false); } - if (isProfiling && isProfilingSupported && !didBailoutAtRoot) { + if (isProfiling && isProfilingSupported) { const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get( currentRootID, ); diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js index db6b0652c870d..b7aa9b88b6e0a 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js @@ -60,26 +60,54 @@ export function prepareProfilingDataFrontendFromBackendAndStore( throw Error(`Could not find profiling snapshots for root ${rootID}`); } + const filteredCommitData = []; + const filteredOperations = []; + + // Filter empty commits from the profiler data. + // It is very important to keep operations and commit data arrays perfect in sync. + // So we must use the same criteria to filter both. + // If these two arrays were to get out of sync, the profiler would runtime error. + // We choose to filter on commit metadata, rather than the operations array, + // because the latter may have false positives, + // (e.g. a commit that re-rendered a component with the same treeBaseDuration as before). + commitData.forEach((commitDataBackend, commitIndex) => { + if (commitDataBackend.fiberActualDurations.length > 0) { + filteredCommitData.push({ + changeDescriptions: + commitDataBackend.changeDescriptions != null + ? new Map(commitDataBackend.changeDescriptions) + : null, + duration: commitDataBackend.duration, + fiberActualDurations: new Map( + commitDataBackend.fiberActualDurations, + ), + fiberSelfDurations: new Map(commitDataBackend.fiberSelfDurations), + interactionIDs: commitDataBackend.interactionIDs, + priorityLevel: commitDataBackend.priorityLevel, + timestamp: commitDataBackend.timestamp, + }); + filteredOperations.push(operations[commitIndex]); + } + }); + dataForRoots.set(rootID, { - commitData: commitData.map((commitDataBackend, commitIndex) => ({ - changeDescriptions: - commitDataBackend.changeDescriptions != null - ? new Map(commitDataBackend.changeDescriptions) - : null, - duration: commitDataBackend.duration, - fiberActualDurations: new Map( - commitDataBackend.fiberActualDurations, - ), - fiberSelfDurations: new Map(commitDataBackend.fiberSelfDurations), - interactionIDs: commitDataBackend.interactionIDs, - priorityLevel: commitDataBackend.priorityLevel, - timestamp: commitDataBackend.timestamp, - })), + commitData: filteredCommitData, + displayName, + initialTreeBaseDurations: new Map(initialTreeBaseDurations), + interactionCommits: new Map(interactionCommits), + interactions: new Map(interactions), + operations: filteredOperations, + rootID, + snapshots, + }); + + dataForRoots.set(rootID, { + commitData: filteredCommitData, displayName, initialTreeBaseDurations: new Map(initialTreeBaseDurations), interactionCommits: new Map(interactionCommits), interactions: new Map(interactions), - operations, + operations: filteredOperations, rootID, snapshots, });