Currently web worker memory usage does not appear to show up in about:memory. The attachment creates a worker that uses 200MB of RAM. The 200MB appears in heap-unclassified. (In comparison, using 200MB in the same way in the web page itself, and not in a worker, causes it to show up properly in the compartment in about:memory.) I don't know if this is a JS engine or a worker thing.

Weird... How does about:memory even know about my non-main JSRuntime?

(In reply to comment #1) > Weird... How does about:memory even know about my non-main JSRuntime? You mean, how does the memory show up in heap-unclasified at all? That number is reported by malloc.

Yeah, jemalloc knows how much memory it has allocated. So about:memory doesn't actually know about the non-main JSRuntime at all.

Hm, ok. Are our memory reporters threadsafe?

There is NS_THREADSAFE_MEMORY_REPORTER_IMPLEMENT in xpcom/base/nsIMemoryReporter.idl. See nsCacheService.cpp for an example. Is that good enough? Or maybe you need to do some locking when you're doing the memory measurement? XPConnectJSCompartmentsMultiReporter::CollectReports (in xpcjsruntime.cpp) might be instructive; it locks the GC heap before traversing it to get all the GC heap stats. The nsIMemoryMultiReporter interface might be more suitable for your purposes than nsIMemoryReporter.

This mimics XPConnect, I consolidated as much code as I could without making custom chunk allocators for workers (ick!).

Comment on attachment 549290 [details] [diff] [review] Patch, v1 Review of attachment 549290 [details] [diff] [review]: ----------------------------------------------------------------- What code is responsible for allocating chunks for web workers? ::: dom/workers/RuntimeService.cpp @@ +357,5 @@ > + const nsCString& name = stats->name; > + > + BYTES0(GetPath(name, "gc-heap/arena-headers"), JS_GC_HEAP_KIND, > + stats->gcHeapArenaHeaders, > + "See js/compartment/gc-heap/arena-headers."); Oh wow, this is way too much cut and paste from XPConnectJSCompartmentsMultiReporter for my liking. Keeping the two files in sync as new reporters are added will be a pain. There is enough similarity between the two CollectReports methods that it must be possible to factor out the common bits. The only significant differences AFAICT is that the paths are different (so pass in a function to generate them) and XPConnectJSCompartmentsMultiReporter reports a few extra things... but is there a reason why each worker can't report exactly the same things? Maybe that's the custom chunk allocator issue you mentioned?

Ok, this is much more consolidated. Look better?

Comment on attachment 549487 [details] [diff] [review] Patch, v2 Review of attachment 549487 [details] [diff] [review]: ----------------------------------------------------------------- The factoring out of the duplicated code is a big improvement! Thanks for that. There's still something I don't like, though. A heap of code got moved around, which is fine... but you also took the opportunity to reformat it, including changing the bracing style, and removing the space between "if"/"for" and the following paren? That seems unnecessary, and also goes against https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide. That warrants an r-, IMHO. I don't think you need the "domain=" and "url=" bits in the reporter paths -- it's not consistent with the compartment reporters -- but I'll leave the final decision up to you. I tried it out, here's an example: ├───32,898,728 B (24.14%) -- dom │ └──32,898,728 B (24.14%) -- workers(domain=opera.com) │ └──32,898,728 B (24.14%) -- worker(url=worker.js) │ ├──16,777,216 B (12.31%) -- stack [2] │ ├──16,080,052 B (11.80%) -- compartment(Web Worker) │ │ ├──13,180,928 B (09.67%) -- gc-heap │ │ │ ├──12,800,064 B (09.39%) -- strings [2] │ │ │ ├─────129,664 B (00.10%) -- arena-padding [2] │ │ │ ├──────82,192 B (00.06%) -- arena-unused [2] │ │ │ ├──────77,232 B (00.06%) -- arena-headers [2] │ │ │ ├──────46,720 B (00.03%) -- shapes [2] │ │ │ └──────45,056 B (00.03%) -- objects [2] │ │ ├───2,112,384 B (01.55%) -- object-slots [2] │ │ ├─────384,096 B (00.28%) -- tjit-data │ │ │ ├──296,000 B (00.22%) -- allocators-reserve [2] │ │ │ └───88,096 B (00.06%) -- allocators-main [2] │ │ ├─────262,144 B (00.19%) -- tjit-code [2] │ │ ├─────131,072 B (00.10%) -- mjit-code [2] │ │ ├───────4,624 B (00.00%) -- property-tables [2] │ │ ├───────2,848 B (00.00%) -- mjit-data [2] │ │ ├───────1,940 B (00.00%) -- scripts [2] │ │ └──────────16 B (00.00%) -- string-chars [2] │ └──────41,460 B (00.03%) -- compartment(atoms) │ ├──40,960 B (00.03%) -- gc-heap │ │ ├──32,576 B (00.02%) -- strings [2] │ │ ├───7,872 B (00.01%) -- arena-unused [2] │ │ ├─────272 B (00.00%) -- arena-padding [2] │ │ └─────240 B (00.00%) -- arena-headers [2] │ └─────500 B (00.00%) -- string-chars [2] All those "[2]" suffixes mean that there are duplicate reporters. I guess worker.js actually spawns two workers? I wonder if it's worth adding the worker's address to each "worker(...)" reporter, as is done for system compartments, to separate them out? ::: dom/workers/RuntimeService.cpp @@ +371,5 @@ > + nsRefPtr<WorkerMemoryReporter> reporter = > + new WorkerMemoryReporter(workerPrivate, rt); > + if (NS_FAILED(NS_RegisterMemoryMultiReporter(reporter))) { > + NS_WARNING("Failed to register memory reporter!"); > + reporter = nsnull; We don't check for registration failure anywhere else... you might still want to check, I'll leave it up to you. @@ +380,5 @@ > + if (reporter) { > + if (NS_FAILED(NS_UnregisterMemoryMultiReporter(reporter))) { > + NS_WARNING("Failed to unregister memory reporter!"); > + } > + reporter = nsnull; Ditto for unregistration. ::: dom/workers/WorkerPrivate.cpp @@ +2041,5 @@ > > { > MutexAutoUnlock unlock(mMutex); > > + JSAutoRequest ar(aCx); I don't understand any of the changes in this file. You might want to get someone else to review them.

(In reply to comment #9) > There's still something I don't like, though. A heap of code got moved > around, which is fine... but you also took the opportunity to reformat it, > including changing the bracing style, and removing the space between > "if"/"for" and the following paren? Yeah, that's xpconnect style. As ugly as it is I've been forced to follow it multiple times and since I had to move it all around I figured that I would fix it. Apparently whoever wrote/reviewed this code either didn't know or didn't care about preserving the existing style in this file... Johnny, Blake, do you guys care one way or another? > I don't think you need the "domain=" and "url=" bits in the reporter paths > -- it's not consistent with the compartment reporters -- but I'll leave the > final decision up to you. Ok, I wasn't particularly in love with that either, I'll remove it. > All those "[2]" suffixes mean that there are duplicate reporters. I guess > worker.js actually spawns two workers? I wonder if it's worth adding the > worker's address to each "worker(...)" reporter, as is done for system > compartments, to separate them out? Sure, that sounds nice. > We don't check for registration failure anywhere else... you might still > want to check, I'll leave it up to you. Yeah, I'll leave that. > I don't understand any of the changes in this file. You might want to get > someone else to review them. Oh right, I figured as much. Sorry, I should have told you to skip that.

In bug 674480 I have added an API to JS to get the total number of JS chunks allocated by JSRuntime. That should avoid the need for any custom allocators.

(In reply to comment #10) > (In reply to comment #9) > > There's still something I don't like, though. A heap of code got moved > > around, which is fine... but you also took the opportunity to reformat it, > > including changing the bracing style, and removing the space between > > "if"/"for" and the following paren? > > Yeah, that's xpconnect style. As ugly as it is I've been forced to follow it > multiple times and since I had to move it all around I figured that I would > fix it. Apparently whoever wrote/reviewed this code either didn't know or > didn't care about preserving the existing style in this file... Johnny, > Blake, do you guys care one way or another? Given that the code that got moved around should have used the formatting that Ben made it use now (since that's the style of the surrounding code here), I'd say it's fine to change the formatting here (as much as I hate the formatting too). If I had reviewed the moved code when it went in in the first place and noticed the formatting differences I would've r-'ed based on that.

Comment on attachment 549487 [details] [diff] [review] Patch, v2 Nick, can you take another look now based on comment 13? Johnny, can you review the request changes in RuntimeService.cpp/WorkerPrivate.cpp?

Comment on attachment 549487 [details] [diff] [review] Patch, v2 r=jst for the changes in RuntimeService.cpp and WorkerPrivate.cpp.

Comment on attachment 549487 [details] [diff] [review] Patch, v2 Review of attachment 549487 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, what a horrid style. Sigh. r=me. BTW, avoiding the cut and paste code will pay immediate dividends when I do bug 675136 -- which will tweak the JS-related reporters -- so thanks again for that. ::: dom/workers/RuntimeService.cpp @@ +371,5 @@ > + nsRefPtr<WorkerMemoryReporter> reporter = > + new WorkerMemoryReporter(workerPrivate, rt); > + if (NS_FAILED(NS_RegisterMemoryMultiReporter(reporter))) { > + NS_WARNING("Failed to register memory reporter!"); > + reporter = nsnull; We don't check for registration failure anywhere else... you might still want to check, I'll leave it up to you. @@ +380,5 @@ > + if (reporter) { > + if (NS_FAILED(NS_UnregisterMemoryMultiReporter(reporter))) { > + NS_WARNING("Failed to unregister memory reporter!"); > + } > + reporter = nsnull; Ditto for unregistration. ::: dom/workers/WorkerPrivate.cpp @@ +2041,5 @@ > > { > MutexAutoUnlock unlock(mMutex); > > + JSAutoRequest ar(aCx); I don't understand any of the changes in this file. You might want to get someone else to review them. ::: js/src/xpconnect/src/xpcjsruntime.cpp @@ +1237,5 @@ > + PRInt64 n = 0; > + for(JSScript *script = (JSScript *)c->scripts.next; > + &script->links != &c->scripts; > + script = (JSScript *)script->links.next) > + n += script->totalSize(); Does the xpconnect style forbid braces in this situation? Because they make loops with multi-line headers like this *much* more readable. @@ +1256,5 @@ > + PRInt64 n = 0; > + for(JSScript *script = (JSScript *)c->scripts.next; > + &script->links != &c->scripts; > + script = (JSScript *)script->links.next) > + n += script->jitDataSize(); Same here. @@ +1372,5 @@ > +template <int N> > +inline void > +ReportMemory(const nsACString &path, PRInt32 kind, PRInt32 units, > + PRInt64 amount, const char (&desc)[N], > + nsIMemoryMultiReporterCallback *callback, nsISupports *closure) Does this really need the |template <int N>|? Does |const char *desc| not suffice?

Hmm, Splinter decided to repeat some of my comments from the previous review, but thankfully not all of them.

(In reply to comment #16) > Does the xpconnect style forbid braces in this situation? Because they make > loops with multi-line headers like this *much* more readable. I actually couldn't find any previous examples... So I made it use braces ;) > Does this really need the |template <int N>|? Does |const char *desc| not > suffice? It prevents strlen calls.

(In reply to comment #19) > > > Does this really need the |template <int N>|? Does |const char *desc| not > > suffice? > > It prevents strlen calls. At the cost of having a different instantiation of the function for every string length passed to it? strlen calls don't matter here, but code size always matters. Doesn't seem like the right trade-off to me.

The landed patch still does not account for total workers JS heap, it only reports arenas. I will update that in bug 674480.

(In reply to comment #20) > At the cost of having a different instantiation of the function for every > string length passed to it? strlen calls don't matter here, but code size > always matters. Doesn't seem like the right trade-off to me. Oh, it should be inlined, so this shouldn't really be bloating the code.

(In reply to comment #23) > Oh, it should be inlined, so this shouldn't really be bloating the code. If it would be inlined, then a compiler can (and GCC at least does) optimize strlen(static-string) into a compile-time constant.

(In reply to comment #23) > > Oh, it should be inlined, so this shouldn't really be bloating the code. Ok, good :)

For years now, jst, mrbkap and I among others have been file-wise reforming xpconnect's style. If we start piece-wise within a file reforming it too, maybe it will be messier -- or maybe it'll be reformed sooner. Anyway, separate patch. /be

The memory-reporting portion of xpcjsruntime.cpp has gotten big enough that I've been thinking about splitting it into a separate file. I could fix the formatting at the same time...