Last Comment Bug 692277 - Remove js/src from xpconnect LOCAL_INCLUDES
: Remove js/src from xpconnect LOCAL_INCLUDES
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Ms2ger
:
Mentors:
Depends on:
Blocks: 554088
  Show dependency treegraph
 
Reported: 2011-10-05 14:32 PDT by Bill McCloskey (:billm)
Modified: 2012-01-07 07:22 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (31.77 KB, patch)
2011-12-11 10:40 PST, :Ms2ger
no flags Details | Diff | Review
Patch v1.1 (31.80 KB, patch)
2011-12-11 11:32 PST, :Ms2ger
luke: review+
Details | Diff | Review
Part a: MamoryApi v1 (26.95 KB, patch)
2011-12-12 14:19 PST, :Ms2ger
n.nethercote: review-
Details | Diff | Review
Part b: The rest (24.08 KB, patch)
2011-12-12 14:21 PST, :Ms2ger
luke: review+
Details | Diff | Review
Part a: MemoryApi v2 (29.12 KB, patch)
2011-12-13 12:13 PST, :Ms2ger
n.nethercote: review+
Details | Diff | Review
Part b: The rest v2 (24.07 KB, patch)
2011-12-13 12:14 PST, :Ms2ger
no flags Details | Diff | Review
Part a: MemoryApi v2.1 (28.34 KB, patch)
2011-12-14 04:34 PST, :Ms2ger
dmandelin: feedback+
Details | Diff | Review
Part a: MemoryApi v2.2 (31.62 KB, patch)
2011-12-21 10:24 PST, :Ms2ger
n.nethercote: review+
dmandelin: superreview+
Details | Diff | Review

Description Bill McCloskey (:billm) 2011-10-05 14:32:40 PDT
This allows xpconnect to circumvent INSTALLED_HEADERS, so it should be fixed. The main offenders seem to be nsXPConnect.cpp and xpcjsruntime.cpp.

According to mxr, xpconnect is the only offender.
Comment 1 :Ms2ger 2011-12-11 10:40:25 PST
Created attachment 580772 [details] [diff] [review]
Patch v1
Comment 2 :Ms2ger 2011-12-11 11:32:49 PST
Created attachment 580777 [details] [diff] [review]
Patch v1.1

With less typos and more compiling.
Comment 3 Bobby Holley (busy) 2011-12-11 18:39:39 PST
Comment on attachment 580777 [details] [diff] [review]
Patch v1.1

Looking over this patch, I don't think I'm the appropriate reviewer. This patch touches lots of stuff in XPConnect, but fundamentally deals with spidermonkey design decisions that I'm not in a position to authorize.

I think we want an xpconnect-knowledgeable spidermonk here. The best candidates I can think of are luke, jorendorff, gal, and mrbkap. Flagging luke.
Comment 4 Luke Wagner [:luke] 2011-12-12 09:53:58 PST
Comment on attachment 580777 [details] [diff] [review]
Patch v1.1

Nice, thanks!

I'd like to make one stylistic request: it seems like most of the non-trivial touching was adding friend apis for the about:memory xpconnect stuff to call.  Instead of adding this to jsfriendapi.h (which is our "dirty little secrets" header which memory reporting isn't), perhaps we could add a new public header, say js/public/MemoryApi.h, and then put the definitions of, e.g., JS::GetScriptDataSize right next to the definition of JSScript::dataSize (less annoyance and definition-chasing that way).

If you agree, could you make a patch that does just that and r?:njn and sr?dmandelin?  Then my r+ carries for this patch minus that patch.
Comment 5 :Ms2ger 2011-12-12 14:19:59 PST
Created attachment 581051 [details] [diff] [review]
Part a: MamoryApi v1

As requested.
Comment 6 :Ms2ger 2011-12-12 14:21:11 PST
Created attachment 581052 [details] [diff] [review]
Part b: The rest

This is patch v1.1 without the memory stuff, in case anybody wants to see it.
Comment 7 Luke Wagner [:luke] 2011-12-12 14:37:30 PST
Comment on attachment 581052 [details] [diff] [review]
Part b: The rest

Oh, I did see a few things:

>+/**
>+ * If protoKey is not JSProto_Null, then clasp is ignored. If protoKey is

/* , not /**

>+    } else if (JS_GetCompartmentPrincipals(c)) {
>+        if (JS_GetCompartmentPrincipals(c)->codebase) {
>+            name.Assign(JS_GetCompartmentPrincipals(c)->codebase);

  } else if (JSPrincipals *prin = JS_GetCompartmentPrincipals(c)) {
      if (prin->codebase) {
          name.Assign(prin->codebase);
Comment 8 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-12 16:06:45 PST
Comment on attachment 581051 [details] [diff] [review]
Part a: MamoryApi v1

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

This is very much on the right track.  I'm giving an r- because I've done some heavy bikeshedding on the names and I'd like to see a new patch with updated names before landing.  Thanks!

::: js/public/MemoryApi.h
@@ +34,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef js_MemoryApi_h
> +#define js_MemoryApi_h

I don't like the name MemoryApi.h.  "Memory" is too general, and "Api" is redundant for a header in js/public/.

How about MemoryMeasurement.h?  (Or even SizeOf.h?  Maybe that's too short/obscure.)

@@ +52,5 @@
> +    int64 temporary;
> +};
> +
> +extern JS_PUBLIC_API(void)
> +GetTypeInferenceMemoryStats(JSContext *cx, JSCompartment *compartment,

AIUI using a "Get" prefix in SpiderMonkey means that it's fallible.  Also, I've been using SizeOfFoo-style names everywhere else (see https://wiki.mozilla.org/Memory_Reporting).

So, I'd like to suggest the following names:

- SizeOfCompartmentTypeInferenceData
- SizeOfObjectTypeInferenceData
- SizeOfObjectDynamicSlots
- SizeOfCompartmentShapeTable
- SizeOfCompartmentMjitCode (and see the comment below in XPCJSRuntime.cpp about changing this function slightly)
- IsShapeInDictionary
- SizeOfShapePropertyTable
- SizeOfShapeKids
- SizeOfScriptData
- SizeOfScriptJitData
- SystemCompartmentCount / UserCompartmentCount (see comment in jsapi.cpp below)

(BTW, I fully admit that I haven't named things consistently like this within SpiderMonkey, that's because I added all this measurement code to SpiderMonkey before I worked out consistent naming conventions.  Hopefully I'll get around to changing the internal names at some point :)

::: js/src/jsapi.cpp
@@ +921,5 @@
>      rt->data = data;
>  }
>  
> +JS_PUBLIC_API(void)
> +JS::GetSystemCompartmentCount(const JSRuntime *rt, size_t *system, size_t *user)

This function is misnamed, in this form it should just be GetCompartmentCount().

But can you split it into two functions, GetSystemCompartmentCount() and GetUserCompartmentCount()?  We have to make two calls anyway, one for each reporter.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1254,2 @@
>      JS_ASSERT(regexp == 0);     /* this execAlloc is only used for method code */
>      curr->mjitCode = method + unused;

While you're here, can you please move the regexp==0 check and the addition of |method| and |unused| into GetCompartmentSizeOfCode, so it only returns a single number (i.e. |method + unused|).
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-12 16:12:58 PST
> - SystemCompartmentCount / UserCompartmentCount (see comment in jsapi.cpp
> below)
>
> But can you split it into two functions, GetSystemCompartmentCount() and
> GetUserCompartmentCount()?  We have to make two calls anyway, one for each
> reporter.

Oh, I was inconsistent with the suggestions.  The "Get" prefixes aren't necessary.
 
> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ +1254,2 @@
> >      JS_ASSERT(regexp == 0);     /* this execAlloc is only used for method code */
> >      curr->mjitCode = method + unused;
> 
> While you're here, can you please move the regexp==0 check and the addition
> of |method| and |unused| into GetCompartmentSizeOfCode, so it only returns a
> single number (i.e. |method + unused|).

Actually, you can instead move that code into JSCompartment::sizeOfCode() and rename it sizeOfMjitCode().  So you'll end up with these two functions:

  size_t JSCompartment::sizeOfMjitCode() const;
  size_t SizeOfCompartmentMjitCode(const JSCompartment *c);

Thanks!
Comment 10 David Mandelin [:dmandelin] 2011-12-12 16:55:30 PST
(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 581051 [details] [diff] [review]
> Part a: MamoryApi v1
> 
> Review of attachment 581051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is very much on the right track.  I'm giving an r- because I've done
> some heavy bikeshedding on the names and I'd like to see a new patch with
> updated names before landing.  Thanks!
> 
> ::: js/public/MemoryApi.h
> @@ +34,5 @@
> > + *
> > + * ***** END LICENSE BLOCK ***** */
> > +
> > +#ifndef js_MemoryApi_h
> > +#define js_MemoryApi_h
> 
> I don't like the name MemoryApi.h.  "Memory" is too general, and "Api" is
> redundant for a header in js/public/.
> 
> How about MemoryMeasurement.h?  (Or even SizeOf.h?  Maybe that's too
> short/obscure.)

Since they are intended to be public APIs, why not put them in jsapi.h?
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-12 17:21:20 PST
> Since they are intended to be public APIs, why not put them in jsapi.h?

That'd be ok by me, so long as they're all together and clearly marked (with a comment) as all relating to memory measurement.
Comment 12 Luke Wagner [:luke] 2011-12-12 17:51:38 PST
(In reply to David Mandelin from comment #10)
> Since they are intended to be public APIs, why not put them in jsapi.h?

That was my recommendation.  The reason is that these are functions that are (1) not dirty internal details we intend to remove, so belong in js/public (and not jsfriendapi.h) (2) very likely to change and thus probably not good to make a stable API commitment, so not jsapi.h material.
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-12 17:56:34 PST
> (2) very likely to change and thus probably not good
> to make a stable API commitment, so not jsapi.h material.

Good point -- I have some changes in mind already (but no code written).
Comment 14 Bill McCloskey (:billm) 2011-12-12 18:02:51 PST
Long term, it would really be nice if we could move the data collection side of the memory reporters into the JS engine. I think that's where that code belongs.
Comment 15 David Mandelin [:dmandelin] 2011-12-12 18:36:16 PST
(In reply to Nicholas Nethercote [:njn] from comment #13)
> > (2) very likely to change and thus probably not good
> > to make a stable API commitment, so not jsapi.h material.
> 
> Good point -- I have some changes in mind already (but no code written).

OK. How about "MemMetrics.h" or "MemoryMetrics.h" as the header file name? ("MemoryMeasurement.h" seems long to me.) Or maybe even "AboutMemory.h", since that's really what it's for?
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-12 18:41:24 PST
MemMetrics.h is good.  MemReport.h is possibly even better!
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-12 18:43:11 PST
(In reply to David Mandelin from comment #15)
> OK. How about "MemMetrics.h" or "MemoryMetrics.h" as the header file name?
> ("MemoryMeasurement.h" seems long to me.) Or maybe even "AboutMemory.h",
> since that's really what it's for?

Characters are cheap.  I wouldn't go for "Mem" over "Memory", myself.
Comment 18 :Ms2ger 2011-12-12 23:16:51 PST
(In reply to Nicholas Nethercote [:njn] from comment #8)
> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ +1254,2 @@
> >      JS_ASSERT(regexp == 0);     /* this execAlloc is only used for method code */
> >      curr->mjitCode = method + unused;
> 
> While you're here, can you please move the regexp==0 check and the addition
> of |method| and |unused| into GetCompartmentSizeOfCode, so it only returns a
> single number (i.e. |method + unused|).

It wasn't clear to me that the assert holds for other potential callers, but I guess there aren't any at this point.

I'll paint the bikeshed in whatever colour you've all decided on by the time I get home tonight :)
Comment 19 :Ms2ger 2011-12-13 12:13:20 PST
Created attachment 581365 [details] [diff] [review]
Part a: MemoryApi v2

With review comments addressed, in MemoryMetrics.h.
Comment 20 :Ms2ger 2011-12-13 12:14:17 PST
Created attachment 581366 [details] [diff] [review]
Part b: The rest v2

With review comments addressed.
Comment 21 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-13 16:50:50 PST
Comment on attachment 581365 [details] [diff] [review]
Part a: MemoryApi v2

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

Looks great!  Thanks very much.

::: js/public/MemoryMetrics.h
@@ +33,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef js_MemoryMetrics_h

Can you add a comment here, something like:  "These declarations are not within jsapi.h because they are highly likely to change in the future.  Depend on them at your own risk."
Comment 22 :Ms2ger 2011-12-14 04:18:18 PST
Comment on attachment 581365 [details] [diff] [review]
Part a: MemoryApi v2

>--- a/js/src/jscompartment.h
>+++ b/js/src/jscompartment.h
>-    void sizeOfCode(size_t *method, size_t *regexp, size_t *unused) const;

This is used in the shell...
Comment 23 :Ms2ger 2011-12-14 04:34:51 PST
Created attachment 581592 [details] [diff] [review]
Part a: MemoryApi v2.1
Comment 24 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-14 15:39:39 PST
> >--- a/js/src/jscompartment.h
> >+++ b/js/src/jscompartment.h
> >-    void sizeOfCode(size_t *method, size_t *regexp, size_t *unused) const;
> 
> This is used in the shell...

Just keep the old version of your patch and change the shell to call JSCompartment::sizeOfMjitCode().  r=me on the old patch with that.
Comment 25 :Ms2ger 2011-12-15 11:06:14 PST
Shouldn't I return method + regexp + unused, then, and remove the assert?
Comment 26 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-12-15 14:44:07 PST
(In reply to Ms2ger from comment #25)
> Shouldn't I return method + regexp + unused, then, and remove the assert?

No.  The execAlloc in JSCompartment only allocates mjit code.  The execAlloc in ThreadData only allocates regexp code.  So the assertion is a good thing here :)
Comment 27 :Ms2ger 2011-12-19 10:13:41 PST
dmandelin, review ping?
Comment 28 David Mandelin [:dmandelin] 2011-12-21 10:00:53 PST
Comment on attachment 581592 [details] [diff] [review]
Part a: MemoryApi v2.1

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

I'll sr it when it has been r+'d. It looks good to me, though.
Comment 29 :Ms2ger 2011-12-21 10:24:16 PST
Created attachment 583546 [details] [diff] [review]
Part a: MemoryApi v2.2

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