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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
The diff tool is in toolkit/components/aboutmemory/tools/diff-memory-reports.js, BTW.
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.
Attachment #735606 - Flags: review?(bugmail.mozilla) → review+
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?
> - 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.
(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.
Depends on: 856917
Summary: Handle memory reports diffs better in about:memory → Add ability to diff two files to about:memory
Attachment #735606 - Attachment is obsolete: true
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)
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 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 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+
> 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.
Trivial patch.  This simplifies memory report diffs.
Attachment #740119 - Flags: review?(wmccloskey)
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 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+
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.

Attachment

General

Created:
Updated:
Size: