1200853 - Generate tree-table structure from heap census breakdown

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1200853-census-middleware.patch

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 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1200853-census-middleware.patch

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1200853-census-middleware.patch

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!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/db8602cd98b1

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db8602cd98b1