1149385 - Shortest path(s) from GC roots view

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

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

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. https://www.yourkit.com/docs/java/help/merged_paths.png

Chrome's containment view: https://developer.chrome.com/devtools/docs/javascript-memory-profiling#containment-view

Can also be inversed, as an "Outgoing References" view http://techpubs.borland.com/optimizeit/optimizeit5/profiler/images/outgoing_win.gif

Comment 1

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

Fundamental feature of the memory tool. Probably highest priority feature after dominator trees land.

Comment 2

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

Attachment: 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...

Comment 3

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

Attachment: WIP shortest paths view

Stil WIP, but getting closer...

Comment 4

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

Attachment: 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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aee612965a2b

Comment 5

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

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/components.properties";
> +        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 = d3.select("#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
https://github.com/mozilla/gecko-dev/blob/78520c7e6684244e3c8a916b2181c1c2cf0a2e0a/devtools/client/webaudioeditor/views/context.js#L61-L65

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?

Comment 6

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

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.

Comment 7

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

Attachment: Render retaining paths in the dominators view

Addresses review comments.

Comment 8

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

Attachment: Render retaining paths in the dominators view

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

Comment 9

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

Ok the blocker patch had some issues and has been updated. New try push with new blocker patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cb965ccbc1

Comment 10

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

Note that this depends on the patch in bug 1249147, which is also marked checkin-needed.

Comment 11

[Pulsebot]

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

Comment 12

[Lina Butler [:lina]]

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