1249788 - Ability to view retaining paths of nodes in census category

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

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

When working with the leaking MouseEvents in https://github.com/TatumCreative/bunny-memory-leak, it was frustrating that we could see that MouseEvents were leaking but couldn't view their retained paths. Retaining paths only make sense for individual objects, and the only view we have dealing with individual objects right now is the dominator tree, so bug 1149385 only adds shortest retaining paths for dominator tree items. The problem is the dominator tree sorts nodes by largest retained size and the MouseEvents were small in comparison.

See also bug 1243091, which is the best long term solution for leaks, but we may be able to get some short term wins with this bug.

Comment 1

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

Attachment: Part 0: Add the INDIVIDUALS view state to the memory panel

Note that this patch is stand alone enough that extant tests pass, but the
INDIVIDUALS view state changes a little more in later patches. Still hopefully
easier to review with this split out.

Comment 2

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

Attachment: Part 1: Rename "dominator tree display" to "label display"

Mechanical renaming. Just needs a rubber stamp.

Comment 3

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

Attachment: Part 2: Implement the census individuals view

And the meat of it all.

Comment 4

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

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

Comment 5

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

Attachment: individuals.gif

Demo!

Comment 6

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

Attachment: Part 2: Implement the census individuals view

Fix typo.

Comment 7

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

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

Comment 8

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

Comment on attachment 8739558 [details] [diff] [review]
Part 0: Add the INDIVIDUALS view state to the memory panel

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

::: devtools/client/memory/components/toolbar.js
@@ +144,5 @@
>              )
>            )
>          : null;
> +    } else if (view.state === viewState.INDIVIDUALS){
> +      assert(false, "TODO FITZGEN");

Should have a helpful assertion here

::: devtools/client/memory/reducers/view.js
@@ +20,5 @@
> +  const { newViewState, oldDiffing, oldSelected } = action;
> +  assert(newViewState);
> +
> +  if (newViewState === viewState.INDIVIDUALS) {
> +    assert(oldDiffing || oldSelected);

needs second arg

Comment 9

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

Comment on attachment 8739559 [details] [diff] [review]
Part 1: Rename "dominator tree display" to "label display"

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

No l10n changes needed here?

Comment 10

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

Comment on attachment 8739566 [details] [diff] [review]
Part 2: Implement the census individuals view

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

::: devtools/shared/heapsnapshot/CensusUtils.js
@@ +437,5 @@
>   * @overrides Visitor.prototype.enter
>   */
>  GetLeavesVisitor.prototype.enter = function(breakdown, report, edge) {
>    this._index++;
> +  console.log("FITZGEN: GetLeavesVisitor/enter:", report);

oops, few console logs in this file

Comment 11

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

Attachment: Implement the census individuals view

Rolled up the patches into one and addressed review comments.

Comment 12

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

Attachment: Implement the census individuals view

Fix test failures introduced when rebasing + fixing up.

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4cbe058eacf8

Comment 15

[Francesco Lodolo [:flod]]

Can someone explain the meaning of multiple "# TODO FITZGEN" in memory.properties? 

I also wonder about the accessibility of characters like ← or ⁂

Comment 16

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

D'oh, that's what I get for grepping for TODO in devtools/client/memory and not in all of devtools proper :-/

Will land a follow up soon.

Comment 17

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

Attachment: Follow up: remove debug logs and TODO comments that should not have landed

Jim, I'm passing this review to you since Jordan has left us. Small follow up
for some really dumb logs/comments that should not have landed with the original
patch.

Comment 18

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

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

Comment 19

[Jim Blandy :jimb]

Comment on attachment 8740635 [details] [diff] [review]
Follow up: remove debug logs and TODO comments that should not have landed

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

Just some typos.

::: devtools/client/locales/en-US/memory.properties
@@ +181,5 @@
>  # tool's filter search box.
>  filter.tooltip=Filter the contents of the heap snapshot
>  
> +# LOCALIZATION NOTE (tree-tiem.view-individuals.tooltip): The tooltip for the
> +# button to view individuals in this group.

"tree-tiem"

@@ +311,5 @@
>  # snapshot state ERROR, used in the main heap view.
>  snapshot.state.error.full=There was an error processing this snapshot.
>  
> +# LOCALIZATION NOTE (individuals.state.error): The short message displayed when
> +# there is an error fetching indiduals from a group.

"indiduals"

@@ +316,4 @@
>  individuals.state.error=Error
>  
> +# LOCALIZATION NOTE (individuals.state.error.full): The longer message displayed
> +# when there is an error fetching indiduals from a group.

"indiduals"

Comment 20

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

Attachment: Follow up: remove debug logs and TODO comments that should not have landed

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ae9a0d3be52

Comment 24

[Will Bamberg [:wbamberg]]

I've added some docs for this, here: https://developer.mozilla.org/en-US/docs/Tools/Memory/Aggregate_view#Type

Please let me know if this covers it!

Comment 25

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

Looks great! Thanks Will!