Filter empty commits (all Fibers bailed out) from Profiler (facebook#…

…22745)
zhengjitf · Apr 15, 2022 · a37f0be · a37f0be
1 parent 8daae6c
commit a37f0be
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 208 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
@@ -1700,94 +1700,6 @@ 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 [
@@ -2330,115 +2242,6 @@ 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,
-          },
-        ],
-      ],
-    },
   ],
   "version": 5,
 }

diff --git a/packages/react-devtools-shared/src/__tests__/profilerStore-test.js b/packages/react-devtools-shared/src/__tests__/profilerStore-test.js
@@ -124,6 +124,57 @@ describe('ProfilerStore', () => {
     expect(data.operations).toHaveLength(1);
   });
 
+  it('should filter empty commits alt', () => {
+    let commitCount = 0;
+
+    const inputRef = React.createRef();
+    const Example = () => {
+      const [, setTouched] = React.useState(false);
+
+      const handleBlur = () => {
+        setTouched(true);
+      };
+
+      require('scheduler').unstable_advanceTime(1);
+
+      React.useLayoutEffect(() => {
+        commitCount++;
+      });
+
+      return <input ref={inputRef} onBlur={handleBlur} />;
+    };
+
+    const container = document.createElement('div');
+
+    // This element has to be in the <body> for the event system to work.
+    document.body.appendChild(container);
+
+    // It's important that this test uses legacy sync mode.
+    // The root API does not trigger this particular failing case.
+    legacyRender(<Example />, container);
+
+    expect(commitCount).toBe(1);
+    commitCount = 0;
+
+    utils.act(() => store.profilerStore.startProfiling());
+
+    // Focus and blur.
+    const target = inputRef.current;
+    target.focus();
+    target.blur();
+    target.focus();
+    target.blur();
+    expect(commitCount).toBe(1);
+
+    utils.act(() => store.profilerStore.stopProfiling());
+
+    // Only one commit should have been recorded (in response to the "change" event).
+    const root = store.roots[0];
+    const data = store.profilerStore.getDataForRoot(root);
+    expect(data.commitData).toHaveLength(1);
+    expect(data.operations).toHaveLength(1);
+  });
+
   it('should throw if component filters are modified while profiling', () => {
     utils.act(() => store.profilerStore.startProfiling());
 

diff --git a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js
@@ -108,7 +108,10 @@ describe('ProfilingCache', () => {
       });
     }
 
-    expect(allProfilingDataForRoots).toHaveLength(3);
+    // 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);
 
     utils.exportImportHelper(bridge, store);
 

diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js
@@ -1546,6 +1546,19 @@ export function attach(
   }
 
   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.
+      if (
+        !isProfiling ||
+        currentCommitProfilingMetadata == null ||
+        currentCommitProfilingMetadata.durations.length === 0
+      ) {
+        return;
+      }
+    }
+
     if (pendingOperationsQueue !== null) {
       pendingOperationsQueue.push(operations);
     } else {
@@ -2608,18 +2621,26 @@ export function attach(
     }
 
     if (isProfiling && isProfilingSupported) {
-      const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get(
-        currentRootID,
-      );
-      if (commitProfilingMetadata != null) {
-        commitProfilingMetadata.push(
-          ((currentCommitProfilingMetadata: any): CommitProfilingData),
-        );
-      } else {
-        ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).set(
+      // 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
+      ) {
+        const commitProfilingMetadata = ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get(
           currentRootID,
-          [((currentCommitProfilingMetadata: any): CommitProfilingData)],
         );
+
+        if (commitProfilingMetadata != null) {
+          commitProfilingMetadata.push(
+            ((currentCommitProfilingMetadata: any): CommitProfilingData),
+          );
+        } else {
+          ((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).set(
+            currentRootID,
+            [((currentCommitProfilingMetadata: any): CommitProfilingData)],
+          );
+        }
       }
     }