Shortest path(s) from GC roots view

RESOLVED FIXED in Firefox 47



Developer Tools: Memory
3 years ago
2 years ago


(Reporter: jsantell, Assigned: fitzgen)


(Blocks: 3 bugs)

37 Branch
Firefox 47
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)



(1 attachment, 4 obsolete attachments)

I should be able to list all objects in the heap in tree-form 
showing from the GC root, a path of object references leading to the object in question.

Alternative takes on this from YourKit Java Profiler "Merged Paths" view, that’s similar to path from GC view, but does not show path of individual objects, but shows paths from multiple objects grouped by class.

Chrome's containment view:

Can also be inversed, as an "Outgoing References" view


3 years ago
Blocks: 957791


3 years ago
Depends on: 961323
Blocks: 1221506
Has STR: --- → irrelevant
Fundamental feature of the memory tool. Probably highest priority feature after dominator trees land.
Priority: -- → P1
Summary: Path from GC View → Shortest path(s) from GC roots view
Assignee: nobody → nfitzgerald
Depends on: 1247658
Blocks: 1241298
Depends on: 1248085
Created attachment 8720014 [details] [diff] [review]
WIP shortest-paths view

This is very much WIP. I think we need a splitter component so we can resize the
shortest paths side bar first...
Depends on: 1249147
Created attachment 8720534 [details] [diff] [review]
WIP shortest paths view

Stil WIP, but getting closer...
Created attachment 8720979 [details] [diff] [review]
Render retaining paths in the dominators view

This commit extends the devtools' memory panel's dominators view with a sidebar
that displays the 5 shortest retaining paths from the GC roots for the selected
node in the dominator tree.

Try push:
Attachment #8720979 - Flags: review?(jsantell)
Attachment #8720014 - Attachment is obsolete: true
Attachment #8720534 - Attachment is obsolete: true
Comment on attachment 8720979 [details] [diff] [review]
Render retaining paths in the dominators view

Review of attachment 8720979 [details] [diff] [review]:

Looks good -- only concern I have is whether or not we want to store the size of the h splitter in our main state, or just have that component handle it on its own. It certainly adds complexity here, and think it should be a view state, not a redux property, but defer to others on that.

::: devtools/client/memory/components/shortest-paths.js
@@ +11,5 @@
> +const { isSavedFrame } = require("devtools/shared/DevToolsUtils");
> +const { getSourceNames } = require("devtools/client/shared/source-utils");
> +const { L10N } = require("../utils");
> +
> +Object.defineProperty(this, "unknownSourceString", (function () {

This seems like overkill for lazilyloading this one string, which is already loaded via the frame component. If we need to lazy load some L10N stuff (for the shared components? but like I said, this is already included via frame component), we should add it to utils

@@ +17,5 @@
> +  return {
> +    get() {
> +      if (!cached) {
> +        const COMPONENTS_STRINGS_URI = "chrome://devtools/locale/";
> +        const componentsL10N = new ViewHelpers.L10N(COMPONENTS_STRINGS_URI);

If we need to use ViewHelpers, we should explicitly require it above; this only works because we're using browser loader

@@ +38,5 @@
> +  for (let i = 0, length = label.length; i < length; i++) {
> +    const piece = label[i];
> +
> +    if (isSavedFrame(piece)) {
> +      const { short } = getSourceNames(piece.source, unknownSourceString);

Or maybe we should just have the utility function getSourceName just provide it's own unknownSourceString. L10N makes modularization of this stuff hard.

@@ +44,5 @@
> +    } else if (piece === "noStack") {
> +      sanitized[i] = L10N.getStr("tree-item.nostack");
> +    } else if (piece === "noFilename") {
> +      sanitized[i] = L10N.getStr("tree-item.nofilename");
> +    } else if (piece === "JS::ubi::RootList") {

preferred `const ROOT_LIST = "JS::ubi::RootList"` with the rest of the consts at the top of the file

@@ +173,5 @@
> +    const target ="#graph-target");
> +
> +    let zoom = this.state.zoom;
> +    if (!zoom) {
> +      zoom = d3.behavior.zoom().on("zoom", function() {

I know I ran into issues with leaks when not unbinding this zoom in the web audio tool

edit: looks like you took care of all that here \m/

::: devtools/client/themes/memory.css
@@ +223,5 @@
>  /**
>   * Main panel
>   */
> +.vbox {

Wonder if we should start adding these in a shared css file for dexulification?
Attachment #8720979 - Flags: review?(jsantell) → review+
Regarding where to store the splitter size, I will quote myself from the HSplitBox bug:

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from bug 1249147 comment #9)
> If the width is not a prop, then it can't be maintained across component
> setup/teardown cycles. For example, if I drag to a certain width, switch
> views to something that doesn't use this component anymore so it is
> unmounted/destroyed, and then switch back to the original view again, I want
> my width to be preserved. If we don't have the width as a prop and keep it
> in state, it will not be preserved. This means that callers of the component
> *must* maintain that state somewhere, explicitly pass it in as a prop, and
> handle updating that state via onResize. Luckily, this is pretty
> straightforward, see the WIP patch in bug 1149385.
> I went through this same thing with the tree component wrt focus and
> expansion. It turns out that if you want state to persist, you have to
> persist that state. Obvious, when said that way. Slightly annoying with a
> little boiler plate, but very straightforward, non-magical, and explicit.
Created attachment 8721069 [details] [diff] [review]
Render retaining paths in the dominators view

Addresses review comments.
Attachment #8721069 - Flags: review+
Attachment #8720979 - Attachment is obsolete: true
Blocks: 1250312
Created attachment 8722204 [details] [diff] [review]
Render retaining paths in the dominators view

Rebased. New try push:
Attachment #8722204 - Flags: review+
Attachment #8721069 - Attachment is obsolete: true
Ok the blocker patch had some issues and has been updated. New try push with new blocker patch:
Note that this depends on the patch in bug 1249147, which is also marked checkin-needed.
Keywords: checkin-needed

Comment 11

2 years ago
Keywords: checkin-needed
Blocks: 1250966
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.