Closed
Bug 1149385
Opened 10 years ago
Closed 9 years ago
Shortest path(s) from GC roots view
Categories
(DevTools :: Memory, defect, P1)
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)
31.64 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•9 years ago
|
Blocks: memory-frontend
Assignee | ||
Updated•9 years ago
|
Has STR: --- → irrelevant
Assignee | ||
Comment 1•9 years ago
|
||
Fundamental feature of the memory tool. Probably highest priority feature after dominator trees land.
Priority: -- → P1
Assignee | ||
Updated•9 years ago
|
Summary: Path from GC View → Shortest path(s) from GC roots view
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
This is very much WIP. I think we need a splitter component so we can resize the
shortest paths side bar first...
Assignee | ||
Comment 3•9 years ago
|
||
Stil WIP, but getting closer...
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8720014 -
Attachment is obsolete: true
Attachment #8720534 -
Attachment is obsolete: true
Reporter | ||
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8720979 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Rebased. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7ce84fc98d4
Attachment #8722204 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8721069 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Comment 10•9 years ago
|
||
Note that this depends on the patch in bug 1249147, which is also marked checkin-needed.
Keywords: checkin-needed
Comment 11•9 years ago
|
||
Keywords: checkin-needed
Comment 12•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•