Closed
Bug 857382
Opened 11 years ago
Closed 11 years ago
Add ability to diff two files to about:memory
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(4 files, 1 obsolete file)
18.26 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
43.49 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
15.15 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
Currently when viewing a memory report diff in about:memory you have to add "?diff" to put it in "diff mode", which stops it complaining about negative values and percentages greater than 100. This is undocumented. A better approach would be to encode in the JSON whether the reports are from a diff. That way the user wouldn't need to worry about it. A top-level boolean "isDiff" field would suffice. The version number of the format would also need bumping.
Assignee | ||
Comment 1•11 years ago
|
||
The diff tool is in toolkit/components/aboutmemory/tools/diff-memory-reports.js, BTW.
Assignee | ||
Comment 2•11 years ago
|
||
Here's a clearer list of what needs changing. - The comment in xpcom/base/nsIMemoryInfoDumper.idl needs to be change to document the new "isDiff" field in the JSON. - xpcom/base/nsMemoryInfoDumper.cpp needs to be changed to actually write the new field. The format version should be bumped from 1 to 2, as well. - toolkit/components/aboutmemory/content/aboutMemory.js: - gCurrentFileFormatVersion needs to be bumped from 1 to 2. - The "isDiff" field needs to be read and checked in updateAboutMemoryFromJSONString(), and gIsDiff needs to be set there instead of when parsing document.title. - In toolkit/components/aboutmemory/tools/diff-memory-reports.js, the comment at the top needs to be changed so it no longer mentions the "?diff" mode. - We should have a test for this. Probably easiest to modify toolkit/components/aboutmemory/tests/{test_aboutmemory3.xul,memory-reports-good.json} to include a true "isDiff" field and add some negative numbers and percentages greater than 100.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #735606 -
Flags: review?(bugmail.mozilla)
Updated•11 years ago
|
Attachment #735606 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 4•11 years ago
|
||
Now I'm not sure if this is the best way to do this. It deepens the notion that memory report diffing is a separate command line tool. An alternative is to allow you to diff two files directly in about:memory. It would be easy to import the diff-memory-reports.js code into aboutMemory.js. I'm not quite sure what the UI for selecting the two files would be, but that's a solvable problem. Pros: - Normal users can easily do diffs. Cons: - Can't script/automate diffing. (Not sure how useful that is in practice.) - Can't create a single diff file per se. If we were to do this, I bet people will immediately ask to skip the save-as-file steps, and just generate a diff by clicking on "start" and "stop" buttons. Thoughts?
Comment 5•11 years ago
|
||
> - Can't create a single diff file per se.
We could get around this con by extending the JSON format to contain two about:memory dumps, which we'd then diff.
Comment 6•11 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4) > An alternative is to allow you to diff two files directly in about:memory. > It would be easy to import the diff-memory-reports.js code into > aboutMemory.js. I'm not quite sure what the UI for selecting the two files > would be, but that's a solvable problem. This does sound like an improvement. > Pros: > - Normal users can easily do diffs. > > Cons: > - Can't script/automate diffing. (Not sure how useful that is in practice.) I don't think this happens often enough to need to be easily automatable. > - Can't create a single diff file per se. You could make the diff exportable (and re-importable) from about:memory. Even if you don't make it exportable using a save-as-json.gz option, you can still copy out the diff in the old ASCII-art format which is probably good enough for posting in bugs and such. > > If we were to do this, I bet people will immediately ask to skip the > save-as-file steps, and just generate a diff by clicking on "start" and > "stop" buttons. This sounds useful.
Assignee | ||
Updated•11 years ago
|
Depends on: 856917
Summary: Handle memory reports diffs better in about:memory → Add ability to diff two files to about:memory
Assignee | ||
Updated•11 years ago
|
Attachment #735606 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
about:memory has a defect: if a process has "other" reports but no "explicit" reports, the "other" reports don't show up. This never happens normally, but it did show up in my artificial testing of diff mode. This patch fixes it, and augments the tests to test this case, and also the case where a process has "explicit" reports but no "other" reports. kats, it might make sense to look at the test changes first, and the aboutMemory.js changes second.
Attachment #739335 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 8•11 years ago
|
||
This moves the diffing functionality from diff-memory-reports.js into aboutMemory.js. One good thing about this is that the diffing functionality is now tested in a mochitest. I'm a bit uncertain about the UI. I added a "Load and diff..." button; when you click on it it pops up two file pickers in a row. That works reasonably well, but I'm concerned about how it fits with future changes. The "Show Memory Reports" box now has four buttons: - Measure - Load... - Load and diff... - Read from clipboard The last button will change at some point to "Load from URL" (bug 859603). It's unclear if that URL will be read directly from the clipboard, or if it has to be pasted into a textbox of some kind. So, in the same way you will be able to load from a file *or* load from a URL, it would be nice to be able to diff from two files *or* diff from two URLs. But I'm not sure what the UI should be for the two-URL case -- there's no obvious parallel to showing two file pickers in a row. Unless we pop up a window that you then paste the URL into... which would be a URL picker of sorts. Anyway, I may be worrying about things too far ahead. This is definitely a useful thing to have in about:memory, and its UI might be good enough for now; we can always tweak it later if necessary.
Attachment #739337 -
Flags: review?(bugmail.mozilla)
Comment 9•11 years ago
|
||
Comment on attachment 739335 [details] [diff] [review] (part 1) - Correctly handle memory report files that have no measurements in the "explicit" or "other" sections. Review of attachment 739335 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comment below fixed. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +723,5 @@ > function appendAboutMemoryMain(aProcess, aHasMozMallocUsableSize, > aForceShowSmaps) > { > + let pcollsByProcess = > + getPCollsByProcess(aProcess, pcollsByProcess, aForceShowSmaps); getPCollsByProcess takes only two parameters; I think the middle arg here should be taken out. Guessing it was left over from a previous version.
Attachment #739335 -
Flags: review?(bugmail.mozilla) → review+
Comment 10•11 years ago
|
||
Comment on attachment 739337 [details] [diff] [review] (part 2) - Add ability to diff two files to about:memory. Review of attachment 739337 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +416,5 @@ > > let header = appendElement(document.body, "div", "ancillary"); > > + function appendHiddenFileInput(aId, aChangeListener) { > + let input = appendElementWithText(header, "input", "hidden", ""); I'd move this function out to the top level next to appendButton for consistency. Also, it looks like this patch to aboutMemory.js is not based on what's currently in hg. In the latest inbound, the appendButton function is inside the appendAboutMemoryFooter. Is there some other patch that this applies on top of that hasn't landed yet? @@ +448,5 @@ > + let filename1 = this.filename1; > + delete this.filename1; > + updateAboutMemoryFromTwoFiles(filename1, file.mozFullPath); > + } > + }); nit: Remove trailing whitespace while you're here @@ +848,5 @@ > + > + // Strip out some non-deterministic stuff that prevents clean diffs -- > + // e.g. PIDs, addresses. > + let strippedProcess = jr.process.replace(/pid \d+/, "pid NNN"); > + let strippedPath = jr.path.replace(/0x[0-9A-Fa-f]+/, "0xNNN"); In my differ I also needed to do the equivalent of: jr.path.replace(/zone\([0-9A-Fa-f]+\)/, "zone(NNN)"); because the zone(...) contains a hex address but it isn't prefixed with "0x". Maybe the right fix for that is to update the memory reporter, but if so, I think this patch should still do that, and then a follow-up patch can remove it and update the memory reporter. ├───39,667,720 B (34.86%) -- js-non-window │ ├──30,061,440 B (26.41%) -- zones │ │ ├──26,703,688 B (23.46%) -- zone(10b427800) @@ +889,5 @@ > + > + for (let processPath in aDReportMap2) { > + let r2 = aDReportMap2[processPath]; > + result[processPath] = new DReport(r2._kind, r2._units, r2._amount, > + r2._description, r2._nMerged); Something I did in my differ and found slightly useful is to annotate which reports were in one file but not the other, by tacking on "(new)" and "(removed)" to the path. It allows somebody to looking at the diff to more quickly point out possible causes of regressions, because if a new jsm shows up that wasn't there before then it's almost certainly related to the regression. Ideally these would be obvious anyway but sometimes the noise in the report data can hide this. It's something to consider but if you want to leave it out for now I'm fine with that. ::: toolkit/components/aboutmemory/tests/test_aboutmemory3.xul @@ +139,5 @@ > + > + input1.dispatchEvent(e); > + > + } else { > + let file2 = getFile(aFilename2); I'd rather move this line down to just above where file2.path is used. @@ +147,5 @@ > + > + // Hack alert: input2's onchange handler calls input3.click(). But we > + // don't want that to happen, because we want to bypass the file picker > + // for the test. So we set |e.skipClick|, which causes input3.click() to > + // be skipped, and dispatch the second change event directly ourselves. This comment refers to input3 which presumably was in an older version of this patch but isn't in this one.
Attachment #739337 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 11•11 years ago
|
||
> Is there some other patch that this > applies on top of that hasn't landed yet? Yes! These patches apply on top of the patches in bug 856917. Sorry I forgot to mention that.
Assignee | ||
Comment 12•11 years ago
|
||
Trivial patch. This simplifies memory report diffs.
Attachment #740119 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 13•11 years ago
|
||
I wrote a simple patch that modified the path, but then I decided that it would be more in the spirit of the existing UI to add [+]/[-] elements that are not part of the path, but are notes added in the same way that e.g. "[2]" notes are currently added. This patch does that. The tooltip for these notes says "This value was only present in the {first,second} set of memory reports."
Attachment #740164 -
Flags: review?(bugmail.mozilla)
Attachment #740119 -
Flags: review?(wmccloskey) → review+
Comment 14•11 years ago
|
||
Comment on attachment 740164 [details] [diff] [review] (part 3b) - Add [+] and [-] elements to diffs where appropriate. Review of attachment 740164 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #740164 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6969de65be https://hg.mozilla.org/integration/mozilla-inbound/rev/0640cd38f16c https://hg.mozilla.org/integration/mozilla-inbound/rev/150bb08461b5
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8f6969de65be https://hg.mozilla.org/mozilla-central/rev/0640cd38f16c https://hg.mozilla.org/mozilla-central/rev/150bb08461b5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•