1229229 - Display dominator trees in the UI

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

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

This will involve an interesting interaction between the HeapAnalysesWorker and the frontend as we shouldn't copy the whole dominator tree into the JS heap because it can just get too big. Will have some fun request/response protocols here and lazy loading of children and such things. Haven't thought it all through 100% yet.

Comment 1

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

Attachment: WIP: Display dominator trees in the memory tool's UI

Comment 2

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

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #1)
> Created attachment 8703849 [details] [diff] [review]
> WIP: Display dominator trees in the memory tool's UI

This is almost in a ship-able state. I've tried to factor out various things into blocking bugs as much as possible for reviewers' sakes. There were some changes to the Tree component that had to happen atomically with this commit, unfortunately, such as making the focused node and expanded set explicit props rather than implicit internal state.

Major TODOs still standing are:

* Support incrementally fetching more than one subtree at once (or adding some kind of queuing mechanism). Right now, concurrent requests to fetch and load lazy subtrees will result in a mismatched state (which is caught by existing assertions \o/ woo!).

* Replace the toggle-diffing-on-or-off action and reducers with the new toggle-the-current-active-view action and reducers which consider census, diffing, or dominator tree. The code right now is an abhorrent amalgamation of the old diffing toggling and the new "current active view" switching with some assumptions about doing them both in a certain order. I don't want to ship that mess :-P

* Thorough integration tests and unit tests for the actions+reducers.

Comment 3

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

Attachment: WIP: Display dominator trees in the memory tool's UI

Now supports incrementally fetching multiple subtrees at once.

Replaces the diffing toggle with the current-view-selection machinery.

Lots of tests here. Still lots more to write.

Comment 4

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

Attachment: Display dominator trees in the memory tool's UI

This patch overhauls the memory tool's frontend from being in a state of viewing
a snapshot's census (the implicit default) or viewing a census diff (a special
cased kind of thing) to one of three recognized view states: (1) census, (2)
diffing, or (3) dominator tree. The logic surrounding our current view is more
explicit now. View option (3) is the new one, whose introduction required those
clean ups.

Dominator trees are too large to render in full, all at once. Instead, we
eagerly load and render a few levels deep and then incrementally fetch further
subtrees as their parents are expanded in the UI. This means that we get new
Tree component instances across incremental fetches. Before this commit, the
Tree component stored a bunch of state internally (in this.state rather than
this.props) and we would lose focus and expansion state across incremental
fetches. This internal state has been pulled out and made as explicit props,
which are now managed by actions and reducers just like the rest of the state.

Comment 5

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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d332a4cf88

The new integration test will likely fail on DEBUG builds because of bug 1219554, but I figure we may as well give it a shot.

Comment 6

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

Key feature for the memory tool.

Comment 8

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

Comment on attachment 8705416 [details] [diff] [review]
Display dominator trees in the memory tool's UI

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

Here's a giant chunk of comments, up to the first test in the list

::: devtools/client/locales/en-US/memory.properties
@@ +69,5 @@
> +toolbar.view=View:
> +
> +# LOCALIZATION NOTE (toolbar.view.census): The label for the census view option
> +# in the toolbar.
> +toolbar.view.census=Census

Is there a more user friendly way to describe this? "Aggregates" or something?

@@ +195,5 @@
> +dominatorTree.state.error=Error
> +
> +# LOCALIZATION NOTE (dominatorTree.state.error): The label describing the
> +# dominator tree state ERROR, used in the dominator tree view.
> +dominatorTree.state.error.full=There was an error while processing the dominator tree

Should end with a ".", yeah? Our other similar error/warning messages have this, except "diffing.state.error.full" which I probably missed (and probably should have one)

::: devtools/client/memory/actions/diffing.js
@@ +15,5 @@
>  /**
>   * Toggle diffing mode on or off.
>   */
>  const toggleDiffing = exports.toggleDiffing = function () {
> +  return function*(dispatch, getState) {

We should not use a generator function here, just a function/thunk to use `getState` -- otherwise it's not obvious whether or not this is an async function resolved in the next tick if sometimes it's yielded, and other times not -- a pattern I see a few through this patch. I think some consistent rules would help with any race conditions:

* All generator actions must be yielded to
* Only use generator actions when necessary

This isn't just for `toggleDiffing` but many other action creators here.

::: devtools/client/memory/actions/snapshot.js
@@ +246,5 @@
> +    const snapshot = getSnapshot(getState(), id);
> +    assert(snapshot.dominatorTree, "Should have dominator tree model");
> +    assert(snapshot.dominatorTree.state === dominatorTreeState.COMPUTED ||
> +           snapshot.dominatorTree.state === dominatorTreeState.LOADED ||
> +           snapshot.dominatorTree.state === dominatorTreeState.INCREMENTAL_FETCHING,

Same with snapshot states, maybe should have a helper, `isDomTreeComputed(dominatorTree)`, checking if it matches any of the states

@@ +479,5 @@
> +  return {
> +    type: actions.FOCUS_DOMINATOR_TREE_NODE,
> +    id,
> +    node,
> +  };

Consider pulling out dominator tree actions into its own action file, this is getting long and relatively contained

::: devtools/client/memory/actions/view.js
@@ +12,5 @@
> +    view
> +  };
> +};
> +
> +exports.changeViewAndRefresh = function (view, heapWorker) {

Comments, what type is "view"?

@@ +15,5 @@
> +
> +exports.changeViewAndRefresh = function (view, heapWorker) {
> +  return function* (dispatch, getState) {
> +    dispatch(changeView(view));
> +    dispatch(refresh.refresh(heapWorker));

yield, `refresh` is async

::: devtools/client/memory/app.js
@@ +28,5 @@
> +  focusCensusNode,
> +  expandDominatorTreeNode,
> +  collapseDominatorTreeNode,
> +  focusDominatorTreeNode,
> +} = require("./actions/snapshot");

Consider undestructuring these actions, we're starting to have a lot here and may be more legible in the code to have `SnapshotAction.focusCensusNode`

@@ +199,5 @@
> +                     "If focusing dominator tree nodes, should be in dominator tree view");
> +              assert(selectedSnapshot, "...and we should have a selected snapshot");
> +              assert(selectedSnapshot.dominatorTree,
> +                     "...and that snapshot should have a dominator tree");
> +              dispatch(focusDominatorTreeNode(selectedSnapshot.id, node));

all those assertions <3

::: devtools/client/memory/components/DominatorTreeItem.js
@@ +8,5 @@
> +const Frame = createFactory(require("devtools/client/shared/components/frame"));
> +const { TREE_ROW_HEIGHT } = require("../constants");
> +
> +const Rsaquo = createFactory(createClass({
> +  displayName: "Rsaquo",

what

@@ +11,5 @@
> +const Rsaquo = createFactory(createClass({
> +  displayName: "Rsaquo",
> +
> +  render() {
> +    return dom.span({ className: "rsaquo" }, "›");

what

@@ +35,5 @@
> +
> +  formatNumber(number) {
> +    const rounded = Math.round(number);
> +    if (rounded === 0 || rounded === -0) {
> +      return "0";

This returns both strings and numbers, can't we just have if rounded === -0, then rounded = 0?

@@ +63,5 @@
> +
> +    const shallowSize = this.formatNumber(item.shallowSize);
> +    const percentShallowSize = this.formatPercent(getPercentSize(item.shallowSize));
> +
> +    const label = Array(item.label.length * 2 - 1).fill(undefined);

This and the for loop need more comments, I'm not sure what's going on here

@@ +84,5 @@
> +        label[i * 2] = piece;
> +      }
> +
> +      if (i < length - 1) {
> +        label[i * 2 + 1] = Rsaquo({ key: `${item.nodeId}-rsaquo-${i}` });

what is this rsaquo again

@@ +127,5 @@
> +        },
> +        arrow,
> +        label,
> +        dom.span({ className: "heap-tree-item-address" },
> +                 `@ 0x${item.nodeId.toString(16)}`)

Does this have to be localized? Hex addresses probably not

::: devtools/client/memory/components/moz.build
@@ +9,5 @@
> +    'CensusTreeItem.js',
> +    'DominatorTree.js',
> +    'DominatorTreeHeader.js',
> +    'DominatorTreeItem.js',
> +    'Heap.js',

Why are these all capitalized now? Outside of some server things or JSMs, all of our file names are lower cased, hyphenated, consistent with many other style guides (node, browserify for one), this requires knowing an extra bit of knowledge whether or not the file is capitalized when `require`ing it. I feel rather strongly about leaving these lowercased -- what's the reason against that?

::: devtools/client/memory/components/toolbar.js
@@ +155,4 @@
>      return (
> +      dom.div(
> +        {
> +          className: "devtools-toolbar"

Why Java style curly brackets in these? I understand maybe if it's the only thing in a row (I think that occurred somewhere else in the patch), but these just seem like excessive white space, and doesn't match our style anywhere else.

::: devtools/client/memory/models.js
@@ +17,5 @@
> + * ONLY USE THIS FOR MODEL VALIDATORS IN CONJUCTION WITH assert()!
> + *
> + * React checks that the returned values from validator functions are instances
> + * of Error, but because React is loaded in its own global, that check is always
> + * false and always results in a warning.

this is terrifying

::: devtools/client/memory/reducers/snapshots.js
@@ +69,5 @@
>  
>  handlers[actions.TAKE_CENSUS_END] = function (snapshots, { id, report, breakdown, inverted, filter }) {
>    const census = {
>      report,
> +    expanded: new Set(),

I didn't know we could have Sets in state? How does this get serialized, any idea? Wonder if this would cause issues with react tooling or something.

@@ +95,5 @@
> +
> +    // Warning: mutable operations couched in an immutable update ahead :'( This
> +    // at least lets us use referential equality on the census model itself,
> +    // even though the expanded set is mutated in place.
> +    const expanded = snapshot.census.expanded;

We can do `new Set(snapshot.census.expanded)` to get a new "cloned" Set object, which would fix this mutability, yeah?

@@ +115,5 @@
> +
> +    // Warning: mutable operations couched in an immutable update ahead :'( See
> +    // above comment in the EXPAND_CENSUS_NODE handler.
> +    const expanded = snapshot.census.expanded;
> +    expanded.delete(node.id);

`new Set(snapshot.census.expanded)`

@@ +251,5 @@
> +           "Should have the dominator tree's expanded set");
> +
> +    // Warning: mutable operations couched in an immutable update ahead :'( See
> +    // above comment in the EXPAND_DOMINATOR_TREE_NODE handler.
> +    const expanded = snapshot.dominatorTree.expanded;

`new Set(oldSet)`

@@ +296,5 @@
> +  });
> +};
> +
> +handlers[actions.FETCH_IMMEDIATELY_DOMINATED_END] = function (snapshots, {
> +  id,

We don't do this formatting anywhere else -- at the very least, the function is on a new line, idented (@see setDominatorTreeBreakdownAndRefresh)

@@ +363,5 @@
>  
>  module.exports = function (snapshots = [], action) {
>    const handler = handlers[action.type];
>    if (handler) {
>      return handler(snapshots, action);

With this module.exports reducer pattern, maybe we can add asserts that we're returning a new object, like:

if (handler) {
  let ret = handler(snapshots, action);
  assert(ret !== snapshots);
  return ret;
}

Might be something we can easily miss?

::: devtools/client/memory/store.js
@@ +15,5 @@
>    // If testing, store the action history in an array
>    // we'll later attach to the store
>    if (DevToolsUtils.testing) {
>      history = [];
> +    shouldLog = true;

should be false?

Comment 9

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

Comment on attachment 8705416 [details] [diff] [review]
Display dominator trees in the memory tool's UI

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

As a high level thought, does moving the expand/collapse state out of the component, into the store, mean that other consumers of the Tree component will also need to manually handle that themselves? Or is it opt in behaviour to handle this manually?

Removing review -- for the most part, great stuff! Very excited to get this landed. If my comments/concerns (async action creator consistency, lowercase-and-hyphenated-file-names-dot-js, a few follow ups, answering some questions I have on the reasoning behind some lines) addressed, I'll either review it once more, or rather, address the comments and we'll go from there -- but not too much left to do, I think. Hot.

::: devtools/client/memory/test/browser/browser_memory_dominator_trees_01.js
@@ +147,5 @@
> +  EventUtils.synthesizeKey("VK_RIGHT", {}, panel.panelWin);
> +  yield waitUntilState(store, state =>
> +    state.snapshots[0].dominatorTree.focused === firstChild);
> +  ok(doc.querySelector(`.node-${firstChild.nodeId}`).classList.contains("focused"),
> +     "The first child should now be focused");

Dang. Nice.

::: devtools/client/memory/test/chrome/chrome.ini
@@ +7,5 @@
> +[test_Heap_01.html]
> +[test_Heap_02.html]
> +[test_Heap_03.html]
> +[test_Toolbar_01.html]
> +[test_Toolbar_02.html]

Awesome that we're testing the tool's non-shareable components directly as well!

::: devtools/client/memory/test/chrome/head.js
@@ +74,5 @@
> +  return node;
> +}
> +
> +var TEST_DOMINATOR_TREE = Object.freeze({
> +  dominatorTreeId: 666,

\m/

::: devtools/client/memory/utils.js
@@ +173,5 @@
> + *
> + * @param {String} name
> + * @return {Object}
> + */
> +exports.dominatorTreeBreakdownNameToSpec = function (name) {

All these dominator breakdown utils -- might make sense to just reuse the current utils and pass in a { dominator: true } flag or something?

::: devtools/client/themes/memory.css
@@ +421,5 @@
>  /**
>   * Frame View components
>   */
>  
> +.rsaquo,

this is the worst class name

Comment 10

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

We taked about everything else in person, but we didn't mention this one:

> Why Java style curly brackets in these?

Have never heard this referred to as "java style" but, most editors mess up the old style. This was mostly what we had:

> foobar({ opt1: "one", opt2: "two" },
>   baz,
>   bang
> )

However, most editors want to align the arguments to a function by default, like this:

> foobar({ opt1: "one", opt2: "two" },
>        baz,
>        bang
>       )

This was too much for me. So I just made sure that all arguments are on the same (minimal) indentation level for my own ease of use.

Comment 11

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

Attachment: Display dominator trees in the memory tool's UI

This patch overhauls the memory tool's frontend from being in a state of viewing
a snapshot's census (the implicit default) or viewing a census diff (a special
cased kind of thing) to one of three recognized view states: (1) census, (2)
diffing, or (3) dominator tree. The logic surrounding our current view is more
explicit now. View option (3) is the new one, whose introduction required those
clean ups.

Dominator trees are too large to render in full, all at once. Instead, we
eagerly load and render a few levels deep and then incrementally fetch further
subtrees as their parents are expanded in the UI. This means that we get new
Tree component instances across incremental fetches. Before this commit, the
Tree component stored a bunch of state internally (in this.state rather than
this.props) and we would lose focus and expansion state across incremental
fetches. This internal state has been pulled out and made as explicit props,
which are now managed by actions and reducers just like the rest of the state.

Comment 12

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

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04ec928b857f

Comment 13

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

Attachment: Display dominator trees in the memory tool's UI

This patch overhauls the memory tool's frontend from being in a state of viewing
a snapshot's census (the implicit default) or viewing a census diff (a special
cased kind of thing) to one of three recognized view states: (1) census, (2)
diffing, or (3) dominator tree. The logic surrounding our current view is more
explicit now. View option (3) is the new one, whose introduction required those
clean ups.

Dominator trees are too large to render in full, all at once. Instead, we
eagerly load and render a few levels deep and then incrementally fetch further
subtrees as their parents are expanded in the UI. This means that we get new
Tree component instances across incremental fetches. Before this commit, the
Tree component stored a bunch of state internally (in this.state rather than
this.props) and we would lose focus and expansion state across incremental
fetches. This internal state has been pulled out and made as explicit props,
which are now managed by actions and reducers just like the rest of the state.

Comment 14

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

New iteration has the file naming changes requested, eg CensusHeader.js => census-header.js

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3642a82c41fd

Comment 15

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

Comment on attachment 8707233 [details] [diff] [review]
Display dominator trees in the memory tool's UI

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

r+, ensure that logging is really what you want, but I'd guess there's overhead there if intentional

::: devtools/client/memory/reducers/snapshots.js
@@ +96,5 @@
> +    // Warning: mutable operations couched in an immutable update ahead :'( This
> +    // at least lets us use referential equality on the census model itself,
> +    // even though the expanded set is mutated in place.
> +    const expanded = snapshot.census.expanded;
> +    expanded.add(node.id);

Follow up bug for investigating performance of things like this, and see if we can continue making this immutable if perf is there

::: devtools/client/memory/reducers/view.js
@@ +6,5 @@
> +const { actions, viewState } = require("../constants");
> +
> +const handlers = Object.create(null);
> +
> +handlers[actions.CHANGE_VIEW] = function (_, { view }) {

There are a few (only 2 I guess?) of these simple reducers, could be written like `= (_, { view }) => view;`

::: devtools/client/memory/store.js
@@ +15,5 @@
>    // If testing, store the action history in an array
>    // we'll later attach to the store
>    if (DevToolsUtils.testing) {
>      history = [];
> +    shouldLog = true;

This logs and JSON.stringifies the action, which could be large, no? Is there a reason this is true for production?

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a531aa40e49

Comment 18

[Tim Nguyen :ntim]

Release Note Request (optional, but appreciated)
[Why is this notable]: New Memory Tool feature
[Suggested wording]: Dominator trees in Memory Tool
[Links (documentation, blog post, etc)]:

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Noted for Aurora 46 as "Display dominator trees in Memory tool" with a link to the basic Memory tool page in MDN.

Comment 20

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Noted for Aurora 46 as "Display dominator trees in Memory tool" with a link
> to the basic Memory tool page in MDN.

Thanks, that sounds great! We should have some docs for this feature on MDN soon.

Comment 21

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/docs/Tools/Memory/Dominators_view

Nick's already reviewed this stuff, so I'll just mark is dev-doc-complete.

Comment 22

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID	 	20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected