Closed
Bug 1238695
Opened 9 years ago
Closed 9 years ago
Render census data as a treemap
Categories
(DevTools :: Memory, defect, P1)
DevTools
Memory
Tracking
(firefox48 verified, relnote-firefox 48+)
RESOLVED
FIXED
Firefox 48
People
(Reporter: fitzgen, Assigned: gregtatum)
References
(Blocks 3 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 3 obsolete files)
596.60 KB,
image/gif
|
Details | |
223.71 KB,
patch
|
Details | Diff | Splinter Review |
We have d3 in the tree already and d3 does treemaps. Census reports are already trees. It should be fairly straight forward to render census data as a treemap with d3. Maybe good first bug for new person on our team?
Updated•9 years ago
|
Assignee: nobody → gtatum
Reporter | ||
Updated•9 years ago
|
Has STR: --- → irrelevant
Priority: P3 → P1
Assignee | ||
Comment 1•9 years ago
|
||
I'm implementing the view side over this repo:
https://github.com/TatumCreative/memory-treemap
Live view:
http://tatumcreative.github.io/memory-treemap/
Once this gets to where it needs to be I'll begin integrating it into the Firefox codebase. I'll probably build the dropdown first that provides the necessary report data in a separate bug and try and land that first. Then I'll pull the visualization over.
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Ok, this ended up touching a few more files than I expected. I went ahead and changed
the default view to the tree map as we had discussed. I had to update tests
to make them go the coarseType view.
I should have addressed all of Victor's reviews from: https://bugzilla.mozilla.org/show_bug.cgi?id=1252955
In addition to the tree map visualization, it includes all of the integration with
the memory devtools UI and tests. I added both Nick and Victor to for review, as
Victor already reviewed the visualization code, and I've interacted more with Nick
on the rest of the memory tools.
Attachment #8729597 -
Flags: review?(vporof)
Attachment #8729597 -
Flags: review?(nfitzgerald)
Reporter | ||
Comment 5•9 years ago
|
||
A tip for future patches: it would have been nice to separate the changing of default into another patch (or even another bug). That would make the patch much easier to review and also touch many fewer files.
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8729597 [details] [diff] [review]
Render census data as a treemap
Review of attachment 8729597 [details] [diff] [review]:
-----------------------------------------------------------------
Overall, this looks great, but there are a bunch of nits and few things that need changing before this lands. I'd like to look at it again after you've fixed up some things.
The tree map should not be available for diffing. Attempting to tree map a diff with many negative numbers results in an incomprehensible mess right now. We shouldn't land this patch until this is fixed.
I think that the tree map should probably be a different "view" option, rather than a census "group by" option. Both in the UI and in terms of the app state. The latter would make it easy to not allow diffings to be rendered as a tree map.
The following things can be follow ups, or part of this bug if you want. We just need to make sure they land before the next merge (4/18).
* Tooltips for boxes with the label, size, and count of the group. This would be especially helpful for when the label is cut off.
* Zooming is still a little squirrel-y, but not unusable. Specifically, if I put my mouse on the intersection of some boxes and zoom in, I don't quite zoom into that intersection of boxes, but past it.
* There is some margin around the whole tree map that should not be there.
Thanks, Greg!
::: devtools/client/locales/en-US/memory.properties
@@ +78,5 @@
> censusDisplays.invertedAllocationStack.tooltip=Group items by the inverted JavaScript call stack recorded when the object was created
>
> +# LOCALIZATION NOTE (breakdowns.treeMap.tooltip): The tooltip for the "tree map"
> +# breakdown option.
> +censusDisplays.treeMap.tooltip=Visualize the space taken up by the memory
I think "Visualize memory usage" conveys the same thing, but is a bit more concise and clear.
Maybe even add something about how the size of blocks is proportional to the memory usage?
"Visualize memory usage: larger blocks account for a larger percent of memory usage"
@@ +390,5 @@
> +tree-map.abbreviated-bytes.megabyte=MB
> +
> +# LOCALIZATION NOTE (tree-map.abbreviated-bytes.gigabyte): The abbreviation for
> +# gigabyte size in tree maps
> +tree-map.abbreviated-bytes.gigabyte=G
This should be "GB", no?
I'm not 100% sure these need to be localized -- do these units change in different locales?
::: devtools/client/memory/components/heap.js
@@ +187,5 @@
> ? snapshot.census
> : diffing.census;
> +
> + if (census.display.breakdown.treeMap) {
> + return this._renderTreeMap(state, census);
What happens when we render diffing censuses as a tree map and there are negative values? Do we abs(val)? I don't think that would make sense for the visualization. Should we disallow the tree map when diffing?
@@ +288,5 @@
>
> + _renderTreeMap(state, census) {
> + return this._renderHeapView(
> + state,
> + TreeMap({census})
Nit: "{ census }"
::: devtools/client/memory/components/tree-map.js
@@ +13,5 @@
> + census: censusModel
> + },
> +
> + componentDidMount() {
> + let { census } = this.props;
Nit: const for bindings that don't get re-assigned here and elsewhere
@@ +14,5 @@
> + },
> +
> + componentDidMount() {
> + let { census } = this.props;
> + if(census) {
Nit: "if" space "(" here and elsewhere
@@ +23,5 @@
> + componentDidUpdate(prevProps) {
> + let oldCensus = prevProps.census;
> + let newCensus = this.props.census;
> +
> + if (oldCensus !== newCensus) {
This check should be in the `componentShouldUpdated` method and then this method should only check whether it should start or stop a visualization.
@@ +37,5 @@
> + this.state.stopVisualization();
> + }
> + },
> +
> + stopVisualization() {
Nit: this isn't a public method of the component, so prefix with an underscore
Same for startVisualisation
::: devtools/client/memory/components/tree-map/canvas-utils.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
Note: I'm leaving review of everything within the tree-map/ subdir to Victor
::: devtools/client/memory/constants.js
@@ +140,5 @@
> + return L10N.getStr("censusDisplays.treeMap.tooltip");
> + },
> + breakdown: {
> + by: "coarseType",
> + treeMap: true,
This should be a property of the display model, not the breakdown:
{
displayName: "Tree Map",
get tooltip() { ... },
treeMap: true,
breakdown: { ... },
}
::: devtools/client/memory/utils.js
@@ +398,5 @@
> exports.formatNumber(percent, showSign));
> };
> +
> +/**
> + * return a debounced function
Use full sentences with punctuation for documentation and comments.
@@ +410,5 @@
> + * @param {Integer} timeout (optional)
> + * setTimeout can be injected for testing purposes
> + * @return {Function}
> + */
> +exports.debounce = function(fn, wait, now, timeout) {
We have implemented things like this a few times. Move this function to devtools/shared/ThreadSafeDevToolsUtils.js (so we have a canonical version), add a usage example (utilities shared across all devtools deserve even better documentation), and add a sanity test to devtools/shared/tests/unit/ for this function.
@@ +411,5 @@
> + * setTimeout can be injected for testing purposes
> + * @return {Function}
> + */
> +exports.debounce = function(fn, wait, now, timeout) {
> + // Just return the function if set to 0, useful for writing tests
Nit: period at end of sentence.
@@ +412,5 @@
> + * @return {Function}
> + */
> +exports.debounce = function(fn, wait, now, timeout) {
> + // Just return the function if set to 0, useful for writing tests
> + if(wait === 0) {
Nit: space between "if" and "("
@@ +416,5 @@
> + if(wait === 0) {
> + return fn;
> + }
> + // Only define the defaults after the wait == 0 as these may not be defined
> + // during a test
Nit: period at end of sentence.
@@ +435,5 @@
> + }
> + }
> + }, wait);
> + }
> +}
Nit: missing ; on pretty much everything in this file -- do you have eslint setup? https://wiki.mozilla.org/DevTools/CodingStandards#JS_linting_with_ESLint
@@ +443,5 @@
> + * ctx.fillStyle string.
> + *
> + * @param {Array} hsl
> + * values ranged between [0 - 1]
> + * @return {type}
@return {String}
@@ +445,5 @@
> + * @param {Array} hsl
> + * values ranged between [0 - 1]
> + * @return {type}
> + */
> +exports.hslToStyle = function([h, s, l]) {
Why take an array? Why not do:
function(h, s, l) { ... }
?
The array method requires an unnecessary allocation. Even if you happen to usually have arrays around, you can call it like this: `hslToStyle(...myArray)`.
Attachment #8729597 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #6)
> A tip for future patches: it would have been nice to separate the changing
> of default into another patch (or even another bug). That would make the
> patch much easier to review and also touch many fewer files.
Good point, I didn't realize this would be as drastic as it is.
> * Tooltips for boxes with the label, size, and count of the group. This
> would be especially helpful for when the label is cut off.
>
> * Zooming is still a little squirrel-y, but not unusable. Specifically, if I
> put my mouse on the intersection of some boxes and zoom in, I don't quite
> zoom into that intersection of boxes, but past it.
I'll do these in another patch.
> > +tree-map.abbreviated-bytes.megabyte=MB
> > +
> > +# LOCALIZATION NOTE (tree-map.abbreviated-bytes.gigabyte): The abbreviation for
> > +# gigabyte size in tree maps
> > +tree-map.abbreviated-bytes.gigabyte=G
>
> I'm not 100% sure these need to be localized -- do these units change in
> different locales?
In Spanish it's KiB, MiB, etc.
Comment 8•9 years ago
|
||
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #7)
> > > +tree-map.abbreviated-bytes.megabyte=MB
> > > +
> > > +# LOCALIZATION NOTE (tree-map.abbreviated-bytes.gigabyte): The abbreviation for
> > > +# gigabyte size in tree maps
> > > +tree-map.abbreviated-bytes.gigabyte=G
> >
> > I'm not 100% sure these need to be localized -- do these units change in
> > different locales?
>
> In Spanish it's KiB, MiB, etc.
In English it's KiB, MiB, GiB too. I mean, some people don't like the binary scale factors, but they are standard, and I don't think they're hard to understand.
https://en.wikipedia.org/wiki/Kibibyte
So I'd say, no localization; just follow the international standard.
Comment 9•9 years ago
|
||
Comment on attachment 8729597 [details] [diff] [review]
Render census data as a treemap
Review of attachment 8729597 [details] [diff] [review]:
-----------------------------------------------------------------
This is wonderful. Thanks for addressing all of my comments.
Attachment #8729597 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Ok, I've hopefully addressed your comments fitzgen. I apologize in advance for
the size of this patch. I ended up changing how the census state was saved per
out conversations on IRC. Instead of snapshot.state being able to have the value
state.CENSUS_SAVING and state.CENSUS_SAVED, it's now on snapshot.census.state,
with censusState.SAVING and censusState.SAVED. This pattern is followed for the
snapshot.treeMap.state as well. I had to hit a lot of tests to make this change
happen.
I will follow up in subsequent patches with your other comments on the margin
and scrolling, but I do want to get this behemoth in the tree to avoid code churn.
Attachment #8736754 -
Flags: review?(nfitzgerald)
Reporter | ||
Comment 12•9 years ago
|
||
Still reviewing, but I noticed that the scrolling-way-past-the-mouse-while-zooming behavior still exists. I think the last iteration was better than this one?
Doesn't need to block landing, but please please please fix this before uplift.
Reporter | ||
Comment 13•9 years ago
|
||
Bug found:
* Take snapshot
* Switch to aggregates view
* Filter for "Shape"
* Switch to tree map view
ER:
Shows whole tree map again OR still has filter box available
AR:
Only shows tree map of "Shape" related things, no filter box available to change filter or give clue that filtering is in effect
Note that switching back to aggregates, removing the filter string, and then going back to the tree map does NOT result in the full tree map being shown again.
Reporter | ||
Comment 14•9 years ago
|
||
Comment on attachment 8736754 [details] [diff] [review]
Render census data as a treemap -r=fitzgen,vporof
Review of attachment 8736754 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the filtering bug fixed and all these comments addressed.
Thanks, Greg!
::: devtools/client/memory/actions/diffing.js
@@ +77,5 @@
> + yield dispatch(snapshotActions.takeCensus(heapWorker, first.id));
> + }
> + if (!second.census) {
> + yield dispatch(snapshotActions.takeCensus(heapWorker, second.id));
> + }
Why do we need to have the individual, non-diffing censuses before we create the diffing census?
The answer definitely deserves to be a comment here, because I can't figure out why it is the case.
::: devtools/client/memory/actions/snapshot.js
@@ +188,5 @@
> + // flight. Recheck that the display used for the census is the same as the
> + // state's display.
> + do {
> + display = getDisplay(getState());
> + filter = getState().filter;
This should be factored out into a getFilter(getState()) function as well -- I believe this is the origin of the bug I just found while testing the patch.
::: devtools/client/memory/actions/tree-map-display.js
@@ +6,5 @@
> +const { assert } = require("devtools/shared/DevToolsUtils");
> +const { actions } = require("../constants");
> +const { refresh } = require("./refresh");
> +
> +exports.setTreeMapAndRefresh = function(heapWorker, display) {
Doc comment please.
::: devtools/client/memory/components/snapshot-list-item.js
@@ +67,5 @@
> }
>
> let details;
> + if (!selectedForDiffing) {
> + // See if a tree map or census is in the read state
Nit: add missing period
@@ +73,2 @@
>
> + // If there is census data, fill in the total bytes
Nit: add missing period.
::: devtools/client/memory/components/toolbar.js
@@ +107,5 @@
> })
> );
> + } else if (view == viewState.TREE_MAP) {
> + // Only show the dropdown if there are multiple display options
> + viewToolbarOptions = treeMapDisplays.length <= 1 ? null : dom.div(
treeMapDisplays.length should always be at least 1, right? That seems like an invariant that we should assert:
assert(treeMapDisplays.length >= 1,
"Should always have at least one tree map display");
And then we can change the check to `length === 1` or equivalently `length > 1` and swap the consequent and alternative if you find that reads more naturally.
@@ +190,5 @@
> },
> dom.option(
> {
> + value: viewState.TREE_MAP,
> + title: L10N.getStr("toolbar.view.treemap.tooltip"),
Thanks for adding a tooltip!
::: devtools/client/memory/constants.js
@@ +209,5 @@
> + get tooltip() {
> + const { L10N } = require("./utils");
> + return L10N.getStr("treeMapDisplays.coarseType.tooltip");
> + },
> + breakdown: COARSE_TYPE
Let's explicitly add
inverted: false
rather than let undefined get coerced to false in the census action tasks. It relays the intent to the reader, who would otherwise be left guessing.
@@ +234,5 @@
> * An FSM describing snapshot states:
> *
> + * SAVING -> SAVED -> READING -> READ
> + * ↗
> + * IMPORTING
Can you update the devtools/docs/memory-panel.md too: https://dxr.mozilla.org/mozilla-central/source/devtools/docs/memory-panel.md#207
@@ +248,5 @@
> snapshotState.READ = "snapshot-state-read";
> +
> +/*
> + * Various states the census model can be in.
> + *
Thanks for splitting this out from the snapshot state. It probably should have happened sooner, but better late than never.
@@ +249,5 @@
> +
> +/*
> + * Various states the census model can be in.
> + *
> + * SAVING -> SAVED
This edge should be bidirectional, as we can take new censuses with different breakdown which moves us from SAVED back to SAVING.
@@ +264,5 @@
> +
> +/*
> + * Various states the tree map model can be in.
> + *
> + * SAVING -> SAVED
Ditto.
::: devtools/client/memory/models.js
@@ +88,5 @@
> +
> +/**
> + * Tree map model.
> + */
> +let treeMapModel = exports.treeMapModel = PropTypes.shape({
s/let/const/
::: devtools/client/memory/reducers/snapshots.js
@@ +123,5 @@
> +};
> +
> +handlers[actions.TAKE_TREE_MAP_END] = function (snapshots, { id,
> + report,
> + display }) {
Nit: this alignment is a bit funky. If the destructuring won't fit in the params without making the line too long, we can do:
handlers[actions.TAKE_TREE_MAP_END] = function (snapshots, action) {
const { id, report, display } = action;
...
};
::: devtools/client/memory/test/browser/browser_memory_percents_01.js
@@ +28,5 @@
> const doc = panel.panelWin.document;
>
> + dispatch(changeView(viewState.CENSUS));
> +
> +
Nit: no double empty line, please.
::: devtools/client/memory/test/browser/head.js
@@ +177,5 @@
> + * Wait until the state has censuses in a certain state.
> + *
> + * @return {Promise}
> + */
> +function waitUntilCensusState (store, getCensus, expected) {
Make sure to do DEBUG builds/tests in your try run, which is where leak checks run.
We used to have a very similar helper utility that would cause the post processing of the ++DOMWINDOW and --DOMWINDOW log messages to report leaking windows. I could never pinpoint the root cause, unfortunately, because our tools don't consider cycle-collected things yet (those logs correspond to nsGlobalWindow objects). This is why this utility is pretty much inlined all over the place as:
yield waitUntilState(store, state =>
snapshots.length === 2 &&
snapshots[0].state === FOO &&
snapshots[1].state === FOO);
The leaks went away when we inlined the equivalent thing :-/
So, you may have to expand this out in the browser mochitests if you find that DEBUG builds are leaking :(
@@ +196,5 @@
> + info(`Waiting for snapshots' censuses to be of state: ${expected}`);
> + return waitUntilState(store, predicate);
> +}
> +/**
> + * mock out the requestAnimationFrame
Nit: capital and period
::: devtools/client/memory/test/unit/test_utils.js
@@ +54,5 @@
> +
> + equal(utils.formatAbbreviatedBytes(12), "12B", "Formats bytes");
> + equal(utils.formatAbbreviatedBytes(12345), "12KiB", "Formats kilobytes");
> + equal(utils.formatAbbreviatedBytes(12345678), "11MiB", "Formats megabytes");
> + equal(utils.formatAbbreviatedBytes(12345678912), "11GiB", "Formats gigabytes");
Can you file a follow up to use this helper utility for the total size of the snapshot in the sidebar? Thanks!
::: devtools/client/memory/utils.js
@@ +305,4 @@
> && display === census.display;
> };
>
> +exports.canTakeCensus = function (snapshot, assertCan = false) {
Doc comment please.
@@ +312,5 @@
> +
> + if (assertCan) {
> + assert(ready,
> + `Attempting to take a census when the snapshot is not in a ready state.`);
> + }
This doesn't return `ready`? This just sometimes asserts and other times is a no-op? Very funky.
Let's make this function return `ready`, not do any asserting, and then callers can assert(canTakeCensus(snapshot)) if that is how they would like to use the return value.
@@ +343,5 @@
> + }
> + if (snapshot.census && snapshot.census.state === censusState.SAVED) {
> + return snapshot.census;
> + }
> +}
Nit: ";" missing at end.
Also, actually return `null` if neither exist. Otherwise the documentation is misleading, since `undefined` will be implicitly returned and leaves the reader wondering whether there should be an assert that we took one of the two branches instead.
Attachment #8736754 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 15•9 years ago
|
||
> Why do we need to have the individual, non-diffing censuses before we create
> the diffing census?
>
> The answer definitely deserves to be a comment here, because I can't figure
> out why it is the case.
Logical error on my part, they don't need to be there. I removed them.
> Make sure to do DEBUG builds/tests in your try run, which is where leak
> checks run.
>
> We used to have a very similar helper utility that would cause the post
> processing of the ++DOMWINDOW and --DOMWINDOW log messages to report leaking
> windows. I could never pinpoint the root cause, unfortunately, because our
> tools don't consider cycle-collected things yet (those logs correspond to
> nsGlobalWindow objects). This is why this utility is pretty much inlined all
> over the place as:
>
> yield waitUntilState(store, state =>
> snapshots.length === 2 &&
> snapshots[0].state === FOO &&
> snapshots[1].state === FOO);
>
> The leaks went away when we inlined the equivalent thing :-/
>
> So, you may have to expand this out in the browser mochitests if you find
> that DEBUG builds are leaking :(
I'll probably hit you up on IRC about this one to understand it.
> @@ +312,5 @@
> > +
> > + if (assertCan) {
> > + assert(ready,
> > + `Attempting to take a census when the snapshot is not in a ready state.`);
> > + }
>
> This doesn't return `ready`? This just sometimes asserts and other times is
> a no-op? Very funky.
>
> Let's make this function return `ready`, not do any asserting, and then
> callers can assert(canTakeCensus(snapshot)) if that is how they would like
> to use the return value.
Yeah that's bizarre. No idea why I wrote that code like that.(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #12)
> Still reviewing, but I noticed that the
> scrolling-way-past-the-mouse-while-zooming behavior still exists. I think
> the last iteration was better than this one?
>
> Doesn't need to block landing, but please please please fix this before
> uplift.
It should be the same as the last patch, but I'll definitely get this handled. I created a P1 bug for this and assigned it to myself. https://bugzilla.mozilla.org/show_bug.cgi?id=1261159
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Comment 17•9 years ago
|
||
The review comments have been addressed with this patch.
Assignee | ||
Updated•9 years ago
|
Attachment #8729597 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8736754 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
I'm getting an assertion error on Windows 7 debug builds only and am having trouble figuring out the proper approach for dealing with it.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6250fcac3ba5&filter-searchStr=Windows%207%20debug%20Mochitest%20Mochitest%20Other%20M(oth)
> DevToolsUtils.assert threw an exception: Error: Assertion failure: Should have a parent map
> Stack: reallyAssert@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:297:17
> exports.censusModel<.state<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/memory/models.js:151:9
> catchAndIgnore/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/memory/models.js:40:10
> validate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:13327:19
> checkType@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:13164:14
> validate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:13327:19
> checkType@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:13164:14
> checkPropTypes@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:9819:17
> validatePropTypes@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:9849:5
> [58]</ReactElementValidator.createElement@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/vendor/react-dev.js:9883:5
> window.onload<@chrome://mochitests/content/chrome/devtools/client/memory/test/chrome/test_Heap_01.html:35:36
> TaskImpl_run@resource://gre/modules/Task.jsm:319:40
> TaskImpl@resource://gre/modules/Task.jsm:280:3
> createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14
> EventHandlerNonNull*@chrome://mochitests/content/chrome/devtools/client/memory/test/chrome/test_Heap_01.html:17:26
> Line: 297, column: 17
What's weird is that looking into it I'm not sure how any of the other debug builds wouldn't trip up on this as well. With the tree map feature I added some additional model validation in the models.js file for the censusModel that checks to make sure the report properties are correctly added. In the tests/chrome/head.js file there is a TEST_HEAP_PROPS object, and the census on these props doesn't have the parentMap property. So according to my assertion it should always fail, but doesn't. I'm not sure why this would be different on different systems.
The resolution seems to be either to add the parentMap to the test fixture, or remove the check from the model validation. I can't tell if parentMap will always be there. In the heap worker it states:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/HeapAnalysesClient.js#160-162
> - parentMap:
> The result of calling CensusUtils.createParentMap on the generated
> delta. Only exists if asTreeNode or asInvertedTreeNode are set.
But then in takeCensus...
https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/actions/snapshot.js#177-179
> let opts = display.inverted
> ? { asInvertedTreeNode: true }
> : { asTreeNode: true };
Which makes it seems like it will always be defined in this case.
Even after making the change to fix this I'm not sure how it could even pass in the first place. I don't really understand when and how the model validation is happening.
Flags: needinfo?(nfitzgerald)
Reporter | ||
Comment 19•9 years ago
|
||
We should add the parent map to the test fixture. It should always be there. The ability to return a raw census report from the worker rather than convert it to a CensusTreeNode tree is purely historical, and is only used in tests. We should probably remove that ability too, but it isn't really much of a priority.
Flags: needinfo?(nfitzgerald)
Reporter | ||
Comment 20•9 years ago
|
||
As for why it was only failing on windows: no idea. But we should fix it up either way.
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Comment 22•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8736918 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
Keywords: checkin-needed
Comment 24•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 25•9 years ago
|
||
Comment on attachment 8737289 [details] [diff] [review]
Render census data as a treemap -r=fitzgen,vporof
Review of attachment 8737289 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/client/locales/en-US/memory.properties
@@ +78,5 @@
> censusDisplays.invertedAllocationStack.tooltip=Group items by the inverted JavaScript call stack recorded when the object was created
>
> +# LOCALIZATION NOTE (breakdowns.treeMap.tooltip): The tooltip for the "tree map"
> +# breakdown option.
> +censusDisplays.treeMap.tooltip=Visualize memory usage: larger blocks account for a larger percent of memory usage
Is the string ID wrong, or is it the comment? I assume the latter.
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #25)
> ::: devtools/client/locales/en-US/memory.properties
> @@ +78,5 @@
> > censusDisplays.invertedAllocationStack.tooltip=Group items by the inverted JavaScript call stack recorded when the object was created
> >
> > +# LOCALIZATION NOTE (breakdowns.treeMap.tooltip): The tooltip for the "tree map"
> > +# breakdown option.
> > +censusDisplays.treeMap.tooltip=Visualize memory usage: larger blocks account for a larger percent of memory usage
>
> Is the string ID wrong, or is it the comment? I assume the latter.
The comment is wrong, it should be `censusDisplays.treeMap.tooltip`. I can submit a patch to fix it unless you are already in there working on it.
Assignee | ||
Comment 27•9 years ago
|
||
Actually I just realized I'm hitting devtools/client/locales/en-US/memory.properties in Bug 1248988. I'll fix it there. Thanks for catching this!
Reporter | ||
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 29•9 years ago
|
||
I have reproduced this bug on Nightly 46.0a1 (2016-01-11) on ubuntu 14.04 LTS, 32 bit!
The bug's fix is now verified on Latest Nightly 48.0a1!
Build ID: 20160412050029
User Agent: Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0
QA Whiteboard: [bugday-20160413]
Comment 30•9 years ago
|
||
I've added a page on this:
https://developer.mozilla.org/en-US/docs/Tools/Memory/Tree_map_view
Please let me know if I need anything else here.
Flags: needinfo?(gtatum)
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 32•9 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]: new devtools feature
[Suggested wording]: New tree map view in memory tool
[Links (documentation, blog post, etc)]: see comment 30
relnote-firefox:
--- → ?
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•