Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DevTools Profiler: Improve how empty commits are filtered #17771

Merged
merged 1 commit into from
Jan 5, 2020
Merged

DevTools Profiler: Improve how empty commits are filtered #17771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 4 additions & 48 deletions packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -1520,19 +1510,7 @@ Object {
},
"interactionCommits": Map {},
"interactions": Map {},
"operations": Array [
Array [
1,
6,
0,
2,
4,
9,
8,
7,
6,
Comment on lines -1528 to -1533
Copy link
Contributor Author

@bvaughn bvaughn Jan 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to better annotate this change and what the deleted operations mean:

  1,  # rendered ID
  6,  # root ID
  0,  # string table isze
  2,  # TREE_OPERATION_REMOVE operation
  4,  # number of Fibers removed
  9,  # ids of Fibers removed...
  8,  
  7,  
  6,

As explained in the PR description, this snapshot update was expected.

],
],
"operations": Array [],
"rootID": 6,
"snapshots": Map {
6 => Object {
Expand Down Expand Up @@ -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 [
Expand All @@ -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 [
Expand Down
36 changes: 7 additions & 29 deletions packages/react-devtools-shared/src/backend/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 = {
Expand Down Expand Up @@ -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,
);
Expand Down
58 changes: 43 additions & 15 deletions packages/react-devtools-shared/src/devtools/views/Profiler/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down