Closed
Bug 1201213
Opened 9 years ago
Closed 9 years ago
Add a worker for running offline heap analyses
Categories
(DevTools :: Memory, defect)
DevTools
Memory
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: fitzgen, Assigned: fitzgen)
References
Details
Attachments
(1 file, 2 obsolete files)
13.19 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8656202 -
Flags: review?(jsantell)
Assignee | ||
Updated•9 years ago
|
Blocks: memory-tools-fx44
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5505a8f3df0
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8656202 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
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+
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8656419 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8656846 [details] [diff] [review] Add a HeapAnalyses{Worker,Client} for running heap analyses Just a rebase.
Attachment #8656846 -
Flags: review+
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bb0fbbc4d84b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•