Fixed edge case bug in Profiler (#24031)

facebook · Mar 10, 2022 · 48a8574 · 48a8574
1 parent 72a933d
commit 48a8574
Show file tree

Hide file tree

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