Comment on attachment 8656202 [details] [diff] [review] Add a HeapAnalyses{Worker,Client} for running heap analyses Review of attachment 8656202 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/heapsnapshot/HeapAnalysesClient.js @@ +65,5 @@ > + * @returns Promise<census report> > + * The report generated by the given census breakdown. > + */ > +HeapAnalysesClient.prototype.takeCensus = function (snapshotFilePath, > + censusOptions=undefined) { Does this need default arg of `undefined`? Should also just all be on the same line, we don't have 80 char limit ::: toolkit/devtools/heapsnapshot/HeapAnalysesWorker.js @@ +18,5 @@ > +/** > + * @see HeapAnalysesClient.prototype.readHeapSnapshot > + */ > +workerHelper.createTask(self, "readHeapSnapshot", ({ snapshotFilePath }) => { > + snapshots[snapshotFilePath] = same line @@ +19,5 @@ > + * @see HeapAnalysesClient.prototype.readHeapSnapshot > + */ > +workerHelper.createTask(self, "readHeapSnapshot", ({ snapshotFilePath }) => { > + snapshots[snapshotFilePath] = > + ThreadSafeChromeUtils.readHeapSnapshot(snapshotFilePath); Where does ThreadSafeChromeUtils come from? Worker global? I'd make a comment referencing where/what this is. ::: toolkit/devtools/heapsnapshot/tests/unit/test_HeapAnalyses_readHeapSnapshot_01.js @@ +10,5 @@ > +add_task(function* () { > + const client = new HeapAnalysesClient(); > + > + const snapshotFilePath = saveNewHeapSnapshot(); > + yield client.readHeapSnapshot(snapshotFilePath); Can this test anything else other than it not throwing? ::: toolkit/devtools/heapsnapshot/tests/unit/test_HeapAnalyses_takeCensus_02.js @@ +15,5 @@ > + yield client.readHeapSnapshot(snapshotFilePath); > + ok(true, "Should have read the heap snapshot"); > + > + const report = yield client.takeCensus(snapshotFilePath, { > + breakdown: { by: "count" } Let's preemptively make this work without defaults { by: "count", count: true, bytes: true }

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5505a8f3df0

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2) > ::: > toolkit/devtools/heapsnapshot/tests/unit/ > test_HeapAnalyses_readHeapSnapshot_01.js > @@ +10,5 @@ > > +add_task(function* () { > > + const client = new HeapAnalysesClient(); > > + > > + const snapshotFilePath = saveNewHeapSnapshot(); > > + yield client.readHeapSnapshot(snapshotFilePath); > > Can this test anything else other than it not throwing? Not really, the client API doesn't expose anything else and I don't really feel like it should. Note that the other test handles the failure cases. > ::: > toolkit/devtools/heapsnapshot/tests/unit/test_HeapAnalyses_takeCensus_02.js > @@ +15,5 @@ > > + yield client.readHeapSnapshot(snapshotFilePath); > > + ok(true, "Should have read the heap snapshot"); > > + > > + const report = yield client.takeCensus(snapshotFilePath, { > > + breakdown: { by: "count" } > > Let's preemptively make this work without defaults { by: "count", count: > true, bytes: true } Good call.

Comment on attachment 8656419 [details] [diff] [review] Add a HeapAnalyses{Worker,Client} for running heap analyses https://treeherder.mozilla.org/#/jobs?repo=try&revision=10c11f07dee4

Comment on attachment 8656846 [details] [diff] [review] Add a HeapAnalyses{Worker,Client} for running heap analyses Just a rebase.