Last Comment Bug 714264 - Move JS memory reporting code and data structures from XPCJSRuntime.cpp to js/src
: Move JS memory reporting code and data structures from XPCJSRuntime.cpp to js...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks: 677079
  Show dependency treegraph
 
Reported: 2011-12-30 05:28 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-01-11 05:01 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Make js::Vector's second and third template parameter optional in js/Vector.h (2.74 KB, patch)
2012-01-02 10:30 PST, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review-
Details | Diff | Splinter Review
Part b: Move CompartmentStats to MemoryMetrics.h (15.79 KB, patch)
2012-01-02 10:31 PST, :Ms2ger (⌚ UTC+1/+2)
n.nethercote: review+
Details | Diff | Splinter Review
Part c: Move IterateData / CollectCompartmentStatsForRuntime / GetExplicitNonHeapForRuntime to js/MemoryMetrics.h (48.97 KB, patch)
2012-01-02 10:32 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Part a, alternative approach (2.75 KB, patch)
2012-01-02 14:47 PST, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Ms2ger: checkin+
Details | Diff | Splinter Review
Part b v1 (28.67 KB, patch)
2012-01-06 05:00 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Part c v2 (50.87 KB, patch)
2012-01-06 05:01 PST, :Ms2ger (⌚ UTC+1/+2)
n.nethercote: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-12-30 05:28:45 PST
This code needs to touch a lot of js-private stuff, so it would better live in js/src. That also allows us to remove a number of the APIs in MemoryMetrics.h.
Comment 1 Bill McCloskey (:billm) 2011-12-30 09:48:36 PST
Yay! This is a great plan.
Comment 2 Nicholas Nethercote [:njn] 2011-12-30 14:17:35 PST
Please get me to review this once the patch is written! :)
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-12-31 04:31:32 PST
It is written, but failing to build on windows (<https://bitbucket.org/ms2ger/spidermonkey-patches/src> / <https://tbpl.mozilla.org/?tree=Try&rev=8c00dd5daf8e>)
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-12-31 09:20:44 PST
But if <https://tbpl.mozilla.org/?tree=Try&rev=2babb7ad32e3> passes, I'll probably attach patches tomorrow.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-01-02 10:30:23 PST
Created attachment 585304 [details] [diff] [review]
Part a: Make js::Vector's second and third template parameter optional in js/Vector.h
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-01-02 10:31:06 PST
Created attachment 585305 [details] [diff] [review]
Part b: Move CompartmentStats to MemoryMetrics.h
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-01-02 10:32:21 PST
Created attachment 585307 [details] [diff] [review]
Part c: Move IterateData / CollectCompartmentStatsForRuntime / GetExplicitNonHeapForRuntime to js/MemoryMetrics.h
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-02 14:40:40 PST
Comment on attachment 585304 [details] [diff] [review]
Part a: Make js::Vector's second and third template parameter optional in js/Vector.h

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

jspubtd.h doesn't currently expose namespace js or anything inside it.  I think that's intentional.  I don't think we want to change that.

Moreover, jspubtd.h and jsprvtd.h are a bad idiom.  Rather than concentrate many unrelated types in one massive super-header, we should be separating types into multiple headers, so that a change to one type doesn't trigger a rebuild of the full set of files (possibly extending into the browser, depending how friendly the types are).

I'm not sure having Vector in jsprvtd.h actually helps at all anyway -- you need Vector.h to have a useful Vector.  So really, people who need Vector should just include Vector.h.

I went ahead and wrote up a patch along these lines (sans removing Vector from jsprvtd.h, because Vector's so widely used that it's hard to easily disentangle it) to be sure it was possible, so I guess I should take this.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-02 14:47:48 PST
Created attachment 585335 [details] [diff] [review]
Part a, alternative approach

This puts the canonical declarations of Vector (which has the default-argument problem noted here) and HashMap (which has it as well, albeit not noted here) and InlineMap (perhaps the most similar other class declared in jsprvtd.h) in their respective headers.

I'm more than happy to move these includes into the files that want them, if required.  From the standpoint of avoiding the problem that motivated this bug, however, this is an adequate interim fix.
Comment 10 Luke Wagner [:luke] 2012-01-02 15:12:14 PST
Comment on attachment 585335 [details] [diff] [review]
Part a, alternative approach

reasonable
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-02 17:05:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3e0e48bb8c for attachment 585335 [details] [diff] [review].  This bug should remain open for subsequent work, of course.
Comment 12 Nicholas Nethercote [:njn] 2012-01-02 22:39:40 PST
Comment on attachment 585307 [details] [diff] [review]
Part c: Move IterateData / CollectCompartmentStatsForRuntime / GetExplicitNonHeapForRuntime to js/MemoryMetrics.h

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

I'll wait to see what the revised part b looks like before giving this patch r+ or r-.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ -82,3 @@
>          mWatchdogWakeup = JS_NEW_CONDVAR(mJSRuntime->gcLock);
>          mJSRuntime->setActivityCallback(ActivityCallback, this);
>  #endif

This is the same garbled hunk you had in the other patch, AFAICT.

@@ +1775,5 @@
>  
> +        data.xpconnect +=
> +            xpcrt->SizeOfIncludingThis(xpc::JsMallocSizeOf);
> +        data.xpconnect +=
> +            XPCWrappedNativeScope::SizeOfAllScopesIncludingThis(xpc::JsMallocSizeOf);

Why did you move this out of CollectCompartmentStatsForRuntime?  Now .xpconnect won't be determined for worker threads.  Please put it back.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-01-03 01:54:09 PST
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Comment on attachment 585307 [details] [diff] [review]
> Part c: Move IterateData / CollectCompartmentStatsForRuntime /
> GetExplicitNonHeapForRuntime to js/MemoryMetrics.h
> 
> Review of attachment 585307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll wait to see what the revised part b looks like before giving this patch
> r+ or r-.

Which revised part b?

> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ -82,3 @@
> >          mWatchdogWakeup = JS_NEW_CONDVAR(mJSRuntime->gcLock);
> >          mJSRuntime->setActivityCallback(ActivityCallback, this);
> >  #endif
> 
> This is the same garbled hunk you had in the other patch, AFAICT.

That's the list of code that depends on jscntxt.h, yes.

> @@ +1775,5 @@
> >  
> > +        data.xpconnect +=
> > +            xpcrt->SizeOfIncludingThis(xpc::JsMallocSizeOf);
> > +        data.xpconnect +=
> > +            XPCWrappedNativeScope::SizeOfAllScopesIncludingThis(xpc::JsMallocSizeOf);
> 
> Why did you move this out of CollectCompartmentStatsForRuntime?  Now
> .xpconnect won't be determined for worker threads.  Please put it back.

Because I can't access that from js/src.
Comment 14 Marco Bonardo [::mak] 2012-01-03 03:46:00 PST
first part merged https://hg.mozilla.org/mozilla-central/rev/5c3e0e48bb8c
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 07:02:36 PST
Regarding the first part, I had to remove the ds/InlineMap.h bit, because that header's not exported in browsers, apparently.  The rest was fine (after I #ifdef __cplusplus'd the hashtable/vector includes).
Comment 16 Nicholas Nethercote [:njn] 2012-01-03 17:07:10 PST
> > I'll wait to see what the revised part b looks like before giving this patch
> > r+ or r-.
> 
> Which revised part b?

Ugh, I reviewed it but the review disappeared somehow, sorry.

My main comment was that the void* stuff involving the compartment name was yucky, and it would be much nicer if GetCompartmentName() used a JSString and lived within the JS engine, and I wanted to see a new version of the patch that did that.
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-01-04 01:21:37 PST
(In reply to Nicholas Nethercote [:njn] from comment #16)
> > > I'll wait to see what the revised part b looks like before giving this patch
> > > r+ or r-.
> > 
> > Which revised part b?
> 
> Ugh, I reviewed it but the review disappeared somehow, sorry.
> 
> My main comment was that the void* stuff involving the compartment name was
> yucky, and it would be much nicer if GetCompartmentName() used a JSString
> and lived within the JS engine, and I wanted to see a new version of the
> patch that did that.

I tried that, but I gave up when I realized that the JSString would be of no use to anybody, and we'd end up doing UTF16 -> UTF8 conversion in MakeMemoryReporterPath, and then UTF8 -> UTF16 conversion when we pass it back into JS.

Also, AFAICT, the only way to print a 64-bit integer into a JSString is JS_printf'ing it, and then copying it into a StringBuffer. We also don't really have an easy way to report allocation failure here.
Comment 18 Nicholas Nethercote [:njn] 2012-01-05 15:46:14 PST
Comment on attachment 585305 [details] [diff] [review]
Part b: Move CompartmentStats to MemoryMetrics.h

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

r=me with the nits below fixed.  Thanks.

::: js/public/MemoryMetrics.h
@@ +79,5 @@
> +        destroyNameCb(name);
> +    }
> +
> +    void *name;
> +    DestroyNameCallback destroyNameCb;

I was totally puzzled for a while as to why you used a void* for the name. Eventually I worked out it's because the JS engine can't see nsCString. (And that's also why you need DestroyNameCallback.) A comment here briefly explaining this would help a lot.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ -81,4 @@
>  CollectCompartmentStatsForRuntime
>          mWatchdogWakeup = JS_NEW_CONDVAR(mJSRuntime->gcLock);
>          mJSRuntime->setActivityCallback(ActivityCallback, this);
>  #endif

This hunk looks garbled, I can't see where it matches the file.  Should it even be there?

@@ +1510,4 @@
>                         const char (&reporterName)[N])
>  {
> +  return pathPrefix + NS_LITERAL_CSTRING("compartment(") +
> +         *static_cast<nsCString*>(compartmentName) +

Can you pass in the whole CompartmentStats instead of compartmentName, and use stats.name (with the static_cast) here?  That way the void* access is encapsulated, i.e. not visible in the function signature.
Comment 19 Nicholas Nethercote [:njn] 2012-01-05 16:12:21 PST
> > @@ +1775,5 @@
> > >  
> > > +        data.xpconnect +=
> > > +            xpcrt->SizeOfIncludingThis(xpc::JsMallocSizeOf);
> > > +        data.xpconnect +=
> > > +            XPCWrappedNativeScope::SizeOfAllScopesIncludingThis(xpc::JsMallocSizeOf);
> > 
> > Why did you move this out of CollectCompartmentStatsForRuntime?  Now
> > .xpconnect won't be determined for worker threads.  Please put it back.
> 
> Because I can't access that from js/src.

Sorry I asked you to put it back -- you've found a bug in the existing code!  It turns out there is only ever one XPCJSRuntime, and I'm re-measuring it for every JSRuntime (of which there can be multiple).

Would you mind fixing this in this patch?  (I could do it in a separate bug, but then I'd just end up causing you conflicts.)  What you need to do is:

- Remove the |xpconnect| field from |IterateData|.

- Move the xpconnect measuring code (quoted at the top of this comment) into XPConnectJSCompartmentsMultiReporter::CollectReports(), just after the call to CollectCompartmentStatsForRuntime(), i.e. where you put it in your patch, but the result can go into a local variable.

- Move the ReportMemoryBytes(... data.xpconnect ...) call also into CollectReports(), just after the call to ReportJSRuntimeStats.

Hope that makes sense.  I'll review again when you've updated the patch.
Comment 20 Nicholas Nethercote [:njn] 2012-01-05 22:10:59 PST
> - Move the xpconnect measuring code (quoted at the top of this comment) into
> XPConnectJSCompartmentsMultiReporter::CollectReports(), just after the call
> to CollectCompartmentStatsForRuntime(), i.e. where you put it in your patch,
> but the result can go into a local variable.

Oh, I guess that needs to be put into a scope that has |JSAutoRequest ar(cx);| at the start.  Well, I'm not certain, but it currently is within such a scope so I'd rather play it safe.
Comment 21 :Ms2ger (⌚ UTC+1/+2) 2012-01-06 05:00:12 PST
Created attachment 586388 [details] [diff] [review]
Part b v1

Thanks for the review, addressed review comments.
Comment 22 :Ms2ger (⌚ UTC+1/+2) 2012-01-06 05:01:22 PST
Created attachment 586389 [details] [diff] [review]
Part c v2

And this.
Comment 23 Nicholas Nethercote [:njn] 2012-01-07 02:35:42 PST
Comment on attachment 586389 [details] [diff] [review]
Part c v2

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

Looks good.  Thanks for fixing the xpconnect double-counting bug!

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