DevTools Profiler: Improve how empty commits are filtered (#17771)

Previously, DevTools filtered empty commits on the backend, while profiling, through the use of a bailout heuristic that React currently happens to use. This approach was brittle and may have exacerbated the long-standing Profiler bug #16446. This PR removes that heuristic and adds as a post-processing filtering pass instead. This removes the coupling between DevTools and a React internal implementation detail that may change. I believe DevTools has two choices of criteria for this filtering: * Filter commits that have no actual duration metadata. * Filter commits that have no recorded operations (no mutations to the tree, no changed tree base durations). I chose the first option, filtering by commits that have no reported metadata. It will miss an edge case, e.g. , but we would have nothing meaningful to show in the Profiler for those cases anyway. (This particular edge case is why one of the snapshots changed with this commit.) The second option, filtering by recorded operations, could potentially miss a more important edge case: where a component *did* render, but its didn't change. (In that event, there would be no operations to send.)
facebook · Jan 5, 2020 · 24f8242 · 24f8242
1 parent edeea07
commit 24f8242
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 92 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
@@ -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
@@ -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
@@ -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,
         });