Remove js/src from xpconnect LOCAL_INCLUDES

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: billm, Assigned: Ms2ger)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 580772 [details] [diff] [review]
Patch v1
Assignee: general → Ms2ger
Status: NEW → ASSIGNED
Attachment #580772 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 580777 [details] [diff] [review]
Patch v1.1

With less typos and more compiling.
Attachment #580772 - Attachment is obsolete: true
Attachment #580772 - Flags: review?(bobbyholley+bmo)
Attachment #580777 - Flags: review?(bobbyholley+bmo)
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.
Attachment #580777 - Flags: review?(bobbyholley+bmo) → review?(luke)

Comment 4

5 years ago
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.
Attachment #580777 - Flags: review?(luke) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 581051 [details] [diff] [review]
Part a: MamoryApi v1

As requested.
Attachment #581051 - Flags: superreview?(dmandelin)
Attachment #581051 - Flags: review?(nnethercote)
(Assignee)

Comment 6

5 years ago
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.
Attachment #580777 - Attachment is obsolete: true

Comment 7

5 years ago
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);
Attachment #581052 - Flags: review+
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|).
Attachment #581051 - Flags: review?(nnethercote) → review-
> - 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!
(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?
Attachment #581051 - Flags: superreview?(dmandelin)
> 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

5 years ago
(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.
> (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).
(Reporter)

Comment 14

5 years ago
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.
(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?
MemMetrics.h is good.  MemReport.h is possibly even better!
(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.
(Assignee)

Comment 18

5 years ago
(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 :)
Version: unspecified → Trunk
(Assignee)

Comment 19

5 years ago
Created attachment 581365 [details] [diff] [review]
Part a: MemoryApi v2

With review comments addressed, in MemoryMetrics.h.
Attachment #581051 - Attachment is obsolete: true
Attachment #581365 - Flags: superreview?(dmandelin)
Attachment #581365 - Flags: review?(nnethercote)
(Assignee)

Comment 20

5 years ago
Created attachment 581366 [details] [diff] [review]
Part b: The rest v2

With review comments addressed.
Attachment #581052 - Attachment is obsolete: true
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."
Attachment #581365 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 22

5 years ago
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...
(Assignee)

Comment 23

5 years ago
Created attachment 581592 [details] [diff] [review]
Part a: MemoryApi v2.1
Attachment #581365 - Attachment is obsolete: true
Attachment #581365 - Flags: superreview?(dmandelin)
Attachment #581592 - Flags: superreview?(dmandelin)
Attachment #581592 - Flags: review?(nnethercote)
> >--- 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.
Attachment #581592 - Flags: review?(nnethercote)
(Assignee)

Comment 25

5 years ago
Shouldn't I return method + regexp + unused, then, and remove the assert?
(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 :)
(Assignee)

Comment 27

5 years ago
dmandelin, review ping?
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.
Attachment #581592 - Flags: superreview?(dmandelin) → feedback+
(Assignee)

Comment 29

5 years ago
Created attachment 583546 [details] [diff] [review]
Part a: MemoryApi v2.2
Attachment #581592 - Attachment is obsolete: true
Attachment #583546 - Flags: superreview?(dmandelin)
Attachment #583546 - Flags: review+
Attachment #583546 - Flags: review+ → review?(n.nethercote)
Attachment #583546 - Flags: review?(n.nethercote) → review+
Attachment #583546 - Flags: superreview?(dmandelin) → superreview+
(Assignee)

Comment 30

5 years ago
https://hg.mozilla.org/mozilla-central/rev/53c2fc22835b
https://hg.mozilla.org/mozilla-central/rev/f4d8adba8d74
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 years ago
Blocks: 554088
You need to log in before you can comment on or make changes to this bug.