Remove js/src from xpconnect LOCAL_INCLUDES

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: billm, Assigned: Ms2ger)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

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.
Posted patch Patch v1 (obsolete) — Splinter Review
Assignee: general → Ms2ger
Status: NEW → ASSIGNED
Attachment #580772 - Flags: review?(bobbyholley+bmo)
Posted patch Patch v1.1 (obsolete) — Splinter Review
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 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+
Posted patch Part a: MamoryApi v1 (obsolete) — Splinter Review
As requested.
Attachment #581051 - Flags: superreview?(dmandelin)
Attachment #581051 - Flags: review?(nnethercote)
Posted patch Part b: The rest (obsolete) — Splinter Review
This is patch v1.1 without the memory stuff, in case anybody wants to see it.
Attachment #580777 - Attachment is obsolete: true
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.
(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).
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.
(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
Posted patch Part a: MemoryApi v2 (obsolete) — Splinter Review
With review comments addressed, in MemoryMetrics.h.
Attachment #581051 - Attachment is obsolete: true
Attachment #581365 - Flags: superreview?(dmandelin)
Attachment #581365 - Flags: review?(nnethercote)
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+
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...
Posted patch Part a: MemoryApi v2.1 (obsolete) — Splinter Review
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)
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 :)
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+
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+
https://hg.mozilla.org/mozilla-central/rev/53c2fc22835b
https://hg.mozilla.org/mozilla-central/rev/f4d8adba8d74
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Blocks: 554088
You need to log in before you can comment on or make changes to this bug.