692277 - Remove js/src from xpconnect LOCAL_INCLUDES

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1.1

With less typos and more compiling.

Comment 3

[Bobby Holley (:bholley)]

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]]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: MamoryApi v1

As requested.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: 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]]

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 [inactive]]

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 [inactive]]

> - 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]]

(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 [inactive]]

> 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]]

(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 [inactive]]

> (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 [inactive unless it's an emergency] (:billm)]

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]]

(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 [inactive]]

MemMetrics.h is good.  MemReport.h is possibly even better!

Comment 17

[Jeff Walden [:Waldo]]

(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 (he/him; ⌚ UTC+1/+2)]

(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 (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: MemoryApi v2

With review comments addressed, in MemoryMetrics.h.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Part b: The rest v2

With review comments addressed.

Comment 21

[Nicholas Nethercote [inactive]]

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 (he/him; ⌚ UTC+1/+2)]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: MemoryApi v2.1

Comment 24

[Nicholas Nethercote [inactive]]

> >--- 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 (he/him; ⌚ UTC+1/+2)]

Shouldn't I return method + regexp + unused, then, and remove the assert?

Comment 26

[Nicholas Nethercote [inactive]]

(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 (he/him; ⌚ UTC+1/+2)]

dmandelin, review ping?

Comment 28

[David Mandelin [:dmandelin]]

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 (he/him; ⌚ UTC+1/+2)]

Attachment: Part a: MemoryApi v2.2

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/53c2fc22835b
https://hg.mozilla.org/mozilla-central/rev/f4d8adba8d74