From 9ed51df941dbbfe2b89414a254212f8ddeca31dd Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 7 May 2020 15:26:07 -0700 Subject: [PATCH 1/2] Disable Profiler commit filtering We used to filter "empty" DevTools commits, but it was error prone (see #18798). A commit may appear to be empty (no actual durations) because of component filters, but filtering these empty commits causes interaction commit indices to be off by N. This not only corrupts the resulting data, but also potentially causes runtime errors. For that matter, hiding "empty" commits might cause confusion too. A commit *did happen* even if none of the components the Profiler is showing were involved. --- .../__snapshots__/profilingCache-test.js.snap | 52 +++++++++++++++-- .../src/devtools/views/Profiler/utils.js | 57 +++++++++---------- 2 files changed, 74 insertions(+), 35 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 2ccf1f16affca..28732d8d64a75 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,7 +1500,17 @@ 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 [], + "commitData": Array [ + Object { + "changeDescriptions": Map {}, + "duration": 0, + "fiberActualDurations": Map {}, + "fiberSelfDurations": Map {}, + "interactionIDs": Array [], + "priorityLevel": "Immediate", + "timestamp": 34, + }, + ], "displayName": "Parent", "initialTreeBaseDurations": Map { 6 => 11, @@ -1510,7 +1520,19 @@ Object { }, "interactionCommits": Map {}, "interactions": Map {}, - "operations": Array [], + "operations": Array [ + Array [ + 1, + 6, + 0, + 2, + 4, + 9, + 8, + 7, + 6, + ], + ], "rootID": 6, "snapshots": Map { 6 => Object { @@ -2044,7 +2066,17 @@ Object { "snapshots": Array [], }, Object { - "commitData": Array [], + "commitData": Array [ + Object { + "changeDescriptions": Array [], + "duration": 0, + "fiberActualDurations": Array [], + "fiberSelfDurations": Array [], + "interactionIDs": Array [], + "priorityLevel": "Immediate", + "timestamp": 34, + }, + ], "displayName": "Parent", "initialTreeBaseDurations": Array [ Array [ @@ -2066,7 +2098,19 @@ Object { ], "interactionCommits": Array [], "interactions": Array [], - "operations": Array [], + "operations": Array [ + Array [ + 1, + 6, + 0, + 2, + 4, + 9, + 8, + 7, + 6, + ], + ], "rootID": 6, "snapshots": Array [ Array [ 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 b9aa04fee1667..c453fc166cf5c 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js @@ -60,43 +60,38 @@ 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]); - } - }); + // Do not filter empty commits from the profiler data! + // We used to do this, but it was error prone (see #18798). + // A commit may appear to be empty (no actual durations) because of component filters, + // but filtering these empty commits causes interaction commit indices to be off by N. + // This not only corrupts the resulting data, but also potentially causes runtime errors. + // + // For that matter, hiding "empty" commits might cause confusion too. + // A commit *did happen* even if none of the components the Profiler is showing were involved. + const convertedCommitData = 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, + }), + ); dataForRoots.set(rootID, { - commitData: filteredCommitData, + commitData: convertedCommitData, displayName, initialTreeBaseDurations: new Map(initialTreeBaseDurations), interactionCommits: new Map(interactionCommits), interactions: new Map(interactions), - operations: filteredOperations, + operations, rootID, snapshots, }); From f749ea3ff4e9a4d1557df266317f8a474719d297 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 7 May 2020 16:32:24 -0700 Subject: [PATCH 2/2] Restart flaky CI