Skip to content

Commit

Permalink
Filter empty commits (all Fibers bailed out) from Profiler (#22745)
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Vaughn authored Nov 11, 2021
1 parent e0aa5e2 commit a44a7a2
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 208 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 [
Expand Down Expand Up @@ -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,
}
Expand Down
51 changes: 51 additions & 0 deletions packages/react-devtools-shared/src/__tests__/profilerStore-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
41 changes: 31 additions & 10 deletions packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)],
);
}
}
}

Expand Down

0 comments on commit a44a7a2

Please sign in to comment.