Ability to view retaining paths of nodes in census category

RESOLVED FIXED in Firefox 48

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

({dev-doc-complete})

unspecified
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

1.63 MB, image/gif
Details
12.84 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
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.
Has STR: --- → irrelevant
Priority: -- → P2
Depends on: 1250307
Priority: P2 → P1
Depends on: 1257687
Depends on: 1257696
Depends on: 1260261
Depends on: 1260307
Depends on: 1260589
Depends on: 1260590
Depends on: 1260939
Depends on: 1260938
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Depends on: 1263270
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.
Attachment #8739558 - Flags: review?(jsantell)
Mechanical renaming. Just needs a rubber stamp.
Attachment #8739559 - Flags: review?(jsantell)
And the meat of it all.
Attachment #8739560 - Flags: review?(jsantell)
Posted image individuals.gif
Demo!
Fix typo.
Attachment #8739566 - Flags: review?(jsantell)
Attachment #8739560 - Attachment is obsolete: true
Attachment #8739560 - Flags: review?(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
Attachment #8739558 - Flags: review?(jsantell) → review+
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?
Attachment #8739559 - Flags: review?(jsantell) → review+
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
Attachment #8739566 - Flags: review?(jsantell) → review+
Rolled up the patches into one and addressed review comments.
Attachment #8740141 - Flags: review+
Attachment #8739558 - Attachment is obsolete: true
Attachment #8739559 - Attachment is obsolete: true
Attachment #8739566 - Attachment is obsolete: true
Fix test failures introduced when rebasing + fixing up.
Attachment #8740214 - Flags: review+
Attachment #8740141 - Attachment is obsolete: true

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4cbe058eacf8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Can someone explain the meaning of multiple "# TODO FITZGEN" in memory.properties? 

I also wonder about the accessibility of characters like ← or ⁂
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.
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.
Attachment #8740635 - Flags: review?(jimb)
Attachment #8740214 - Attachment is obsolete: true

Comment 19

3 years ago
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"
Attachment #8740635 - Flags: review?(jimb) → review+
Attachment #8740635 - Attachment is obsolete: true

Updated

3 years ago
Duplicate of this bug: 738973

Updated

3 years ago
Keywords: dev-doc-needed
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!
Flags: needinfo?(nfitzgerald)
Looks great! Thanks Will!
Flags: needinfo?(nfitzgerald)

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.