Skip to content

Commit

Permalink
Disable Profiler commit filtering
Browse files Browse the repository at this point in the history
We used to filter "empty" DevTools commits, but it was error prone (see facebook#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.
  • Loading branch information
Brian Vaughn committed May 7, 2020
1 parent fb3f0ac commit 9ed51df
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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 [
Expand All @@ -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 [
Expand Down
57 changes: 26 additions & 31 deletions packages/react-devtools-shared/src/devtools/views/Profiler/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down

0 comments on commit 9ed51df

Please sign in to comment.