Closed Bug 714264 Opened 12 years ago Closed 12 years ago

Move JS memory reporting code and data structures from XPCJSRuntime.cpp to js/src

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
Blocks: 677079
Please get me to review this once the patch is written! :)
But if <https://tbpl.mozilla.org/?tree=Try&rev=2babb7ad32e3> passes, I'll probably attach patches tomorrow.
Attachment #585305 - Flags: review?(n.nethercote)
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.
Attachment #585304 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #585335 - Flags: review?(luke)
Comment on attachment 585335 [details] [diff] [review]
Part a, alternative approach

reasonable
Attachment #585335 - Flags: review?(luke) → review+
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.
Attachment #585307 - Flags: review?(n.nethercote)
(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.
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).
> > 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.
Attachment #585305 - Flags: review?(n.nethercote) → review-
(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 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.
Attachment #585305 - Flags: review- → review+
> > @@ +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.
> - 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.
Attachment #585304 - Attachment is obsolete: true
Attachment #585335 - Flags: checkin+
Attached patch Part b v1Splinter Review
Thanks for the review, addressed review comments.
Attachment #585305 - Attachment is obsolete: true
Attached patch Part c v2Splinter Review
And this.
Attachment #585307 - Attachment is obsolete: true
Attachment #586389 - Flags: review?(n.nethercote)
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!
Attachment #586389 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/b9077aadd3d7
https://hg.mozilla.org/mozilla-central/rev/2e7afd15d01a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.