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

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

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; };

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!