Closed Bug 1214231 Opened 9 years ago Closed 9 years ago

HeapAnalysesWorker should respond with census tree nodes rather than census reports

Categories

(DevTools :: Memory, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: fitzgen, Assigned: jsantell)

References

Details

Attachments

(1 file)

The census tree node is designed to be fed into a tree view widget and be super easy to render. It's like a lower level IR between the initial report generated by taking a census and the final result of a tree UI displayed to the user.

Because going from census report => census tree node doesn't touch the DOM and is a pure function, we should really do it in the worker rather than on the main thread. We talked about this while writing the census tree node stuff, but haven't actually put our money where our mouth was, so to speak. We should do that.

In summary, the situation right now is (or is on the way towards being):

1. Main thread sends census request to worker
2. Worker takes census of snapshot to generate report
3. Worker replies to main thread with generated census report
4. Main thread turns report into census tree node
5. Main thread renders tree widget UI with census tree node

But (4) doesn't need to happen on the main thread, is O(size of report) and therefore potentially slow, and therefore we should avoid janking the UI and do it in the worker like so:

1. Main thread sends census request to worker
2. Worker takes census of snapshot to generate report
3. Worker turns report into census tree node
4. Worker replies to main thread with census tree node
5. Main thread renders tree widget UI with census tree node
Notes to take into account:

1) existing tests that expect a report response rather than a node response (maybe an optional 'asTreeNode' bool option)
2) module/worker wiring -- make sure that importScripts() works in a worker with this module
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Comment on attachment 8673338 [details] [diff] [review]
1214231-census-tree-node-from-worker.patch

Review of attachment 8673338 [details] [diff] [review]:
-----------------------------------------------------------------

r=me!

::: devtools/shared/heapsnapshot/HeapAnalysesClient.js
@@ +73,2 @@
>   * @returns Promise<census report>
>   *          The report generated by the given census breakdown.

Should probably mention the asTreeNode flag for the @returns as well.

> @returns Promise<census report> | Promise<census tree node>
>          Either the census report or the tree node generated from the report. See
>          the `asTreeNode` option above.

@@ +79,4 @@
>    return this._worker.performTask("takeCensus", {
>      snapshotFilePath,
> +    censusOptions,
> +    asTreeNode: requestOptions.asTreeNode,

You prefer to unpack eagerly here? I have no preference, but was mildly surprised we aren't just sending the requestOptions across the same way we do with censusOptions.

::: devtools/shared/heapsnapshot/tests/unit/test_HeapAnalyses_takeCensus_05.js
@@ +37,5 @@
> +           "count" in type;
> +  }), "all of tree node's children have name, bytes, count");
> +
> +  compareCensusViewData(BREAKDOWN, report, treeNode,
> +    "Returning census as a tree node represents same data as the report");

Awesome, thank you for this check!
Attachment #8673338 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8673338 [details] [diff] [review]
1214231-census-tree-node-from-worker.patch

Review of attachment 8673338 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/heapsnapshot/HeapAnalysesClient.js
@@ +73,2 @@
>   * @returns Promise<census report>
>   *          The report generated by the given census breakdown.

Good call

@@ +79,4 @@
>    return this._worker.performTask("takeCensus", {
>      snapshotFilePath,
> +    censusOptions,
> +    asTreeNode: requestOptions.asTreeNode,

I could go either way, just didn't want to merge them into the censusOptions -- I'll just pass the whole requestOptions in
https://hg.mozilla.org/mozilla-central/rev/c8e4c24e3f00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: