Closed Bug 1149385 Opened 10 years ago Closed 9 years ago

Shortest path(s) from GC roots view

Categories

(DevTools :: Memory, defect, P1)

37 Branch
x86
macOS
defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: jsantell, Assigned: fitzgen)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

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
Blocks: 957791
Depends on: 961323
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
Status: NEW → ASSIGNED
Depends on: 1247658
Depends on: 1248085
Attached patch WIP shortest-paths view (obsolete) — Splinter Review
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
Attached patch WIP shortest paths view (obsolete) — Splinter Review
Stil WIP, but getting closer...
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
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/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?
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.
Addresses review comments.
Attachment #8721069 - Flags: review+
Attachment #8720979 - Attachment is obsolete: true
Attachment #8721069 - Attachment is obsolete: true
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
Note that this depends on the patch in bug 1249147, which is also marked checkin-needed.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: