Closed Bug 768470 Opened 12 years ago Closed 12 years ago

Export and import memory reporter data as JSON

Categories

(Toolkit :: about:memory, defect)

Product:
Toolkit
Component:
about:memory
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 6 obsolete files)

Add ability to import/export memory reports as JSON.
(deleted), patch
n.nethercote
: review+
Details | Diff | Splinter Review
I think that supporting about:memory on b2g as a page on the device (like in Firefox currently) is important from a bug-finding PoV. But from a bug /reporting/ PoV, it's not that useful. On B2G, copy/pasting the whole about:memory page is difficult for a number of reasons (no copy/paste for one). It would be pretty awesome if we had a button in about:memory which sent a full verbose report to a pastebin somewhere. about:memory on b2g may be able to use copy/paste to generate the verbose report, like the test does; I'm not sure if that would work. Alternatively, we could write a script separate from about:memory which dumps the memory reporters, say in JSON. We could then write an external parser. This would be nice in that it would have much lower memory ramifications than loading about:memory verbose. And those of us *looking* at the memory report wouldn't have to look at the verbose report. But of course this would require us to duplicate a lot of the about:memory logic.
I presume you all watch this component, but just in case...
OS: Mac OS X → All
Hardware: x86 → All
As a follow-up to this bug, we could include the list of extensions, buildid, etc. in the pastebin dump. That would make our lives so much better!
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
It would be nice to be able to trigger the generation of this using something like a signal (linux only but b2g is linux) i.e. kill -s usr2 <PID> and have it written locally to a file. Actually, sending the signal could invoke some arbitrary javascript which could then be customized to produce different types/amounts of information.
Today in memshrink we decided that a reasonable path forward would be to dump the memory reporters to a machine-readable format, and then allow you to import that data /into/ about:memory (e.g. via copy/paste, or directly from a file). That way, we could re-use all of our existing about:memory machinery.
Summary: Support dumping about:memory (or something like it) to text → Export and import memory reporter data as JSON
This patch just serializes the memory reports as JSON text and then deserializes them before continuing, as a basic proof of context. There's more to be done, but it's mostly interesting because it made me think of the following issues. 1. If the application gets a signal, how does it then trigger some of this code? Can we do something like make an iframe, set its onload handler to a function in aboutMemory.js, and then close the iframe once the serialization is done? 2. As soon as we implement this, people will request that there be something equivalent to about:memory's "Minimize memory usage" button, I guarantee it. I'm not sure how to facilitate that. 3. Multi-process is going to be interesting. Currently, we register an observer for the "child-memory-reporter-update" process, and if that is triggered, we redo our measurements and regenerate the page. This could lead to duplication in text output. Consider this sequence of events: a) parent process clears the page's <body> b) parent gets the measurements from the memory reporters c) parent starts generating the page d) child process adds additional measurements to the reporter service e) child process sends "child-memory-reporter-update" event f) parent catches the event g) parent clears the page's <body> h) parent gets the (updated) measurements from the memory reporters i) parent generates the page In about:memory, the fact that we did both steps (c) and (i) doesn't matter, because the display is refreshed. But if we instead generate JSON text... well, what happens depends on where the text is sent. If we dump() it, we'll see two sets of measurements. If there's a file (e.g. aboutmemory.txt) then (c) will write it once and then (i) will overwrite it. 4. The text is voluminous, e.g. 100s of KBs. I suspect dump() won't be usable, and we'll have to send it to a file.
(In reply to Nicholas Nethercote [:njn] from comment #6) > Created attachment 654046 [details] [diff] [review] > v0: add JSON serialization into the middle of aboutMemory.js. > > This patch just serializes the memory reports as JSON text and then > deserializes them before continuing, as a basic proof of context. There's > more to be done, but it's mostly interesting because it made me think of the > following issues. > > 1. If the application gets a signal, how does it then trigger some of this > code? Can we do something like make an iframe, set its onload handler to > a function in aboutMemory.js, and then close the iframe once the > serialization is done? I'm hazy on all of this. I was assuming I could have a the signal initiate a runnable on the main thread that would go and execute some arbitrary JS within the appopriate context to generate the report. > 2. As soon as we implement this, people will request that there be something > equivalent to about:memory's "Minimize memory usage" button, I guarantee it. > I'm not sure how to facilitate that. If responding to the signal were to read a JS file from disk and execute the text contained within then you could implement that in a number of interesting ways. There are obvious security implications here, but this could be a "needs to be enabled" feature. > 3. Multi-process is going to be interesting. Currently, we register an > observer for the "child-memory-reporter-update" process, and if that is > triggered, we redo our measurements and regenerate the page. This could > lead to duplication in text output. Consider this sequence of events: > > a) parent process clears the page's <body> > b) parent gets the measurements from the memory reporters > c) parent starts generating the page > d) child process adds additional measurements to the reporter service > e) child process sends "child-memory-reporter-update" event > f) parent catches the event > g) parent clears the page's <body> > h) parent gets the (updated) measurements from the memory reporters > i) parent generates the page > > In about:memory, the fact that we did both steps (c) and (i) doesn't matter, > because the display is refreshed. But if we instead generate JSON text... > well, what happens depends on where the text is sent. If we dump() it, > we'll see two sets of measurements. If there's a file (e.g. > aboutmemory.txt) then (c) will write it once and then (i) will overwrite it. Each process should probably generate a different file which includes the PID in the filename. It could also include a timestamp. It could then forward the name of the file it generated to the parent who would then read the files and merge the contents. > 4. The text is voluminous, e.g. 100s of KBs. I suspect dump() won't be > usable, and we'll have to send it to a file. Having the information in a file, or the option to have it in a file seems useful, especially since one of the use cases for this is trying to figure out what's causing OOM (Out of Memory) conditions on the device.
You could couple the signal processing via the observer service (which is synchronous, which could be useful): https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIObserverService https://developer.mozilla.org/en-US/docs/Observer_Notifications NB: I think for signals you'd need to dispatch a runnable to the main thread if a different thread observes it? (I assume nnethercote is all over the semantics of signals; I am most assuredly not!)
Signals are sent to a process (and not to a thread), so they could potentially be serviced by any thread which is running. So dispatching a runnable would be the right thing to do. Using the observer service also seems useful, since you might then be able to have different behaviours for different signals. There is SIGUSR1 and SIGUSR2 which are "user defined"
> 1. If the application gets a signal, how does it then trigger some of this > code? Can we do something like make an iframe, set its onload handler to > a function in aboutMemory.js, and then close the iframe once the > serialization is done? If you really want to write it in JS, you could write an "XPCOM module" (I think that's the right name) and invoke that. You'd call CreateInstance(nsIAboutMemoryDumper) and get a handle to your JS object. I'm unfortunately not sure what magic you'd need to use here, but Gavin could definitely help. (I think it would be kind of like the magic we use for BrowserElementParent.js, except that is loaded at startup, as opposed to on demand.) But have you considered doing this in C++? You could use the JSAPI to build up the object, and then dump it to JSON. > 2. As soon as we implement this, people will request that there be something > equivalent to about:memory's "Minimize memory usage" button, I guarantee it. > I'm not sure how to facilitate that. We have at least two signals to play with, right? :) > 3. Multi-process is going to be interesting. You could ignore it for the moment. We could write a script which sends the signal to each of the child processes, in B2G. So long as the filename contained the PID, we'd be OK. > 4. The text is voluminous, e.g. 100s of KBs. I too suspect that dumping to a file is probably our best bet. I'd want to write a script which automatically grabs this file off the device, but if you put the file in a predictable location (and put the pid in the filename), that would probably work.
> You could use the JSAPI to build up the object, and then dump it to JSON. Or you could even write the JSON yourself, which would have the advantage of zero memory perturbation. That would be a hack, but perhaps not a bad one!
(In reply to Justin Lebar [:jlebar] from comment #10) > If you really want to write it in JS, you could write an "XPCOM module" (I > think that's the right name) and invoke that. You'd call > CreateInstance(nsIAboutMemoryDumper) and get a handle to your JS object. That sort of interface would be ideal for the use cases that I have been thinking of for Android: - tracking down memory hogs in mochitests: create a nsIAboutMemoryDumper and dump to a file (or console) at select places in the test - getting about:memory info automatically on low memory: observe the memory-pressure event and dump info when it is triggered
Note that even if we went with the signal-based approach, we could still provide an XPCOM interface for dumping about:memory data. Indeed, we could have our class listen on an observer topic, and then wouldn't even need that interface.
This patch adds a "Read reports from file" button to about:memory, which reads memory reports stored as JSON. We don't yet have a way to generate such reports; I've been using a hand-written one for testing purposes. Here's an example of the JSON. { "hasMozMallocUsableSize":true, "reports": [ {"process":"", "path":"explicit/foo/bar", "kind":1, "units":0, "amount":2000000, "description":"Foo bar."}, {"process":"", "path":"heap-allocated", "kind":1, "units":0, "amount":3000000, "description":"Heap allocated."}, {"process":"", "path":"vsize", "kind":1, "units":0, "amount":10000000, "description":"Vsize."} ] } And here's a JSON schema for the format (I'll put this in the code when I actually write code that actually produces data in this format). { "properties": { "hasMozMallocUsableSize": { "type": "boolean", "description": "nsIMemoryReporterManager::hasMozMallocUsableSize", "required": true }, "reports": { "type": "array", "description": "The memory reports.", "required": true "minItems": 1, "items": { "type": "object", "properties": { "process": { "type": "string", "description": "nsIMemoryReporter::process", "required": true }, "path": { "type": "string", "description": "nsIMemoryReporter::path", "required": true, "minLength": 1 }, "kind": { "type": "integer", "description": "nsIMemoryReporter::kind", "required": true }, "units": { "type": "integer", "description": "nsIMemoryReporter::units", "required": true }, "amount": { "type": "integer", "description": "nsIMemoryReporter::amount", "required": true }, "description": { "type": "string", "description": "nsIMemoryReporter::description", "required": true } } } } } } There's obviously also new code for reading from file. Nothing too tricky, but keep in mind that I'm definitely not a web developer :) Also, this required a significant change to how exceptions are handled, in order to handle malformed JSON files with some level of grace. - There's now a distinction between assertInput() which is called for malformed reporter data (which could come from file) vs. assert() which is called for problems that indicate defects in aboutMemory.js's code. - In the malformed data case, we print an error message in about:memory and then still show the page footer, so you can easily update or re-read from file. In the defect/assertion case, we don't catch the exception. - As part of this change, I moved the existing exception handling for memory reporters further "outwards", so it's all in one place now. There's a new test for this stuff, which tests both a successful load and a load of a file that is malformed. Unfortunately the paths to the files are hard-coded, and I haven't yet worked out how to fix that.
Attachment #654046 - Attachment is obsolete: true
Attachment #655528 - Flags: feedback?(justin.lebar+bug)
With the attached patch, the import side of things is in fairly good shape. I'm still not clear on what to do for the export side. My current best idea is to add nsIMemoryReporterManager::dumpReports(), which would dump the memory reports for the current process to a file in /tmp (or equivalent). The filename would include "memory-reports" in it. It feels a bit cheap and nasty but it might be good enough? Suggestions welcome.
This updated patch fixes the problems with the hardcoded file names in the test.
Attachment #655528 - Attachment is obsolete: true
Attachment #655528 - Flags: feedback?(justin.lebar+bug)
Attachment #655846 - Flags: feedback?(justin.lebar+bug)
This patch adds nsIMemoryReporterManager::dumpReports(), which runs all the memory reporters in the current process and dumps them into a file called "memory-reports.txt" (or similar, if such a file already exists) in the tmp directory. I'm new to doing File I/O in Gecko so please look at that code carefully. It also adds a button to about:memory that invokes dumpReports(). I should add a test for that, but not today. I also added |gMgr| as a global variable so I don't have to pass it around everywhere. Comments on both the code and the general approach are welcome.
Attachment #655902 - Flags: feedback?(justin.lebar+bug)
I've only very roughly skimmed these patches (sorry, b2g work week), but one concern I have offhand is: This appears to be a persistent way of storing about:memory data, but it's not -- if we change the order of the enums, for example, we're hosed. That's probably fine -- we can use an old version of Firefox to parse the file, in the odd case that we care -- but we should probably put a version number in this file.
Comment on attachment 655846 [details] [diff] [review] Add "Read reports from file" button to about:memory. >+function handleException(ex) >+{ >+ let str = ex.toString(); >+ if (str.search(gAssertionFailureMsgPrefix) >= 0) { We now have string.startsWith(), if you like. >+ throw ex; // Argh, assertion failure within this file! Give up. >+ } else { >+ badInput(ex); // File or memory reporter problem. Print a message and >+ } // append the footer. It seems like badInput just prints a message; I'm not sure what you mean about appending the footer. Maybe the comment is old? >@@ -362,125 +385,222 @@ function doCC() >- * Top-level function that does the work of generating the page. >+ * Top-level function that does the work of generating the page from the memory >+ * reporters. > */ > function updateAboutMemory() > { > // First, clear the page contents. Necessary because updateAboutMemory() > // might be called more than once due to the "child-memory-reporter-update" > // observer. > let body = clearBody(); > >- let mgr = Cc["@mozilla.org/memory-reporter-manager;1"]. >- getService(Ci.nsIMemoryReporterManager); >+ try { >+ // Process the reports from the memory reporters. >+ let mgr = Cc["@mozilla.org/memory-reporter-manager;1"]. >+ getService(Ci.nsIMemoryReporterManager); >+ let process = function(aIgnoreSingle, aIgnoreMulti, aHandleReport) { >+ processMemoryReporters(mgr, aIgnoreSingle, aIgnoreMulti, aHandleReport); >+ } >+ appendAboutMemoryMain(body, process, mgr.hasMozMallocUsableSize); > >+ } catch (ex) { >+ handleException(ex); >+ >+ } finally { >+ appendAboutMemoryFooter(body); >+ } >+} IIUC, this patch changes our behavior when we encounter an exception. Before, if the exception came from within the memory reporter, we'd go on and process the other reporters. But with this patch, we'll simply stop, because the exception bubbles out of the handle memory multi-reporter function. Is that what we want to do? >+/** >+ * Like updateAboutMemory(), but gets its data from file instead of the memory >+ * reporters. s/file/a file/. Or s/file/a File/? >+function appendAboutMemoryMain(aBody, aProcess, aHasMozMallocUsableSize) It's not clear to me why it's better to pass aBody around as an explicit param, here and elsewhere instead of having a global |body| variable. >+ const RDDesc = "Read memory report data from file."; While we're at it, it would be cool to have "Read memory report data from clipboard," which is often easier than saving to a file. See perhaps accessible/tests/mochitest/common.js::getTextFromClipboard(). But we (or even I!) could do that as a follow-up. >diff --git a/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul b/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul It's not clear to me how this isn't hardcoding in the path, but anyway, that doesn't bother me. :)
Attachment #655846 - Flags: feedback?(justin.lebar+bug) → review+
> > IIUC, this patch changes our behavior when we encounter an exception. > Before, > if the exception came from within the memory reporter, we'd go on and process > the other reporters. But with this patch, we'll simply stop, because the > exception bubbles out of the handle memory multi-reporter function. We won't stop. Such an exception will be caught in updateAboutMemory() or updateAboutMemoryFromFile(), both of which will call handleException(), which will see that the exception's string doesn't contain |gAssertionFailureMsgPrefix|, and so will call badInput(), which prints an error message in the page. Then control will return to updateAboutMemory/updateMemoryFromFile, which will then call appendAboutMemoryFooter(). > While we're at it, it would be cool to have "Read memory report data from > clipboard," which is often easier than saving to a file. See perhaps > accessible/tests/mochitest/common.js::getTextFromClipboard(). But we (or > even > I!) could do that as a follow-up. How would the data be put into the clipboard? > >diff --git a/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul b/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul > > It's not clear to me how this isn't hardcoding in the path, but anyway, that > doesn't bother me. :) I could have been clearer; I meant "hardcoding an *absolute* path" -- previously I was loading /home/njn/a.json! :)
> We won't stop. We'll print the footer, yes, but we won't print any more memory reporters, right? > How would the data be put into the clipboard? Someone posted a file online (say in bugzilla); I opened it in Firefox, ctrl+a ctrl+c.
>diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl >--- a/xpcom/base/nsIMemoryReporter.idl >+++ b/xpcom/base/nsIMemoryReporter.idl >@@ -286,16 +286,23 @@ interface nsIMemoryReporterManager : nsI > * to compute in JS. Accesses can fail. > */ > readonly attribute int64_t explicit; > > /* > * This attribute indicates if moz_malloc_usable_size() works. > */ > readonly attribute boolean hasMozMallocUsableSize; >+ >+ /* >+ * This dumps the memory reports for this process to a file in the tmp >+ * directory called memory-reports.txt (or something similar, such as >+ * memory-reports-1.txt; no existing file will be overwritten). >+ */ >+ void dumpReports (); Update the IID. >diff --git a/xpcom/base/nsMemoryReporterManager.cpp b/xpcom/base/nsMemoryReporterManager.cpp >--- a/xpcom/base/nsMemoryReporterManager.cpp >+++ b/xpcom/base/nsMemoryReporterManager.cpp >@@ -834,16 +839,176 @@ nsMemoryReporterManager::GetHasMozMalloc > return NS_ERROR_OUT_OF_MEMORY; > } > size_t usable = moz_malloc_usable_size(p); > free(p); > *aHas = !!(usable > 0); > return NS_OK; > } > >+#define DUMP(o, s) \ >+ do { \ >+ const char* s2 = (s); \ >+ uint32_t dummy; \ >+ nsresult rv = (o)->Write((s2), strlen(s2), &dummy); \ >+ NS_ENSURE_SUCCESS(rv, rv); \ >+ } while (0) >+ >+static nsresult >+DumpReport(nsIFileOutputStream *aOStream, bool isFirst, >+ const nsACString &aProcess, const nsACString &aPath, int32_t aKind, >+ int32_t aUnits, int64_t aAmount, const nsACString &aDescription) >+ nsCString path(aPath); The obvious comment would be to use nsCAutoString here, but nsCAutoString/nsAutoCString is stuck at a 64-char buffer, which isn't long enough for many of our paths. If we care, we could copy path into a fixed-size on-stack buffer and do ReplaceSubstring ourselves. It wouldn't be hard, but I'm not sure we care, since presumably this case is easy for jemalloc to handle efficiently. Or maybe nsFixedCString would work. You could implement a class in this CPP file, or just do char pathBuf[4 * 1024]; nsFixedCString path(buf, sizeof(buf), 0); Maybe that would work. >+ DUMP(aOStream, "\", \"kind\": "); >+ DUMP(aOStream, nsPrintfCString("%d", aKind).get()); >+ >+ DUMP(aOStream, ", \"units\": "); >+ DUMP(aOStream, nsPrintfCString("%d", aUnits).get()); >+ >+ DUMP(aOStream, ", \"amount\": "); >+ DUMP(aOStream, nsPrintfCString("%lld", aAmount).get()); These nsPrintfCString's are fine, because nsPrintfCString allocates 16 chars of on-stack memory. >+NS_IMETHODIMP >+nsMemoryReporterManager::DumpReports() >+{ >+ // Open a file in NS_OS_TEMP_DIR for writing. >+ >+ nsCOMPtr<nsIFile> tmpFile; >+ nsresult rv = >+ NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(tmpFile)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = tmpFile->AppendNative(nsDependentCString("memory-reports.json")); NS_LITERAL_CSTRING, unless that doesn't work for some reason? Can you please put the pid in the filename? We usually do something awful at the top of files which need to do this: #ifdef XP_WIN #include <process.h> #define getpid _getpid #else #include <unistd.h> #endif >+ rv = tmpFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600); So this creates a unique file with the prefix "$TMPDIR/memory-reports.json"? >+ nsCOMPtr<nsIFileOutputStream> ostream = >+ do_CreateInstance("@mozilla.org/network/file-output-stream;1"); >+ rv = ostream->Init(tmpFile, -1, -1, 0); The docs clam that the second -1 corresponds to file perms of 0664, which isn't the 0600 above. Not sure if it matters. >+ bool more; Move this closer to where it's used (and initialize it to false for good measure)? >+ nsCOMPtr<nsIMemoryReporterManager> mgr = do_GetService("@mozilla.org/memory-reporter-manager;1"); >+ if (mgr == nullptr) >+ return NS_ERROR_FAILURE; >+ >+ DUMP(ostream, "{\n \"hasMozMallocUsableSize\": "); >+ >+ bool hasMozMallocUsableSize; >+ mgr->GetHasMozMallocUsableSize(&hasMozMallocUsableSize); >+ DUMP(ostream, hasMozMallocUsableSize ? "true" : "false"); >+ DUMP(ostream, ",\n"); >+ DUMP(ostream, " \"reports\": "); >+ >+ // Process single reporters. >+ bool isFirst = true; >+ nsCOMPtr<nsISimpleEnumerator> e; >+ EnumerateReporters(getter_AddRefs(e)); >+ while (NS_SUCCEEDED(e->HasMoreElements(&more)) && more) { >+ nsCOMPtr<nsIMemoryReporter> r; >+ e->GetNext(getter_AddRefs(r)); >+ >+ nsCString process; nsCAutoString (or nsAutoCString, depending on when you land). >+ rv = r->GetProcess(process); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCString path; Maybe nsFixedCString? >+ rv = r->GetPath(path); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ int32_t kind; >+ rv = r->GetKind(&kind); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ int32_t units; >+ rv = r->GetUnits(&units); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ int64_t amount; >+ rv = r->GetAmount(&amount); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCString description; Maybe nsFixedCString? Anyway, this looks good. The only trick now is the signal handler...
Attachment #655902 - Flags: feedback?(justin.lebar+bug) → review+
> We'll print the footer, yes, but we won't print any more memory reporters, > right? Oh, right. True. I don't think it matters -- I don't think we've seen reporters aborting in practice -- and handling it would be more difficult, so I'll leave it as is.
> >+ rv = tmpFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600); > > So this creates a unique file with the prefix "$TMPDIR/memory-reports.json"? I've only tested on Linux, where it does. If that file already exists, then it does "$TMPDIR/memory-reports-1.json", and so on. > >+ nsCOMPtr<nsIFileOutputStream> ostream = > >+ do_CreateInstance("@mozilla.org/network/file-output-stream;1"); > >+ rv = ostream->Init(tmpFile, -1, -1, 0); > > The docs clam that the second -1 corresponds to file perms of 0664, which > isn't > the 0600 above. Not sure if it matters. I asked about this on IRC. The file is already created and Init() just opens it so the permissions parameter here has no effect. Maybe there's a circumstance where Init() can create a file... not sure.
A w.r.t. to all the string suggestions... I'd prefer to just use nsCString throughout. This isn't perf-critical, and all the variants you suggested are more complex or assume that the string won't exceed a certain length (which is probably true in a number of cases, but I'd prefer to not have to even think about it).
I don't care at all about the performance: The question is whether the heap perturbation would be significant. Probably not; I'm happy to optimize after the fact.
Comment on attachment 655902 [details] [diff] [review] Add nsIMemoryReporterManager::dumpReports and use it to implement an "Update and write reports to file" button to about:memory. > +NS_IMETHODIMP > +nsMemoryReporterManager::DumpReports() > +[...] > + nsCOMPtr<nsIMemoryReporterManager> mgr = > do_GetService("@mozilla.org/memory-reporter-manager;1"); > + if (mgr == nullptr) > + return NS_ERROR_FAILURE; Isn't mgr == this?
Also, we do |if (!pointer)|, not |if (pointer == nullptr)| in Gecko.
> + bool more; Please move this closer to where it's used. I think I'm done nitting now. I clearly should have actually read that function in the first place. :)
Comment on attachment 655902 [details] [diff] [review] Add nsIMemoryReporterManager::dumpReports and use it to implement an "Update and write reports to file" button to about:memory. Strangely, I get rejects when I apply this patch to hg or git. In hg, patching file toolkit/components/aboutmemory/content/aboutMemory.js Hunk #2 FAILED at 171 Hunk #3 FAILED at 399 Hunk #4 FAILED at 515 3 out of 9 hunks FAILED -- saving rejects to file toolkit/components/aboutmemory/content/aboutMemory.js.rej In git, I get those failures plus a failure to apply one of the hunks from MapsMemoryReporter.cpp. Did you make some (whitespace?) changes in a patch below these or something?
> Did you make some (whitespace?) changes in a patch below these or something? I don't think so. Are you applying both patches? FWIW, I'm planning to combine the two into a single patch, possibly remove the "Write reports" button from about:memory, and think some more about the handling of multiple processes. E.g. I probably will avoid dumping any reports that we've obtained from a child process when DumpReport() is called in the parent process.
> Are you applying both patches? > FWIW, I'm planning to combine the two into a single patch, Sounds good. > possibly remove the "Write reports" button from about:memory, Sure, I think copy/paste is probably a better UX. > I probably will avoid dumping any reports that we've obtained from a child process when > DumpReport() is called in the parent process. That sounds like the right thing to do.
I'm almost done with a strawman signal-handler patch. It would be helpful if you'd write to stderr the filename that we dumped to.
> It would be helpful if you'd write to stderr the filename that we dumped to. Unconditionally? That would be spamming the console...
Updated patch. Notes: - I haven't added code to write the filename to stderr because I'm not sure how/why you want it. I figure you can add it easily enough. - dumpReports() now only dumps reports for the current process, and it puts the pid in the "process" field of each report. jlebar, do you want me to land? (Though I need to run it past try server first.) Do you want me to wait?
Attachment #655846 - Attachment is obsolete: true
Attachment #655902 - Attachment is obsolete: true
(In reply to Nicholas Nethercote [:njn] from comment #34) > > It would be helpful if you'd write to stderr the filename that we dumped to. > > Unconditionally? That would be spamming the console... I guess we'd only want it in response to an interactive signal. When you press the button in about:memory, ideally we'd display the filename there, in about:memory. Although I'd also want to dump to logcat on Android. We can figure this all out in follow-ups. > jlebar, do you want me to land? You kept the button in about:memory to dump to a file? If so, then sure. If not, then there's no way to execute most of this code, so we should wait to land.
Blocks: 788021
Nick, we need to at least escape " to \" in the descriptions. :)
Also I don't think we can have literal newlines in a string, so we need to escape our descriptions' newlines to the c-string "\\n".
Attachment #659598 - Flags: review?(n.nethercote)
Comment on attachment 659598 [details] [diff] [review] Followup, v1 - Add more escaping to descriptions. Review of attachment 659598 [details] [diff] [review]: ----------------------------------------------------------------- I already have this change in my local patch :) I'm having trouble with the test. It passes on all platforms on debug builds, but on opt builds it fails on Windows and Mac. I can reproduce the failure on Mac, it's intermittent. I assume it's because the file read is async and so if it doesn't finish quickly enough the cut+paste fails. I'm not sure how do fix this.
Attachment #659598 - Flags: review?(n.nethercote) → review+
I got the test working by manuallying polling the clipboard for up to three seconds. Try server results look good, though I'm waiting on a second round of runs just to be sure. Assuming they look good, I'll land this tomorrow.
Attachment #657807 - Attachment is obsolete: true
Attachment #659598 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/34c712af8b7c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 790947
Blocks: 797890
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: