Closed Bug 1201213 Opened 9 years ago Closed 9 years ago

Add a worker for running offline heap analyses

Categories

(DevTools :: Memory, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

References

Details

Attachments

(1 file, 2 obsolete files)

      No description provided.
Depends on: 1201215
Depends on: 1200821
Attachment #8656202 - Flags: review?(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 }
Attachment #8656202 - Flags: review?(jsantell) → review+
(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
Attachment #8656419 - Flags: review+
Comment on attachment 8656846 [details] [diff] [review]
Add a HeapAnalyses{Worker,Client} for running heap analyses

Just a rebase.
Attachment #8656846 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bb0fbbc4d84b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: