848560 - Expose JSON memory reporter dumping functionality in about:memory

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

We have infrastucture to dump memory reports as JSON, which has proven very useful on B2G.  But it only works on Linux/Android, and only when you send a magic signal to the process, and then it dumps a gzip'd file to the $TMPDIR.

We should expose this in about:memory so that people can attach JSON dumps to bugs.  Then others can import the dump and use about:memory to inspect the data more easily (because branch collapsing/expanding can be used).  Also, when two measurements are taken, we can use the new diff tool.  (I would have loved to be able to do this for bug 845430, for example).

Users should be able to choose the file name, of course.  And it's probably best to not gzip it, because that makes it harder to use from bugs (e.g. you can't use about:memory's "Read reports from clipboard")... assuming that doesn't regularly hit the file size limit in Bugzilla.

Comment 1

[Nicholas Nethercote [inactive]]

I have a patch stack that successfully dumps a .json.gz file.  Now I want to be able to read .json.gz files (i.e. gzip'd JSON files).

We have nsIGZWriter but no nsIGZReader.  We also have nsIZipWriter and nsIZipReader.

So I guess my options are to (a) write nsIGZReader, or (b) switch to nsIZipWriter/nsIZipReader instead.  The latter seems easier.  (I could even remove nsIGZWriter, perhaps.)

I'm also inclined to remove the "Read reports from clipboard" button.  If gzipped/zipped JSON is to become the standard format, it seems like it won't be useful.

Comment 2

[Justin Lebar (not reading bugmail)]

Using nsIZip{Reader,Writer} seems easier, but I'm not sure it is.  A zip file is the equivalent of a .tar.gz: It can hold more than one file.  So to read a memory report you'd either have to look for a file at a specific path, or you'd have to reject zip files which contain more than one file, or something like that.

> I'm also inclined to remove the "Read reports from clipboard" button.  If gzipped/zipped JSON is to 
> become the standard format, it seems like it won't be useful.

I'd be sad if we removed it before we have a replacement for its functionality (which is likely "read reports from URL on clipboard")...

Comment 3

[Nicholas Nethercote [inactive]]

I've written nsIGZFileReader.idl and nsGZFileReader.{h,cpp} and they're working.  They can read files that are gzipped or not gzipped.

The tricky part is accessing them from JS.  I want something like this:

    let gzReader = Cc["@mozilla.org/gz-file-reader;1"].
                     createInstance(Ci.nsIGZFileReader);
    gzReader.init(file);
    let jsonString = gzReader.read();
    gzReader.finish();

But that requires either putting nsGZFileReader into an existing XPCOM module (which one?) or creating a new module.  I have no experience with such things and so am not sure what to do next.

(I temporarily worked around this situation by adding a readGZFile() method to nsMemoryReporterManager, which calls onto nsGZFileReader.  That's how I know the code is working, but it's not a good long-term solution.)


> I'd be sad if we removed it before we have a replacement for its
> functionality (which is likely "read reports from URL on clipboard")...

I guess the use case is for viewing reports attached to bugs -- you want to be able to load them without having to save and then load the file?

Comment 4

[Nicholas Nethercote [inactive]]

Another interesting puzzle:  the user chooses a name for the saved file.  That's fine if we have a single process.  If we have multiple processes, we'll need to dump multiple files, so presumably we'll need to add the PID in there too.  Maybe for now it's easiest just to ignore this, since B2G's the only multi-process platform and it doesn't have about:memory.  (Experience has shown so far that YAGNI is a good approach when worrying about multi-process on desktop and mobile!)

Comment 5

[Justin Lebar (not reading bugmail)]

> I guess the use case is for viewing reports attached to bugs -- you want to be able to load them 
> without having to save and then load the file?

Yes.  I also will often |xclip memory-report| and then open it that way, but I can figure out another way to do that.

> But that requires either putting nsGZFileReader into an existing XPCOM module (which one?) or 
> creating a new module.  I have no experience with such things and so am not sure what to do next.

nsIGZFileWriter is in xpcom/, so if nsIGZFileReader is in the same place, you could put it in xpcom/build/XPCOMModule?

> (Experience has shown so far that YAGNI is a good approach when worrying about multi-process on 
> desktop and mobile!)

Indeed!

Comment 6

[Nicholas Nethercote [inactive]]

> nsIGZFileWriter is in xpcom/, so if nsIGZFileReader is in the same place,
> you could put it in xpcom/build/XPCOMModule?

Goodness.  I have it working and it was surprisingly pain-free.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - dmd::ClearReports() needs to check if DMD is running before doing anything to avoid crashing.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Prevent memory reporters from being registered while about:memory's tests are running.

This patch prevents new memory reporters being added mid-test.  I had to do
this to get the test in the next patch passing reliably;  the safe browsing
implementation often would register a "prefixset" reporter mid-test.  We've
seen occasional failures of test_aboutmemory.xul (bug 819138) and I'm hopeful
that this is the reason.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps.

This patch adds support for dumping memory reports from about:memory.  It also
expands about:memory's memory report loading functionality to allow gzipped or
ungzipped input files.

jlebar, I haven't modified the UI like we discussed;  I've deferred that to bug
856917.  There's enough going on here for one bug.

I renamed DumpMemoryReportsToFile as DumpMemoryInfoToTempDir.  My use of the
deliberately vague "Info" could be useful in future if we want to dump more
info (e.g. all JS strings -- bug 852010).

nsIGZReader is heavily inspired by nsIGZWriter.

I fixed the JSON dumping so there's now an EOL at the end of the file :)

Note that test_aboutmemory3.xul cheats slightly -- it dumps directly rather
than going through the button.  I couldn't work out how to do it via the save
dialog, and it still tests the important part of the functionality.

Note also that test_aboutmemory3.xul will now fail if the output doesn't match
what's expected due to the insertion of the crucial |ok(false, ...)|
statement(!)  In other words, this test could have had failures previously
without it being noticed.

Comment 10

[Nicholas Nethercote [inactive]]

I should note that "Read reports from clipboard" still works, but there's no easy way to import a gzipped memory reports file from a non-file source.

Comment 11

[Nils Maier [:nmaier]]

Comment on attachment 732188 [details] [diff] [review]
(part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps.

Review of attachment 732188 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsGZFileReader.cpp
@@ +39,5 @@
> +  // Get a FILE out of our nsIFile.  Convert that into a file descriptor which
> +  // gzip can own.  Then close our FILE, leaving only gzip's fd open.
> +
> +  FILE* file;
> +  nsresult rv = aFile->OpenANSIFileDesc("r", &file);

I'm pretty sure _fmode[1] is untouched.
Hence, you need to open the file in binary mode by specifying "rb"[2] explicitly. Which is a good idea anyway, as relying on some global variable in the CRT is nasty.
nsGZFileWriter seems to have the same issue ("w" instead of "wb") ...

This may cause your random truncation issue on Windows.

[1] http://msdn.microsoft.com/en-us/library/ee2849wt%28v=vs.100%29.aspx
[2] http://msdn.microsoft.com/en-us/library/yeby3zcb%28v=vs.100%29.aspx

Comment 12

[Nils Maier [:nmaier]]

Anyway, you'd be probably better off not reinventing the wheel with nsGZFileReader and instead use a regular file inputstream with a "@mozilla.org/streamconv;1?from=gzip&to=uncompressed" nsIStreamConverter on top (IIRC)

Comment 13

[Justin Lebar (not reading bugmail)]

(In reply to Nils Maier [:nmaier] from comment #12)
> Anyway, you'd be probably better off not reinventing the wheel with
> nsGZFileReader and instead use a regular file inputstream with a
> "@mozilla.org/streamconv;1?from=gzip&to=uncompressed" nsIStreamConverter on
> top (IIRC)

I think that reads gzip-compressed data, which is subtly different from .gz files.  We'd have to write a wrapper to strip the .gz headers anyway, at which point I think we might as well have nsGZFileReader.

Comment 14

[Nils Maier [:nmaier]]

> I think that reads gzip-compressed data, which is subtly different from .gz
> files.  We'd have to write a wrapper to strip the .gz headers anyway, at
> which point I think we might as well have nsGZFileReader.

You're thinking of the (raw) deflate converter (from=deflate, from=compress).
The from=gzip converter will actually handle the header:
http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsHTTPCompressConv.cpp#124

The one drawback of the stream converter is that it only implements .asyncConvert ATM, although it shouldn't be too hard to implement .convert as well.

Anyway, the current design of nsGZFileReader, being sync only, accepting only files, not streams, and reading the whole file at once, is kinda limited and flawed IMO. I'd hate to see add-on authors doing multi-megabyte uninterrupted sync I/O on the main thread because this interface is more superficially convenient and likely more discoverable than the stream converter.
Having some stream wrapper like the following would be better (if you don't like streamconv):
interface nsIGzipInputStream: nsIInputStream {
  void init(nsIInputStream aInputStream);
};
Rewriting nsGZFileWriter to be an output stream wrapper along these lines would be great, too.

Comment 15

[Justin Lebar (not reading bugmail)]

Do you mind if we land this and then let you rewrite both patches using the stream hotness?  This blocks important B2G work, and I'd hate to get derailed worrying about what madness addons might do.

Comment 16

[Nils Maier [:nmaier]]

(In reply to Justin Lebar [:jlebar] from comment #15)
> Do you mind if we land this and then let you rewrite both patches using the
> stream hotness?

Not my decision, obviously.
Using streamconv instead right now should be feasible without a lot of work.

>  This blocks important B2G work, and I'd hate to get
> derailed worrying about what madness addons might do.

Keep in mind that you're in the process of creating a *public* API that encourages such "madness".
I don't know about the B2G schedule and how urgent this bug is in the great scheme of things, but a public API is easy to add but hard to remove again.

Comment 17

[Nils Maier [:nmaier]]

Attachment: Alt (part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps.

Mostly the same as Nicholas' (part 3), but without nsGZFileReader.
It uses a stream converter instead.

I also slipped in a "w" -> "wb" mode change in nsGZFileWriter. Platforms not distinguishing between text and binary mode will just ignore the extra "b". From the man page:
> The mode string can also include the letter 'b' either as a last character or as a character between the characters in any of the two-character strings described above. This is strictly for compatibility with C89 and has no effect; the 'b' is ignored on all POSIX conforming systems, including Linux. (Other systems may treat text files and binary files differently, and adding the 'b' may be a good idea if you do I/O to a binary file and expect that your program may be ported to non-UNIX environments.)

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps. code=nnethercote,maierman.

Yes, this is much better.  Thanks, Nils!

About the only thing I don't like is relying on the .gz suffix to indicate
gzipped-ness.  I wonder if we should just remove support for reading ungzipped
files.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps. code=nnethercote,maierman.

I replaced the arrow functions with vanilla functions.  I'm not comfortable
using such a bleeding-edge feature.

Comment 20

[Nils Maier [:nmaier]]

Comment on attachment 733119 [details] [diff] [review]
(part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps.  code=nnethercote,maierman.

Review of attachment 733119 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +511,5 @@
> +    reader.onerror = () => { throw "FileReader.onerror"; };
> +    reader.onabort = () => { throw "FileReader.onabort"; };
> +    reader.onload = (aEvent) => {
> +      updateAboutMemoryFromJSONString(aEvent.target.result);
> +    };

You left these arrow functions in.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps. code=nnethercote,maierman.

I came up with a refactoring of some methods in nsMemoryInfoDumper.cpp that
makes bug 852010 much easier to implement.  Might as well do it here, since
this patch already moves things around quite a bit in that fit.

Comment 22

[Justin Lebar (not reading bugmail)]

Comment on attachment 732187 [details] [diff] [review]
(part 2) - Prevent memory reporters from being registered while about:memory's tests are running.

>diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl

uuid

Perhaps it would be helpful if block/unblock failed if we were already
blocked/unblocked.  Then we'd be less-likely to accidentally try to block
twice.

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 734462 [details] [diff] [review]
(part 3) - Add support to about:memory for writing memory report dumps and reading gzipped memory report dumps.  code=nnethercote,maierman.

>diff --git a/xpcom/base/nsIGZFileWriter.idl b/xpcom/base/nsIGZFileWriter.idl
>--- a/xpcom/base/nsIGZFileWriter.idl
>+++ b/xpcom/base/nsIGZFileWriter.idl

>   /**
>-   * Close this nsIGZFileWriter.  This method is run when the underlying object
>-   * is destroyed, so it's not strictly necessary to explicitly call it from
>-   * your code.
>+   * Close this nsIGZFileWriter.  Classes implementing nsIGZFileWriter should
>+   * run this method when the underlying object is destroyed, so that it's not
>+   * strictly necessary to explicitly call it from your code.
>    *
>    * It's an error to call this method twice, and it's an error to call write()
>    * after finish() has been called.
>    */
>   void finish();
> };

I think it's better to write interface comments from the perspective of the
user of the interface rather than a hypothetical second implementer, since the
vast majority of our interfaces have only one implementation.  Would you be
happy with s/should/will/ and s/so that/so/, or is that too close to the
original?

>diff --git a/toolkit/components/aboutmemory/content/aboutMemory.js b/toolkit/components/aboutmemory/content/aboutMemory.js

>@@ -432,16 +446,28 @@ function updateAboutMemory()
>+ * Handle an update exception.

Comments like this that merely parrot back the name of the function are a pet
peeve of mine.

Perhaps "Handle an exception which occurs while updating about:memory" would be
better, if that's what we mean?  Except essentially any exception will occur
while updating the page, so that's not very helpful.

Perhaps it would be better to call this clearBodyAndHandleException(), or
handleFatalException().  If it's not already there, perhaps we could put this
adjacent to handleException().

>+function handleUpdateException(aEx) {
>+  let body = clearBody();
>+  handleException(aEx);
>+  appendAboutMemoryFooter(body);
>+}

>@@ -467,80 +493,97 @@ function updateAboutMemoryFromJSONString
>-function updateAboutMemoryFromFile(aFile)
>+function updateAboutMemoryFromFile(aFilename)
> {
>-  // Note: reader.onload is called asynchronously, once FileReader.readAsText()
>-  // completes.  Therefore its exception handling has to be distinct from that
>-  // surrounding the |reader.readAsText(aFile)| call.
>+  try {
>+    let reader = new FileReader();
>+    reader.onerror = () => { throw "FileReader.onerror"; };
>+    reader.onabort = () => { throw "FileReader.onabort"; };
>+    reader.onload = (aEvent) => {
>+      updateAboutMemoryFromJSONString(aEvent.target.result);
>+    };

njn, are we using arrow functions?  I don't care either way.

>+    // If it doesn't have a .gz suffix, read it as a (legacy) ungzipped file.
>+    if (!/\.gz$/i.test(aFilename)) {

if (!aFilename.endsWith(".gz")) is only one char longer and a lot clearer (to
me, anyway).

>+      reader.readAsText(new File(aFilename));
>+      return;

>     }
> 
>-    let reader = new FileReader();
>-    reader.onerror = function(aEvent) { throw "FileReader.onerror"; };
>-    reader.onabort = function(aEvent) { throw "FileReader.onabort"; };
>-    reader.onload = function(aEvent) {
>-      updateAboutMemoryFromJSONString(aEvent.target.result);
>-    };
>-    reader.readAsText(file);
>+    // Read compressed gzip file.
>+    let converter = new nsGzipConverter();
>+    converter.asyncConvertData("gzip", "uncompressed", {
>+      data: "",
>+      onStartRequest: function(aR, aC) {},
>+      onDataAvailable: function(aR, aC, aStream, aO, aCount) {
>+        let bi = new nsBinaryStream(aStream);
>+        this.data += bi.readBytes(aCount);

Would it be better to create an array of data pieces and then pass that into
new Blob() instead of doing string concatenation here?

>+      },
>+      onStopRequest: function(aR, aC, aStatusCode) {
>+        try {
>+          if (!Components.isSuccessCode(aStatusCode)) {
>+            throw aStatusCode;
>+          }
>+          reader.readAsText(new Blob([this.data]));

Is there a reason not to call updateAboutMemoryFromJSONString directly?  That
is, is the FileReader doing some transformation on the blob data?  If so, could
you please add a comment explaining what's going on?

Does readAsText or new Blob throw an exception?  AFAICT from the docs they
don't.  If that's correct, we shouldn't use try/catch here.

>+        } catch (ex) {
>+          handleUpdateException(ex);
>+        }
>+      }
>+    }, null);

Both the plain-text and gz file reading are async, so I don't understand the
comment in the test that says

>    // [Actually, it's no longer async, but this code still works.]

>+
>+    let file = new nsFile(aFilename);
>+    let fileChan = Services.io.newChannelFromURI(Services.io.newFileURI(file));
>+    fileChan.asyncOpen(converter, null);
> 
>   } catch (ex) {
>-    let body = clearBody();
>-    handleException(ex);
>-    appendAboutMemoryFooter(body);
>+    handleUpdateException(ex);
>   }
> }
> 
> /**
>  * Like updateAboutMemoryFromFile(), but gets its data from the clipboard
>  * instead of a file.
>  */
> function updateAboutMemoryFromClipboard()
> {
>   // Get the clipboard's contents.
>   let cb = Cc["@mozilla.org/widget/clipboard;1"]
>              .getService(Components.interfaces.nsIClipboard);
>-  let transferable = Cc["@mozilla.org/widget/transferable;1"]
>-                       .createInstance(Ci.nsITransferable);
>+  let transferable = Cc["@mozilla.org/widget/transferable;1"].
>+                       createInstance(Ci.nsITransferable);

Nit: The line immediately below this uses the same style as is being reverted
here, so this change seems unwarranted to me.

>    let loadContext = window.QueryInterface(Ci.nsIInterfaceRequestor)
>                            .getInterface(Ci.nsIWebNavigation)
>                            .QueryInterface(Ci.nsILoadContext);

>@@ -657,24 +701,28 @@ function appendAboutMemoryFooter(aBody)
>   // The standard file input element is ugly.  So we hide it, and add a button
>   // that when clicked invokes the input element.
>   let input = appendElementWithText(div1, "input", "hidden", "input text");
>   input.type = "file";
>   input.id = "fileInput";   // has an id so it can be invoked by a test
>   input.addEventListener("change", function() {
>     let file = this.files[0];
>-    updateAboutMemoryFromFile(file);
>+    let filename = file.mozFullPath;
>+    updateAboutMemoryFromFile(filename);

Nit: updateAboutMemoryFromFile(file.mozFullPath);

>diff --git a/xpcom/base/nsMemoryInfoDumper.cpp b/xpcom/base/nsMemoryInfoDumper.cpp
>--- a/xpcom/base/nsMemoryInfoDumper.cpp
>+++ b/xpcom/base/nsMemoryInfoDumper.cpp

>@@ -707,17 +665,17 @@ class DumpMultiReporterCallback MOZ_FINA
>           const nsACString &aDescription,
>           nsISupports *aData)
>       {
>         nsCOMPtr<nsIGZFileWriter> writer = do_QueryInterface(aData);
>         NS_ENSURE_TRUE(writer, NS_ERROR_FAILURE);
> 
>         // The |isFirst = false| assumes that at least one single reporter is
>         // present and so will have been processed in
>-        // DumpMemoryReportsToFileImpl() below.
>+        // DumpProcessMemoryReportsToNamedFile() below.
>         return DumpReport(writer, /* isFirst = */ false, aProcess, aPath,
>             aKind, aUnits, aAmount, aDescription);
>         return NS_OK;
>       }
> };

Do you mean DumpProcessMemoryReportsToGZFileWriter?

>+  // Rename the memory reports file, now that we're done writing all the files.
>+  // It's final name is "memory-report<-identifier>-<pid>.json.gz".

Its

>@@ -948,23 +914,121 @@ nsMemoryInfoDumper::DumpMemoryReportsToF
>+  // Write a message on the console.

Is "write on the console" a Britishism?  I'd say "to".

>+nsresult
>+DumpProcessMemoryReportsToNamedFile(const nsAString& aFilename)

static nsresult

>+NS_IMETHODIMP
>+nsMemoryInfoDumper::DumpMemoryInfoToTempDir(const nsAString& aIdentifier,
>+                                            bool aMinimizeMemoryUsage,
>+                                            bool aDumpChildProcesses)
>+{

>+  if (aMinimizeMemoryUsage) {
>+    // Minimize memory usage, then run DumpMemoryInfoToTempDir again.
>+    nsRefPtr<DumpMemoryInfoToTempDirRunnable> callback =
>+      new DumpMemoryInfoToTempDirRunnable(identifier,
>+                                            /* minimizeMemoryUsage = */ false,
>+                                            /* dumpChildProcesses = */ false);

Indentation

>+NS_IMETHODIMP
>+nsMemoryInfoDumper::DumpMemoryReportsToNamedFile(
>+    const nsAString& aFilename,
>+    bool aMinimizeMemoryUsage,
>+    bool aDumpChildProcesses)
>+{
>+  // Kick off memory report dumps in our child processes, if applicable.  We
>+  // do this before doing our own report because writing a report may be I/O
>+  // bound, in which case we want to busy the CPU with other reports while we
>+  // work on our own.

Copy/paste mistake?

>+  if (aDumpChildProcesses) {
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }
>+
>+  if (aMinimizeMemoryUsage) {
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+  }
>+
>+  return DumpProcessMemoryReportsToNamedFile(aFilename);

If we don't support the aMinimizeMemoryUsage and aDumpChildProcesses params
here, this method shouldn't take them.

Also, I don't see why we have DumpProcessMemoryReportsToNamedFile if this is
the only place we call that function from.

>diff --git a/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul b/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
>--- a/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
>+++ b/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul

>-  <!-- This file tests the loading of memory reports from file in
>+  <!-- This file tests the saving and loading of memory reports to/from file in
>        about:memory. -->

Article police: s/file/a file/.

>       synthesizeKey("A", {accelKey: true});
>       synthesizeKey("C", {accelKey: true});
>       let actual = SpecialPowers.getClipboardData("text/unicode");
>+      actual = actual.replace(/pid \d+/, "pid NNN");

Nit: Would you mind /\bpid \d+\b/ instead, out of paranoia?

Comment 24

[Nicholas Nethercote [inactive]]

(In reply to Justin Lebar [:jlebar] from comment #23)
> Would you be
> happy with s/should/will/ and s/so that/so/, or is that too close to the
> original?

That's fine.


> njn, are we using arrow functions?  I don't care either way.

jorendorff said I should use them, so I'll keep them.


> if (!aFilename.endsWith(".gz")) is only one char longer and a lot clearer (to
> me, anyway).

Oh, I didn't know we had String.endsWith in Firefox.  I can remove aboutMemory.js's home-grown String.startsWith, too.


> Would it be better to create an array of data pieces and then pass that into
> new Blob() instead of doing string concatenation here?

Yes.  Done.


> Is there a reason not to call updateAboutMemoryFromJSONString directly?

I think Nils used the Blob so that reader would be used in both cases, but you're right that it's not necessary.  I used Array.join() to join the data pieces.


> Does readAsText or new Blob throw an exception?  AFAICT from the docs they
> don't.  If that's correct, we shouldn't use try/catch here.

I think that's there to catch the |throw aStatusCode;|.


> Both the plain-text and gz file reading are async, so I don't understand the
> comment in the test that says
> 
> >    // [Actually, it's no longer async, but this code still works.]

True.  I'll remove the comment.


> Do you mean DumpProcessMemoryReportsToGZFileWriter?

Yep.


> Nit: Would you mind /\bpid \d+\b/ instead, out of paranoia?

I changed it to this instead, to be even more careful:

      actual = actual.replace(/\(pid \d+\)/, "(pid NNN)");

Comment 25

[Justin Lebar (not reading bugmail)]

> I think that's there to catch the |throw aStatusCode;|.

Dunno if you intend to change this, so to be clear, if the only purpose of the try/catch is to catch the exception we throw two lines above, we should use a plain if statement.

Comment 26

[Nils Maier [:nmaier]]

(In reply to Nicholas Nethercote [:njn] from comment #24)
> > Is there a reason not to call updateAboutMemoryFromJSONString directly?
> 
> I think Nils used the Blob so that reader would be used in both cases, but
> you're right that it's not necessary.  I used Array.join() to join the data
> pieces.

Yes, the point was to always use the reader, to achieve the same result.
.readAsText() will parse the binary file data as utf-8 unless you explicitly specify another encoding:
https://developer.mozilla.org/en-US/docs/DOM/FileReader#readAsText%28%29
So, not using readAsText will treat data from an .gz differently than data from a plain file. I didn't question the utf-8 parsing, just kept using it and eventually forgot to mention this bit when I posted my patch.
If you assume .json.gz to be always ascii (with non-ascii stuff escaped), then not using the reader would be fine, I guess.

> > Does readAsText or new Blob throw an exception?  AFAICT from the docs they
> > don't.  If that's correct, we shouldn't use try/catch here.
> 
> I think that's there to catch the |throw aStatusCode;|.

That's one reason.
Also, new Blob() and .readAsText() may throw exceptions in - well - exceptional cases, such as when readText cannot allocate a buffer to hold the hold text, e.g. when the user runs out of memory and/or selects a huge file.
Not having a try-catch would just silently swallow the exception without propagating the error back to the user.

Comment 27

[Nils Maier [:nmaier]]

As to why Array.join isn't pretty great for string joining:
http://jsperf.com/string-part-joining

Comment 28

[Justin Lebar (not reading bugmail)]

I think we found that Array.join was preferable because it caused less memory churn.  But it's moot if we're using the Blob constructor.

Comment 29

[Nils Maier [:nmaier]]

(In reply to Justin Lebar [:jlebar] from comment #28)
> I think we found that Array.join was preferable because it caused less
> memory churn.
Is that still the case in todays engine(s)?
Anyway, since you're importing existing data, not measuring memory, you want to be fast ;)

Comment 30

[Justin Lebar (not reading bugmail)]

> Is that still the case in todays engine(s)?

Who knows, but I think it probably makes sense to keep doing what we measured to be good until we measure otherwise.

> Anyway, since you're importing existing data, not measuring memory, you want to be fast ;)

That's a fair point.  :)

Comment 31

[Nicholas Nethercote [inactive]]

(In reply to Nils Maier [:nmaier] from comment #27)
> As to why Array.join isn't pretty great for string joining:
> http://jsperf.com/string-part-joining

Ah, jsperf -- the site that makes it incredibly easy to write shitty microbenchmarks that don't measure what they're trying to.

I ran your tests.  The Array.join was 200x slower than using +=.  I know the JS engine well enough to know that this is ridiculous.  Turns out your tests don't do anything with the concatenated string during the timing phase.  As a result, the rope that is built up with += never gets flattened.

http://jsperf.com/string-part-joining/2 modifies the tests to do something with the result string.  With that in place, on my Linux box, Array.join is 25% slower than +=.  On my Mac, it's 17% faster than +=.  In other words, there's no noticeable difference.  Except that Array.join is simpler in this case.

Comment 32

[Nils Maier [:nmaier]]

Damn, I stand corrected and embarrassed for forgetting to have some code to force string flattening. :(
Time to go to bed.

Comment 33

[Nicholas Nethercote [inactive]]

> If you assume .json.gz to be always ascii (with non-ascii stuff escaped),
> then not using the reader would be fine, I guess.

That's not an assumption I want to make.  I'll put that readAsText call back in.  
That also avoids the need for the Array.join call :)

Comment 34

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf9107c1785
https://hg.mozilla.org/integration/mozilla-inbound/rev/560ce5e96e6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e47bb858304

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Part three of this bug broke AWSY/mobile. I see the following error in the log:

[JavaScript Error: "memDumper.dumpMemoryReportsToFile is not a function" {file: "chrome://browser/content/MemoryObserver.js" line: 62}]

Looks like the code in mobile/android/chrome/content/MemoryObserver.js wasn't updated.

Comment 36

[Nicholas Nethercote [inactive]]

> Part three of this bug broke AWSY/mobile.

Lying in bed this morning, I was just wondering how this change had manifested on mobile.  If I do a normal Fennec build will I see the problem, or is it somehow AWSY-specific?

Comment 37

[Kartikaya Gupta (email:[e-mail])]

It's not exercised during normal Fennec use, but you can trigger it by using the command at [1]. This is the same command that AWSY uses to get the about:memory dumps.

[1] https://wiki.mozilla.org/Mobile/Fennec/Android#about:memory

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dcf9107c1785
https://hg.mozilla.org/mozilla-central/rev/560ce5e96e6b
https://hg.mozilla.org/mozilla-central/rev/0e47bb858304

Comment 39

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Fix broken memory reporting on Fennec.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 735512 [details] [diff] [review]
(part 4) - Fix broken memory reporting on Fennec.

Review of attachment 735512 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!

Comment 41

[Nicholas Nethercote [inactive]]

Part 4:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbed45f6dbcc

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bbed45f6dbcc

Comment 43

[Jorge Quiñónez]

Probably too late, but why not include an option for XZ compression (LZMA2). Code for this compression is being submitted to Trunk (see bug 366559):

https://bugzilla.mozilla.org/show_bug.cgi?id=366559

Its 10 times slower than gzip. But, w/ modern multicore cpu's it shouldn't be a problem if you're compressing small files. This might be a definite win for transmission/bandwidth.

Comment 44

[Nicholas Nethercote [inactive]]

Thanks for the suggestion.  gzip is sufficient -- these files aren't produced that often, and gzip's compression is enough to get them well under the Bugzilla file size limit, which was the overriding concern.