1201213 - Add a worker for running offline heap analyses

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a HeapAnalyses{Worker,Client} for running heap analyses

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

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 }

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a HeapAnalyses{Worker,Client} for running heap analyses

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a HeapAnalyses{Worker,Client} for running heap analyses

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Just a rebase.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bb0fbbc4d84b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb0fbbc4d84b