From 3deabd4b5ca67ae44429df6c97c985f74198a35e Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 10 Mar 2022 10:35:52 -0800 Subject: [PATCH] Fixed edge case bug in Profiler (#24031) --- .../__snapshots__/profilingCache-test.js.snap | 197 ++++++++++++++++++ .../src/__tests__/profilingCache-test.js | 5 +- .../src/backend/renderer.js | 47 ++--- 3 files changed, 220 insertions(+), 29 deletions(-) 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 258560929435c..3611f67652aa4 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 @@ -2960,6 +2960,94 @@ 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, + "effectDuration": 0, + "fiberActualDurations": Map {}, + "fiberSelfDurations": Map {}, + "passiveEffectDuration": 0, + "priorityLevel": "Immediate", + "timestamp": 34, + "updaters": Array [ + Object { + "displayName": "render()", + "hocDisplayNames": null, + "id": 7, + "key": null, + "type": 11, + }, + ], + }, + ], + "displayName": "Parent", + "initialTreeBaseDurations": Map { + 7 => 11, + 8 => 11, + 10 => 0, + 11 => 1, + }, + "operations": Array [ + Array [ + 1, + 7, + 0, + 2, + 4, + 11, + 10, + 8, + 7, + ], + ], + "rootID": 7, + "snapshots": Map { + 7 => Object { + "children": Array [ + 8, + ], + "displayName": null, + "hocDisplayNames": null, + "id": 7, + "key": null, + "type": 11, + }, + 8 => Object { + "children": Array [ + 10, + 11, + ], + "displayName": "Parent", + "hocDisplayNames": null, + "id": 8, + "key": null, + "type": 5, + }, + 10 => Object { + "children": Array [], + "displayName": "Child", + "hocDisplayNames": null, + "id": 10, + "key": "0", + "type": 5, + }, + 11 => Object { + "children": Array [], + "displayName": "Child", + "hocDisplayNames": Array [ + "Memo", + ], + "id": 11, + "key": null, + "type": 8, + }, + }, +} +`; + exports[`ProfilingCache should collect data for each root (including ones added or mounted after profiling started): imported data 1`] = ` Object { "dataForRoots": Array [ @@ -3504,6 +3592,115 @@ Object { "rootID": 13, "snapshots": Array [], }, + Object { + "commitData": Array [ + Object { + "changeDescriptions": Array [], + "duration": 0, + "effectDuration": 0, + "fiberActualDurations": Array [], + "fiberSelfDurations": Array [], + "passiveEffectDuration": 0, + "priorityLevel": "Immediate", + "timestamp": 34, + "updaters": Array [ + Object { + "displayName": "render()", + "hocDisplayNames": null, + "id": 7, + "key": null, + "type": 11, + }, + ], + }, + ], + "displayName": "Parent", + "initialTreeBaseDurations": Array [ + Array [ + 7, + 11, + ], + Array [ + 8, + 11, + ], + Array [ + 10, + 0, + ], + Array [ + 11, + 1, + ], + ], + "operations": Array [ + Array [ + 1, + 7, + 0, + 2, + 4, + 11, + 10, + 8, + 7, + ], + ], + "rootID": 7, + "snapshots": Array [ + Array [ + 7, + Object { + "children": Array [ + 8, + ], + "displayName": null, + "hocDisplayNames": null, + "id": 7, + "key": null, + "type": 11, + }, + ], + Array [ + 8, + Object { + "children": Array [ + 10, + 11, + ], + "displayName": "Parent", + "hocDisplayNames": null, + "id": 8, + "key": null, + "type": 5, + }, + ], + Array [ + 10, + Object { + "children": Array [], + "displayName": "Child", + "hocDisplayNames": null, + "id": 10, + "key": "0", + "type": 5, + }, + ], + Array [ + 11, + Object { + "children": Array [], + "displayName": "Child", + "hocDisplayNames": Array [ + "Memo", + ], + "id": 11, + "key": null, + "type": 8, + }, + ], + ], + }, ], "timelineData": Array [ Object { diff --git a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js index f41e39a35d695..c1d17fd0f6a3f 100644 --- a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js +++ b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js @@ -110,10 +110,7 @@ describe('ProfilingCache', () => { }); } - // No profiling data gets logged for the 2nd root (container B) - // because it doesn't render anything while profiling. - // (Technically it unmounts but we don't profile root unmounts.) - expect(allProfilingDataForRoots).toHaveLength(2); + expect(allProfilingDataForRoots).toHaveLength(3); utils.exportImportHelper(bridge, store); diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 665b85d183a3f..32ce8e2a9fe55 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1624,20 +1624,29 @@ export function attach( pendingOperations.push(op); } - function flushOrQueueOperations(operations: OperationsArray): void { - if (operations.length === 3) { - // This operations array is a no op: [renderer ID, root ID, string table size (0)] - // We can usually skip sending updates like this across the bridge, unless we're Profiling. - // In that case, even though the tree didn't change– some Fibers may have still rendered. + function shouldBailoutWithPendingOperations() { + if (isProfiling) { if ( - !isProfiling || - currentCommitProfilingMetadata == null || - currentCommitProfilingMetadata.durations.length === 0 + currentCommitProfilingMetadata != null && + currentCommitProfilingMetadata.durations.length > 0 ) { - return; + return false; } } + return ( + pendingOperations.length === 0 && + pendingRealUnmountedIDs.length === 0 && + pendingSimulatedUnmountedIDs.length === 0 && + pendingUnmountedRootID === null + ); + } + + function flushOrQueueOperations(operations: OperationsArray): void { + if (shouldBailoutWithPendingOperations()) { + return; + } + if (pendingOperationsQueue !== null) { pendingOperationsQueue.push(operations); } else { @@ -1668,7 +1677,7 @@ export function attach( recordPendingErrorsAndWarnings(); - if (pendingOperations.length === 0) { + if (shouldBailoutWithPendingOperations()) { // No warnings or errors to flush; we can bail out early here too. return; } @@ -1791,12 +1800,7 @@ export function attach( // We do this just before flushing, so we can ignore errors for no-longer-mounted Fibers. recordPendingErrorsAndWarnings(); - if ( - pendingOperations.length === 0 && - pendingRealUnmountedIDs.length === 0 && - pendingSimulatedUnmountedIDs.length === 0 && - pendingUnmountedRootID === null - ) { + if (shouldBailoutWithPendingOperations()) { // If we aren't profiling, we can just bail out here. // No use sending an empty update over the bridge. // @@ -1805,9 +1809,7 @@ export function attach( // (2) the operations array for each commit // Because of this, it's important that the operations and metadata arrays align, // So it's important not to omit even empty operations while profiling is active. - if (!isProfiling) { - return; - } + return; } const numUnmountIDs = @@ -2724,12 +2726,7 @@ export function attach( } if (isProfiling && isProfilingSupported) { - // Make sure at least one Fiber performed work during this commit. - // If not, don't send it to the frontend; showing an empty commit in the Profiler is confusing. - if ( - currentCommitProfilingMetadata != null && - currentCommitProfilingMetadata.durations.length > 0 - ) { + if (!shouldBailoutWithPendingOperations()) { const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get( currentRootID, );