Closed
Bug 1200853
Opened 9 years ago
Closed 9 years ago
Generate tree-table structure from heap census breakdown
Categories
(DevTools :: Memory, defect)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(1 file, 2 obsolete files)
13.35 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Takes a census report and returns a structured version of it that maps each object to a line in a view:
{
children: [
{ name: "objects", bytes: 10, count: 1 },
{ name: "strings", bytes: 20, count: 2 },
]
}
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00edd2df58db
Attachment #8656253 -
Flags: review?(nfitzgerald)
Assignee | ||
Updated•9 years ago
|
Blocks: memory-tools-fx44
Comment 2•9 years ago
|
||
Comment on attachment 8656253 [details] [diff] [review]
1200853-census-middleware.patch
Review of attachment 8656253 [details] [diff] [review]:
-----------------------------------------------------------------
Can you rebase this on top of bug 1200821 (currently waiting on try push results before landing) and move browser/devtools/memory/* to toolkit/devtools/heapsnapshot/*? This is supposed to run in the worker that is in toolkit/devtools/heapsnapshot and toolkit can't depend on browser. Also, since this doesn't touch the dom or browser UI at all, it doesn't need to be under browser/. This will also let you avoid copying a few testing functions to the new head file.
Follow up bug for { by: "allocationStack" } please.
::: browser/devtools/memory/modules/census.js
@@ +6,5 @@
> +/**
> + * Utilities for interfacing with census reports from dbg.memory.takeCensus().
> + */
> +
> +const COARSE_TYPES = ["objects", "scripts", "strings", "other"];
This should be a Set since it is used for testing membership.
@@ +15,5 @@
> + * a tree to display the data.
> + *
> + * Returns a recursive "CensusViewData" object, looking like:
> + *
> + * {
To make this a tiny bit more obvious to readers, can we do:
CensusViewData = {
children: [<CensusViewData...>],
name: <?String>
count: <?Number>
bytes: <?Number>
}
Also, is there an order to the children? We should document whether there is or is not. (I haven't read the code yet, but I would suggest that we sort them by bytes.)
@@ +29,5 @@
> + * @param {?String} name
> + * @return {Object}
> + */
> +function createCensusViewData (breakdown, report, name) {
> + let result = Object.create(null);
These always have the same set of (sometimes optional) properties, can we pull it out into a class so it is easy to read for devs (and the JIT)? Like this:
function CensusViewData(name) {
this.name = name;
this.count = 0;
this.bytes = 0;
this.children = null;
}
This will let us see all the properties in one place, rather than reading through each branch, and will give the jit/TI really good info. It also lets this function just say it is returning a CensusViewData and we can talk about what that is and what structure it actually has in another place.
Also, we discussed renaming this to CensusTreeNode in person, I think I like that name better but it is up to you.
@@ +36,5 @@
> + result.name = name;
> + }
> +
> + switch (breakdown.by) {
> + case "internalType":
What I'm about to suggest involves a bit of code motion, but not really any logic changes, so I hope it shouldn't be difficult. I'd like to make this a more data-directed approach, rather than hard coding the cases. This will make extending it with more "by" kinds really easy in the future.
This is what I am thinking:
// by -> viewer function
const censusViewers = Object.create(null);
censusViewers.objectClass = function (breakdown, report, name=null) { ... };
censusViewers.coarseType = function (breakdown, report, name=null) { ... };
// ... etc ...
function createCensusViewData(breakdown, report, name=null) {
return censusViewers[breakdown.by](breakdown, report, name);
}
::: browser/devtools/memory/test/unit/test_census-01.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
Nit: test summary needed
::: browser/devtools/memory/test/unit/test_census-02.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
Nit: test summary needed
::: browser/devtools/memory/test/unit/test_census-03.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
Nit: test summary needed
Attachment #8656253 -
Flags: review?(nfitzgerald) → feedback+
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8656253 -
Attachment is obsolete: true
Attachment #8656326 -
Flags: review?(nfitzgerald)
Comment 4•9 years ago
|
||
Comment on attachment 8656326 [details] [diff] [review]
1200853-census-middleware.patch
Review of attachment 8656326 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the directory move I mentioned last review and the comment below addressed.
::: browser/devtools/memory/modules/census.js
@@ +32,5 @@
> +function CensusTreeNode (breakdown, report, name) {
> + this.name = name;
> + this.count = this.bytes = this.children = void 0;
> +
> + CensusTreeNodeBreakdowns[breakdown.by].call(this, breakdown, report);
Whoa, this is funky. I'm sorry if I implied it, but this is not what I was asking for in the last review. Can the CensusTreeNodeBreakdowns functions just return a CensusTreeNode rather than having CensusTreeNodeBreakdowns functions called from the constructor here with `this` and `.call` and all that?
I think the constructor should not do anything other than ensuring that children are sorted if present:
function CensusTreeNode(name=void 0, children=void 0) {
this.name = name;
this.count = void 0;
this.bytes = void 0;
this.children = children;
if (this.children) {
this.children.sort(sortByBytes);
}
}
and then the count CensusBreakdown (for example) would look like this:
CensusTreeNodeBreakdowns.count = function (breakdown, report, name) {
const node = new CensusTreeNode(name);
node.bytes = breakdown.bytes ? report.bytes : void 0;
node.count = breakdown.count ? report.count : void 0;
return node;
};
Attachment #8656326 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Made changes -- not exactly what you have, as that doesn't deal with the recursion necessary, but no longer deal with `this`, and nodes are passed in as args now.
With bug 1200821 not yet landed, I'd rather get this landed and can move into toolkit/devtools/heapsnapshots later rather than holding this up!
Attachment #8656326 -
Attachment is obsolete: true
Attachment #8656614 -
Flags: review+
Comment 7•9 years ago
|
||
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
•