Profiler properly handles unmounted roots
Brian Vaughn committed
May 22, 2019 at 10:19 UTC
b556f7444df042fb64555f12d8c1e893a9f4c3dc
5 files changed
+69
-42
shells/dev/app/index.js
+4
-5
@@ -4,7 +4,6 @@
4
5
import { createElement } from 'react';
6
import {
7
- unmountComponentAtNode,
7
// $FlowFixMe Flow does not yet know about createRoot()
8
unstable_createRoot as createRoot,
9
} from 'react-dom';
@@ -20,17 +19,17 @@ import SuspenseTree from './SuspenseTree';
19
20
import './styles.css';
21
23
-const containers = [];
22
+const roots = [];
23
24
function mountHelper(App) {
25
const container = document.createElement('div');
26
27
((document.body: any): HTMLBodyElement).appendChild(container);
28
30
- containers.push(container);
31
-
29
const root = createRoot(container);
30
root.render(createElement(App));
31
+
32
+ roots.push(root);
33
}
34
35
function mountTestApp() {
@@ -46,7 +45,7 @@ function mountTestApp() {
45
}
46
47
function unmountTestApp() {
49
- containers.forEach(container => unmountComponentAtNode(container));
48
+ roots.forEach(root => root.unmount());
49
}
50
51
mountTestApp();
src/devtools/ProfilerStore.js
+25
-3
@@ -34,12 +34,19 @@ export default class ProfilerStore extends EventEmitter {
34
// even though some of it is lazily parsed/derived via the ProfilingCache.
35
_dataFrontend: ProfilingDataFrontend | null = null;
36
37
+ // Snapshot of all attached renderer IDs.
38
+ // Once profiling is finished, this snapshot will be used to query renderers for profiling data.
39
+ //
40
+ // This map is initialized when profiling starts and updated when a new root is added while profiling;
41
+ // Upon completion, it is converted into the exportable ProfilingDataFrontend format.
42
+ _initialRendererIDs: Set<number> = new Set();
43
+
44
// Snapshot of the state of the main Store (including all roots) when profiling started.
45
// Once profiling is finished, this snapshot can be used along with "operations" messages emitted during profiling,
46
// to reconstruct the state of each root for each commit.
47
// It's okay to use a single root to store this information because node IDs are unique across all roots.
48
//
42
- // This map is only updated while profiling is in progress;
49
+ // This map is initialized when profiling starts and updated when a new root is added while profiling;
50
// Upon completion, it is converted into the exportable ProfilingDataFrontend format.
51
_initialSnapshotsByRootID: Map<number, Map<number, SnapshotNode>> = new Map();
52
@@ -139,6 +146,7 @@ export default class ProfilerStore extends EventEmitter {
146
set profilingData(value: ProfilingDataFrontend | null): void {
147
this._dataBackends.splice(0);
148
this._dataFrontend = value;
149
+ this._initialRendererIDs.clear();
150
this._initialSnapshotsByRootID.clear();
151
this._inProgressOperationsByRootID.clear();
152
this._inProgressScreenshotsByRootID.clear();
@@ -150,6 +158,7 @@ export default class ProfilerStore extends EventEmitter {
158
clear(): void {
159
this._dataBackends.splice(0);
160
this._dataFrontend = null;
161
+ this._initialRendererIDs.clear();
162
this._initialSnapshotsByRootID.clear();
163
this._inProgressOperationsByRootID.clear();
164
this._inProgressScreenshotsByRootID.clear();
@@ -215,6 +224,7 @@ export default class ProfilerStore extends EventEmitter {
224
}
225
226
// The first two values are always rendererID and rootID
227
+ const rendererID = operations[0];
228
const rootID = operations[1];
229
230
if (this._isProfiling) {
@@ -226,6 +236,10 @@ export default class ProfilerStore extends EventEmitter {
236
profilingOperations.push(operations);
237
}
238
239
+ if (!this._initialRendererIDs.has(rendererID)) {
240
+ this._initialRendererIDs.add(rendererID);
241
+ }
242
+
243
if (!this._initialSnapshotsByRootID.has(rootID)) {
244
this._initialSnapshotsByRootID.set(rootID, new Map());
245
}
@@ -278,11 +292,19 @@ export default class ProfilerStore extends EventEmitter {
292
if (isProfiling) {
293
this._dataBackends.splice(0);
294
this._dataFrontend = null;
295
+ this._initialRendererIDs.clear();
296
this._initialSnapshotsByRootID.clear();
297
this._inProgressOperationsByRootID.clear();
298
this._inProgressScreenshotsByRootID.clear();
299
this._rendererQueue.clear();
300
301
+ // Record all renderer IDs initially too (in case of unmount)
302
+ for (let rendererID of this._store.rootIDToRendererID.values()) {
303
+ if (!this._initialRendererIDs.has(rendererID)) {
304
+ this._initialRendererIDs.add(rendererID);
305
+ }
306
+ }
307
+
308
// Record snapshot of tree at the time profiling is started.
309
// This info is required to handle cases of e.g. nodes being removed during profiling.
310
this._store.roots.forEach(rootID => {
@@ -309,13 +331,13 @@ export default class ProfilerStore extends EventEmitter {
331
this._dataBackends.splice(0);
332
this._rendererQueue.clear();
333
312
- for (let rendererID of this._store.rootIDToRendererID.values()) {
334
+ this._initialRendererIDs.forEach(rendererID => {
335
if (!this._rendererQueue.has(rendererID)) {
336
this._rendererQueue.add(rendererID);
337
338
this._bridge.send('getProfilingData', { rendererID });
339
}
318
- }
340
+ });
341
342
this.emit('isProcessingData');
343
}
src/devtools/store.js
-1
@@ -860,7 +860,6 @@ export default class Store extends EventEmitter {
860
861
if (haveRootsChanged) {
862
this._hasOwnerMetadata = false;
863
- this._supportsProfiling = false;
863
this._rootIDToCapabilities.forEach(
864
({ hasOwnerMetadata, supportsProfiling }) => {
865
if (hasOwnerMetadata) {
src/devtools/views/Profiler/CommitTreeBuilder.js
+3
-2
@@ -267,10 +267,11 @@ function updateTree(
267
268
nodes.delete(id);
269
270
- const parentNode = getClonedNode(parentID);
271
- if (parentNode == null) {
270
+ if (!nodes.has(parentID)) {
271
// No-op
272
} else {
273
+ const parentNode = getClonedNode(parentID);
274
+
275
if (__DEBUG__) {
276
debug('Remove', `fiber ${id} from parent ${parentID}`);
277
}
src/devtools/views/Profiler/FlamegraphChartBuilder.js
+37
-31
@@ -117,42 +117,48 @@ export function getChartData({
117
return chartNode;
118
};
119
120
- // Skip over the root; we don't want to show it in the flamegraph.
121
- const root = nodes.get(rootID);
122
- if (root == null) {
123
- throw Error(`Could not find root node with id "${rootID}" in commit tree`);
124
- }
125
-
126
- // Don't assume a single root.
127
- // Component filters or Fragments might lead to multiple "roots" in a flame graph.
120
let baseDuration = 0;
129
- for (let i = root.children.length - 1; i >= 0; i--) {
130
- const id = root.children[i];
131
- const node = nodes.get(id);
132
- if (node == null) {
133
- throw Error(`Could not find node with id "${id}" in commit tree`);
134
- }
135
- baseDuration += node.treeBaseDuration;
136
- walkTree(id, baseDuration, 1);
137
- }
121
139
- fiberActualDurations.forEach((duration, id) => {
140
- const node = nodes.get(id);
141
- if (node != null) {
142
- let currentID = node.parentID;
143
- while (currentID !== 0) {
144
- if (renderPathNodes.has(currentID)) {
145
- // We've already walked this path; we can skip it.
146
- break;
147
- } else {
148
- renderPathNodes.add(currentID);
149
- }
122
+ // Special case to handle unmounted roots.
123
+ if (nodes.size > 0) {
124
+ // Skip over the root; we don't want to show it in the flamegraph.
125
+ const root = nodes.get(rootID);
126
+ if (root == null) {
127
+ throw Error(
128
+ `Could not find root node with id "${rootID}" in commit tree`
129
+ );
130
+ }
131
151
- const node = nodes.get(currentID);
152
- currentID = node != null ? node.parentID : 0;
132
+ // Don't assume a single root.
133
+ // Component filters or Fragments might lead to multiple "roots" in a flame graph.
134
+ for (let i = root.children.length - 1; i >= 0; i--) {
135
+ const id = root.children[i];
136
+ const node = nodes.get(id);
137
+ if (node == null) {
138
+ throw Error(`Could not find node with id "${id}" in commit tree`);
139
}
140
+ baseDuration += node.treeBaseDuration;
141
+ walkTree(id, baseDuration, 1);
142
}
155
- });
143
+
144
+ fiberActualDurations.forEach((duration, id) => {
145
+ const node = nodes.get(id);
146
+ if (node != null) {
147
+ let currentID = node.parentID;
148
+ while (currentID !== 0) {
149
+ if (renderPathNodes.has(currentID)) {
150
+ // We've already walked this path; we can skip it.
151
+ break;
152
+ } else {
153
+ renderPathNodes.add(currentID);
154
+ }
155
+
156
+ const node = nodes.get(currentID);
157
+ currentID = node != null ? node.parentID : 0;
158
+ }
159
+ }
160
+ });
161
+ }
162
163
const chartData = {
164
baseDuration,