Last Comment Bug 674721 - Add memory reporting for web worker data
: Add memory reporting for web worker data
Status: RESOLVED FIXED
[MemShrink], [inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
: 675578 (view as bug list)
Depends on:
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2011-07-27 14:54 PDT by Alon Zakai (:azakai)
Modified: 2011-08-03 00:01 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
example (335 bytes, application/octet-stream)
2011-07-27 14:54 PDT, Alon Zakai (:azakai)
no flags Details
Patch, v1 (30.80 KB, patch)
2011-07-28 19:35 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
n.nethercote: review-
Details | Diff | Review
Patch, v2 (48.47 KB, patch)
2011-07-29 15:23 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
n.nethercote: review+
jst: review+
Details | Diff | Review

Description Alon Zakai (:azakai) 2011-07-27 14:54:57 PDT
Created attachment 548928 [details]
example

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.
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-27 15:22:42 PDT
Weird... How does about:memory even know about my non-main JSRuntime?
Comment 2 Justin Lebar (not reading bugmail) 2011-07-27 15:31:30 PDT
(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.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-27 15:51:04 PDT
Yeah, jemalloc knows how much memory it has allocated.  So about:memory doesn't actually know about the non-main JSRuntime at all.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-27 15:58:46 PDT
Hm, ok. Are our memory reporters threadsafe?
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-27 16:11:36 PDT
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.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-28 19:35:28 PDT
Created attachment 549290 [details] [diff] [review]
Patch, v1

This mimics XPConnect, I consolidated as much code as I could without making custom chunk allocators for workers (ick!).
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-28 21:44:30 PDT
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?
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-29 15:23:31 PDT
Created attachment 549487 [details] [diff] [review]
Patch, v2

Ok, this is much more consolidated. Look better?
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-31 23:30:45 PDT
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.
Comment 10 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-01 08:21:00 PDT
(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.
Comment 11 Igor Bukanov 2011-08-01 08:31:06 PDT
*** Bug 675578 has been marked as a duplicate of this bug. ***
Comment 12 Igor Bukanov 2011-08-01 08:53:03 PDT
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.
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2011-08-01 10:07:37 PDT
(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 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-01 10:11:31 PDT
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 15 Johnny Stenback (:jst, jst@mozilla.com) 2011-08-01 13:21:30 PDT
Comment on attachment 549487 [details] [diff] [review]
Patch, v2

r=jst for the changes in RuntimeService.cpp and WorkerPrivate.cpp.
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-01 17:51:42 PDT
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?
Comment 17 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-01 18:05:44 PDT
Hmm, Splinter decided to repeat some of my comments from the previous review, but thankfully not all of them.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-01 21:10:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/baa3a2a2cea4
Comment 19 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-01 21:11:32 PDT
(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.
Comment 20 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-01 22:05:31 PDT
(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.
Comment 21 Igor Bukanov 2011-08-02 01:39:55 PDT
The landed patch still does not account for total workers JS heap, it only reports arenas. I will update that in bug 674480.
Comment 22 Marco Bonardo [::mak] 2011-08-02 03:45:46 PDT
http://hg.mozilla.org/mozilla-central/rev/baa3a2a2cea4
Comment 23 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-02 08:20:09 PDT
(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.
Comment 24 Igor Bukanov 2011-08-02 08:31:33 PDT
(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.
Comment 25 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-02 16:39:07 PDT
(In reply to comment #23)
> 
> Oh, it should be inlined, so this shouldn't really be bloating the code.

Ok, good :)
Comment 26 Brendan Eich [:brendan] 2011-08-02 18:15:19 PDT
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
Comment 27 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-02 19:04:58 PDT
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...

Note You need to log in before you can comment on or make changes to this bug.