Closed
Bug 1260939
Opened 8 years ago
Closed 8 years ago
Add a method for getting census individuals and their shortest paths to HeapAnalyses{Client,Worker}
Categories
(DevTools :: Memory, defect)
DevTools
Memory
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(1 file, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8736523 -
Flags: review?(jimb)
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Component: Developer Tools → Developer Tools: Memory
Assignee | ||
Comment 2•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9451c696ab0b
Comment 3•8 years ago
|
||
Comment on attachment 8736523 [details] [diff] [review] Add a method for getting census individuals and their shortest paths to HeapAnalyses{Client,Worker} Review of attachment 8736523 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Some testing suggestions. ::: devtools/shared/heapsnapshot/HeapAnalysesWorker.js @@ +113,5 @@ > + const nodeIds = CensusUtils.getCensusIndividuals(indices, censusBreakdown, snapshot); > + > + const nodes = nodeIds > + .sort((a, b) => dominatorTree.getRetainedSize(b) - dominatorTree.getRetainedSize(a)) > + .slice(0, Math.min(nodeIds.length, maxIndividuals)) I believe you can simply write `.slice(0, maxIndividuals)` here and it'll do the right thing. ::: devtools/shared/heapsnapshot/tests/unit/test_HeapAnalyses_getCensusIndividuals_01.js @@ +28,5 @@ > + const client = new HeapAnalysesClient(); > + > + const snapshotFilePath = saveNewHeapSnapshot(); > + yield client.readHeapSnapshot(snapshotFilePath); > + ok(true, "Should have read the heap snapshot"); I don't know the add_task stuff very well; I assume that if client.readHeapSnapshot(snapshotFilePath) fails, it uses the generator's `throw` method to report it to the task. So when we're trying to assert that a yielded operation succeeded, doesn't it make more sense to write something like this? try { yield client.readHeapSnapshot(azaleas); } catch { ok(false, "Should have read the heap snapshot"); } I mean, that's a face only a mother could love, but... @@ +39,5 @@ > + { asTreeNode: true }); > + ok(report, "Should get a report"); > + > + yield* (function* assertCanGetIndividuals(censusNode) { > + if (censusNode.reportLeafIndex !== undefined) { It would be nice to also assert that we saw any censusNodes with a real reportLeafIndex at all. @@ +54,5 @@ > + > + dumpn(`response = ${JSON.stringify(response, null, 4)}`); > + > + ok(response.nodes.length <= MAX_INDIVIDUALS, > + "response.nodes.length <= MAX_INDIVIDUALS"); Should we also assert that we don't get zero nodes, or that it matches the count we saw, if it's less than MAX_INDIVIDUALS?
Attachment #8736523 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #3) > devtools/shared/heapsnapshot/tests/unit/ > test_HeapAnalyses_getCensusIndividuals_01.js > @@ +28,5 @@ > > + const client = new HeapAnalysesClient(); > > + > > + const snapshotFilePath = saveNewHeapSnapshot(); > > + yield client.readHeapSnapshot(snapshotFilePath); > > + ok(true, "Should have read the heap snapshot"); > > I don't know the add_task stuff very well; I assume that if > client.readHeapSnapshot(snapshotFilePath) fails, it uses the generator's > `throw` method to report it to the task. So when we're trying to assert that > a yielded operation succeeded, doesn't it make more sense to write something > like this? > > try { > yield client.readHeapSnapshot(azaleas); > } catch { > ok(false, "Should have read the heap snapshot"); > } > > I mean, that's a face only a mother could love, but... The assert is really there just to provide a log message saying "we got this far" for debugging if/when this test starts failing. We do it in a few other places too, but could replace it with dumpn if we really wanted to. The task runner will catch and report uncaught errors, FWIW. > @@ +39,5 @@ > > + { asTreeNode: true }); > > + ok(report, "Should get a report"); > > + > > + yield* (function* assertCanGetIndividuals(censusNode) { > > + if (censusNode.reportLeafIndex !== undefined) { > > It would be nice to also assert that we saw any censusNodes with a real > reportLeafIndex at all. Good call. We should see exactly four nodes with a reportLeafIndex: 1. The objects leaf 2. The strings leaf 3. The scripts leaf 4. The other leaf > @@ +54,5 @@ > > + > > + dumpn(`response = ${JSON.stringify(response, null, 4)}`); > > + > > + ok(response.nodes.length <= MAX_INDIVIDUALS, > > + "response.nodes.length <= MAX_INDIVIDUALS"); > > Should we also assert that we don't get zero nodes, or that it matches the > count we saw, if it's less than MAX_INDIVIDUALS? Modifying the assertion to be: equal(response.nodes.length, Math.min(MAX_INDIVIDUALS, censusNode.count));
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8736880 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8736523 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•8 years ago
|
||
Note that this bug depends on another bug that is also marked checkin-needed
Comment 7•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/fx-team/rev/9c04c66d849b
Keywords: checkin-needed
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9c04c66d849b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 9•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #4) > > I don't know the add_task stuff very well; I assume that if > > client.readHeapSnapshot(snapshotFilePath) fails, it uses the generator's > > `throw` method to report it to the task. So when we're trying to assert that > > a yielded operation succeeded, doesn't it make more sense to write something > > like this? > > > > try { > > yield client.readHeapSnapshot(azaleas); > > } catch { > > ok(false, "Should have read the heap snapshot"); > > } > > > > I mean, that's a face only a mother could love, but... > > The assert is really there just to provide a log message saying "we got this > far" for debugging if/when this test starts failing. We do it in a few other > places too, but could replace it with dumpn if we really wanted to. > > The task runner will catch and report uncaught errors, FWIW. Okay, that seems just as good.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•