We should be able to have a detailed diff view between two heap snapshots. This would be a superset of the data presented in bug 960770 with extra info on actual object values and references to other objects.

Comment on attachment 8686648 [details] [diff] [review] Add support for diffing heap snapshots to the memory tool frontend Review of attachment 8686648 [details] [diff] [review]: ----------------------------------------------------------------- You'll likely need to rebase your css changes on top of bug 1173397. ::: devtools/client/themes/memory.css @@ +56,5 @@ > +.devtools-toolbar > .toolbar-group:nth-of-type(1) { > + /** > + * We want this to be exactly at a `--sidebar-width` distance from the > + * toolbar's start boundary. A `.devtools-toolbar` has a 3px start padding. > + */ Thanks ! I'm glad we're getting rid of the margin hack @@ +106,5 @@ > min-width: 78px; > } > > +#diff-snapshots { > + font-family: monospace; You should be able to use the .devtools-monospace class instead. @@ +111,5 @@ > +} > + > +#diff-snapshots[disabled] { > + opacity: .5; > +} This shouldn't be needed, especially after bug 1173397. @@ +130,5 @@ > * rather than the definitions in toolbars.css. > * > * @see bug 1173397 for another inverted related bug > */ > +#take-snapshot::before { It's incorrect to invert the icon with the dark theme. Anyway, I'll get rid of this block in bug 1173397. @@ +396,5 @@ > color: var(--theme-content-color3); > } > > .heap-tree-percent { > + flex: 0 0 calc(3.5em); Why is calc needed here ?

Ok this new version is rebased on fx-team on top of all the import/export stuff. :ntim, thanks for the review! I actually meant to flag you but forgot. I will update the patch in one fell swoop once Jordan reviews. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40c2d70b4471

(In reply to Tim Nguyen [:ntim] from comment #8) > @@ +396,5 @@ > > color: var(--theme-content-color3); > > } > > > > .heap-tree-percent { > > + flex: 0 0 calc(3.5em); > > Why is calc needed here ? Just dumbness / copy-paste on my part.

Some thoughts trying this out: * Toolbar needs some style love after rebasing * Is "+/-" more clear than "Diff..." button or something to initiate? * When in a diffing state, a third, non-compared snapshot has the checkbox, and clicking just goes back to the normal state -- we should just hide other checkboxes. * Probably follow up bug worthy, but with the added "+" twice in each column, I'm running into the values being clipped often * When I'm in diff-select mode, and one snapshot is done and the other is still being saved, the loading state is on that snapshot until it's atleast read into memory, and then I can just select it -- VERY NICE! Reminds me we should have something like "isStateAtleastRead" or use bitflags for states so we can just see if a state is ready via `snapshot.state & READY` rather than all these arrays. In general, I thought diffing would generate a new snapshot, rather than introduce a whole mode of diffing -- is this possible? This would give us the ability to save a diff, and flip between a diff and other snapshot without having to reselect. Just wanted to confirm all this behavior before going through with the rest of the review as this seems pretty fundamental to the whole diffing process.

Comment #11 is based off of attachment 8686361 [details] [diff] [review], didn't see latest, FYI

It's strange rounding down our percentages to 0% when we have the integer next to it as "+1000" or some + value. We have this issue without this patch, but maybe the "+" makes it more strange.

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #11) > Created attachment 8686671 [details] > Screen Shot 2015-11-12 at 8.41.09 AM.png > > Some thoughts trying this out: > > * Toolbar needs some style love after rebasing I had this working on one of my rebases, but it must have broken again :-/ > * Is "+/-" more clear than "Diff..." button or something to initiate? It is certainly less verbose for our dwindling toolbar-sidebar real estate :P I trust Helen/ntim opinions on this (as a follow up) more than mine or yours (no offense intended). > * When in a diffing state, a third, non-compared snapshot has the checkbox, > and clicking just goes back to the normal state -- we should just hide other > checkboxes. Yeah we could do that if diffing.state !== SELECTING > * Probably follow up bug worthy, but with the added "+" twice in each > column, I'm running into the values being clipped often I had to expand the percent's width to make room for "+100%", not super surprised that bytes are getting cut off now :-/ > * When I'm in diff-select mode, and one snapshot is done and the other is > still being saved, the loading state is on that snapshot until it's atleast > read into memory, and then I can just select it -- VERY NICE! Thanks! > Reminds me we > should have something like "isStateAtleastRead" or use bitflags for states > so we can just see if a state is ready via `snapshot.state & READY` rather > than all these arrays. -1 for bitflags, I think a helper function is better (usually you would use a helper function to hide the bitflags anyways... I guess we could make our states bitflags *and* write helpers, but this isn't something where we really need to worry about memory as there is only a handful of states in memory at a given time) > In general, I thought diffing would generate a new snapshot, rather than > introduce a whole mode of diffing -- is this possible? This would give us > the ability to save a diff, and flip between a diff and other snapshot > without having to reselect. Just wanted to confirm all this behavior before > going through with the rest of the review as this seems pretty fundamental > to the whole diffing process. We could maybe simulate this in the UI, but saving a diff would be a lot harder and involve new traversals/analyses of ubi::Node graphs. Not impossible, but definitely not a good use of time IMO.

Comment on attachment 8686648 [details] [diff] [review] Add support for diffing heap snapshots to the memory tool frontend Review of attachment 8686648 [details] [diff] [review]: ----------------------------------------------------------------- Looks good -- handful of comments, and follow up bugs or in here from comment #11 -- r+ ::: devtools/client/locales/en-US/memory.properties @@ +69,5 @@ > import-snapshot=Import… > > +# LOCALIZATION NOTE (diff-snapshots): The label for the button that initiates > +# selecting two snapshots to diff with each other. > +diff-snapshots=+/- Should have an l10n for the icon/button (+/- in this case), as well as a tooltip like "Diff Snapshots" ::: devtools/client/memory/actions/snapshot.js @@ +196,5 @@ > }; > }; > > /** > + * @param {snapshotId} @param {snapshotId} id ::: devtools/client/memory/app.js @@ +85,5 @@ > List({ > itemComponent: SnapshotListItem, > items: snapshots, > + onSave: snapshot => dispatch(pickFileAndExportSnapshot(snapshot)), > + onClick: diffing && diffing.state === diffingState.SELECTING We should assemble this not inline, so just have a `let onClick = diffing && diffing.state ...` before the giant return statement ::: devtools/client/memory/components/heap.js @@ +116,5 @@ > + statusText = snapshot ? getSnapshotStatusTextFull(snapshot.state) : ""; > + } > + assert(census !== undefined, "census should have been set"); > + assert(state !== undefined, "state should have been set"); > + assert(statusText !== undefined, "statusText should have been set"); nice ::: devtools/client/memory/components/toolbar.js @@ +56,5 @@ > + dom.button({ > + id: "diff-snapshots", > + className: "devtools-button" + (!!diffing ? " checked" : ""), > + disabled: snapshots.length < 2, > + onClick: onToggleDiffing, should have a tooltip (title) here ::: devtools/client/memory/reducers/diffing.js @@ +23,5 @@ > +}; > + > +handlers[actions.SELECT_SNAPSHOT_FOR_DIFFING] = function (diffing, { snapshot }) { > + assert(diffing, > + "Should never select a snapshot for diffing when we aren't diffing " + just single line this @@ +37,5 @@ > + }); > + } > + > + assert(!diffing.secondSnapshotId, > + "If we aren't selecting the first, then we must be selecting the " + put the assertion message on the same line atleast, we don't have any rules of 80 char line limits here @@ +54,5 @@ > +handlers[actions.TAKE_CENSUS_DIFF_START] = function (diffing, action) { > + assert(diffing, "Should be diffing when starting a census diff"); > + assert(action.first.id === diffing.firstSnapshotId, > + "First snapshot's id should match"); > + assert(action.second.id === diffing.secondSnapshotId, good assertions ::: devtools/client/memory/reducers/snapshots.js @@ +76,5 @@ > }); > }; > > +handlers[actions.SELECT_SNAPSHOT] = function (snapshots, { id }) { > + return snapshots.map(s => immutableUpdate(s, { selected: s.id === id })); nice ::: devtools/client/memory/test/unit/head.js @@ +23,5 @@ > > DevToolsUtils.testing = true; > > +function dumpn(msg) { > + dump("MEMORY-TEST: " + msg + "\n"); `MEMORY-TEST: ${msg}\n` ::: devtools/client/memory/test/unit/test_action_diffing_03.js @@ +41,5 @@ > + equal(getState().diffing.state, diffingState.SELECTING, > + "should be in diffing state SELECTING"); > + > + // Can't select a snapshot that is not in a diffable state. > + equal(getState().snapshots[3].state, snapshotState.SAVING, This should be ok from race conditions, yeah? This is on same tick that executes the snapshot, so it should always be called before any progress on the SAVING->SAVED front. Just thinking outloud. @@ +46,5 @@ > + "the last snapshot is still in the process of being saved"); > + dumpn("Expecting exception:"); > + let threw = false; > + try { > + dispatch(selectSnapshotForDiffing(getState().snapshots[3])); Instead of using a boolean, can just have a falsy assertion here `ok(false, "Should throw if snapshots not ready for diffing")` and the truthy version in the catch. ::: devtools/client/memory/utils.js @@ +15,5 @@ > const CUSTOM_BREAKDOWN_PREF = "devtools.memory.custom-breakdowns"; > const DevToolsUtils = require("devtools/shared/DevToolsUtils"); > +const { snapshotState: states, diffingState, breakdowns } = require("./constants"); > + > +exports.immutableUpdate = function (...objs) { Wonder if this should handle arrays as well. @@ +126,3 @@ > * @return {String} > */ > +exports.getSnapshotStatusText = function (state) { If this takes both snapshot and diffing state, this should be renamed to "getStatusText", as well as the full version below -- or diffing state should have its own function for getting status text @@ +130,3 @@ > > + switch (state) { > + case diffingState.ERROR: Diffing state should probably have a different error message -- atleast for the full version, as it refers to a snapshot, not a diffed view @@ +236,2 @@ > */ > +exports.getSnapshot = function getSnapshot (state, id) { I don't really like that this now takes the entire state, seems opposite of the other API changes (just take an ID rather than a snapshot object, whereas this takes the entire state, rather than just the snapshot array). Also makes it unusable in reducers, as well as in action creators that do not use middleware (and thus have access to getState) ::: devtools/shared/heapsnapshot/tests/unit/test_census-tree-node-07.js @@ -197,5 @@ > - bytes: 0, > - totalBytes: 220, > - count: 0, > - totalCount: 22, > - children: undefined, why are all of these going away?

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #15) > ::: devtools/client/memory/reducers/diffing.js > @@ +23,5 @@ > > +}; > > + > > +handlers[actions.SELECT_SNAPSHOT_FOR_DIFFING] = function (diffing, { snapshot }) { > > + assert(diffing, > > + "Should never select a snapshot for diffing when we aren't diffing " + > > just single line this > > @@ +37,5 @@ > > + }); > > + } > > + > > + assert(!diffing.secondSnapshotId, > > + "If we aren't selecting the first, then we must be selecting the " + > > put the assertion message on the same line atleast, we don't have any rules > of 80 char line limits here Our style guide / eslint actually *does* enforce 80 char limit. I know most of the frontend doesn't follow that, but it makes big ugly red underlines in my editor, so I'd prefer new code attempts to follow that (won't r- a patch for going over, but I'm also not going to turn lines that fit into lines that don't). > ::: devtools/client/memory/test/unit/test_action_diffing_03.js > @@ +41,5 @@ > > + equal(getState().diffing.state, diffingState.SELECTING, > > + "should be in diffing state SELECTING"); > > + > > + // Can't select a snapshot that is not in a diffable state. > > + equal(getState().snapshots[3].state, snapshotState.SAVING, > > This should be ok from race conditions, yeah? This is on same tick that > executes the snapshot, so it should always be called before any progress on > the SAVING->SAVED front. Just thinking outloud. Yes in this case, although the try push did turn up a different location where I was racing similarly. > ::: devtools/client/memory/utils.js > @@ +15,5 @@ > > const CUSTOM_BREAKDOWN_PREF = "devtools.memory.custom-breakdowns"; > > const DevToolsUtils = require("devtools/shared/DevToolsUtils"); > > +const { snapshotState: states, diffingState, breakdowns } = require("./constants"); > > + > > +exports.immutableUpdate = function (...objs) { > > Wonder if this should handle arrays as well. It should already in that 0,1,2,... are properties and will get updated. For concatenating, splicing, prepending, etc we would probably want new methods because having one super generic methods gets very confusing over time IME. (And the "regular" version of some of these is already the "immutable" version). > @@ +236,2 @@ > > */ > > +exports.getSnapshot = function getSnapshot (state, id) { > > I don't really like that this now takes the entire state, seems opposite of > the other API changes (just take an ID rather than a snapshot object, > whereas this takes the entire state, rather than just the snapshot array). > Also makes it unusable in reducers, as well as in action creators that do > not use middleware (and thus have access to getState) I've really come to dislike `combineReducers` because as soon as you move local state out to be shared state or go from one-to-one to one-to-many or many-to-many, you have to throw everything away or do work arounds of some other sort. That's what we're hitting here. Snapshots are shared global state, and it doesn't fit into the "only look at this one tree, not the forest" paradigm of `combineSnapshots`. > ::: devtools/shared/heapsnapshot/tests/unit/test_census-tree-node-07.js > @@ -197,5 @@ > > - bytes: 0, > > - totalBytes: 220, > > - count: 0, > > - totalCount: 22, > > - children: undefined, > > why are all of these going away? Empty leaves are pruned from the tree now \o/

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

New try push because task cluster was on the fritz: https://treeherder.mozilla.org/#/jobs?repo=try&revision=faff97a08ad9

https://hg.mozilla.org/mozilla-central/rev/8f4312027a49 https://hg.mozilla.org/mozilla-central/rev/4fe3fa71d960

There's a problem in this landing http://hg.mozilla.org/mozilla-central/diff/8f4312027a49/devtools/client/locales/en-US/memory.properties -snapshot.state.importing.full=Importing %S… +snapshot.state.importing.full=Importing… Was this change wanted? In that case you need to update the string ID. More details https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings More in general: why do you need two identical strings like this one? diffing.state.taking-diff=Computing difference… diffing.state.taking-diff.full=Computing difference…

(In reply to Francesco Lodolo [:flod] from comment #23) > There's a problem in this landing > http://hg.mozilla.org/mozilla-central/diff/8f4312027a49/devtools/client/ > locales/en-US/memory.properties > > -snapshot.state.importing.full=Importing %S… > +snapshot.state.importing.full=Importing… > > Was this change wanted? In that case you need to update the string ID. More > details > https://developer.mozilla.org/en-US/docs/Mozilla/Localization/ > Localization_content_best_practices#Changing_existing_strings My bad, I forgot to update the string ID, will do that in a follow up. > More in general: why do you need two identical strings like this one? > diffing.state.taking-diff=Computing difference… > diffing.state.taking-diff.full=Computing difference… The UI will use <state> and <state>.full in various places for every <state>. It just so happens that for many <state>s there isn't a substantial difference. It is easier to duplicate the strings than to hard code the subset that do have longer descriptions.

Added to the release notes using "Support for diffing heap snapshots to the memory tool frontend" as wording. Waiting for the doc now :)

(In reply to Sylvestre Ledru [:sylvestre] from comment #25) > Added to the release notes using "Support for diffing heap snapshots to the > memory tool frontend" as wording. > Waiting for the doc now :) Sounds good, but I would drop "frontend" as users only ever see the frontend :)

oups, thanks :)

I've added a note on this, and a little video: https://developer.mozilla.org/en-US/docs/Tools/Memory#Diffing_heap_snapshots.

Thanks, I updated the release notes with the link.

(In reply to Will Bamberg [:wbamberg] from comment #28) > I've added a note on this, and a little video: > https://developer.mozilla.org/en-US/docs/Tools/Memory#Diffing_heap_snapshots. Great video! Love how it shows off the allocation stack recording with the diffing :)