692267 - Remove jsgc.h from INSTALLED_HEADERS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

This is the next logical step. The big question is whether we need to use an inline function to get an object's compartment. Hopefully we don't. I'm going to do a Talos run to find out. Otherwise, we'll have to expose some jsgc.h constants in jsfriendapi.h.

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

How do you deal with the dependence of XPCJSRuntime::TraceJS on GCMarker?

Comment 2

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

You're right, I keep forgetting that xpconnect can circumvent INSTALLED_HEADERS. I guess we'll have to fix that once we've worked out everything else. Right now it looks like nsXPConnect.cpp and xpcjsruntime.cpp are using headers they aren't supposed to.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

The dependence of XPCJSRuntime::TraceJS at least will be fixable by splitting XPConnect's black and gray tracing.

Comment 4

[Luke Wagner [:luke]]

This is one of the few remaining big exposed privates.  Uninstalling jsgc.h would also free up jsgcchunk, jsgcstats, and ds/BitArray.

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Looks like it can be removed from nsNodeUtils without any changes, oddly enough.

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Ah, unless that imports xpcpublic or something...

Comment 7

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

I'm removing the dependency on bug 699230. For the purposes of this bug, it doesn't really matter if getting an object's compartment is in jsapi.h or jsfriendapi.h. All that matters is whether it needs to be an inline function or not (i.e., would we regress performance by taking a function call).

If it does have to be an inline function, then we'll have to lift some bitmasks from jsgc.h into jsfriendapi.h, which is nasty.

Either way, this bug involves comparing some SunSpider/V8/Kraken/Dromaeo/Talos runs and then adding a little code to jsfriendapi.

Comment 8

[Luke Wagner [:luke]]

Also, I think, jscell, gc/Barrier, and gc/Statistics.  With this and bug 692269, there is only jsatom.h and then I think the end is in sight.

Comment 9

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: remove some stuff

This removes some of the stuff, mostly by specializing the interface that the CC uses to only deal with gray, which is what it cares about.  It doesn't deal with GetObjectCompartment or whatever.  Instead I just punt by defining it.  I don't know that I want to work this to completion, but here's a start if anybody else is interested.

Comment 10

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

Where is CellCompartment defined?

Comment 11

[Andrew McCreight (out of office until 8/21) [:mccr8]]

It isn't.  That's the issue on comment 0 I said I wasn't dealing with. ;)

Comment 12

[Terrence Cole [:terrence]]

I had a tilt at this last night and it's looking increasingly like a windmill.  I think we are down to needing 4 symbols:

GetObjectCompartment
js_GetGCThingTraceKind
IterateGrayObjects
ChunkSize

ChunkSize is not in a fast path and can get wrapped in GetChunkSize.  IterateGrayObjects is already a call and can just get moved.  This just leaves the Cell layout wrappers. Bill, did you get around to testing this?  If so, what were the results?

If these do look performance sensitive, I don't think it would overly burdensome at this point to just shadow the operations in gc/Heap.h with hardcoded versions (with appropriately dire comments to the various shadowed bits, etc).

Comment 13

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

Attachment: create js/HeapAPI.h header file

I've been thinking about this, and I think it's inevitable that we'll need to expose enough guts to get an object's compartment via an inline API function. We're probably going to want to expose inline functions for UnmarkGray-related stuff as well (bug 747066).

This first patch creates a new, public header file called js/HeapAPI.h. Initially it just includes gc/Heap.h. Most of the stuff that people use from jsgc.h is actually in gc/Heap.h, but a few things had to be moved out of jsgc.h.

Comment 14

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

Attachment: eliminate gc/Heap.h dependency

This patch moves a few things out of gc/Heap.h into js/HeapAPI.h. I used the JS namespace for the Get{Object,GCThing}Compartment functions, since I think that makes more sense. However, all the uses are still via js::GetObjectCompartment. This works because the js namespace automatically imports everything from JS. Eventually we'll want to change the uses to JS::, but that can be done gradually over time.

Comment 15

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

Comment on attachment 673621 [details] [diff] [review]
eliminate gc/Heap.h dependency

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

::: js/public/HeapAPI.h
@@ +16,5 @@
> + * are forced to support each platform with non-4096 pages as a special case.
> + * Note: The freelist supports a maximum arena shift of 15.
> + * Note: Do not use JS_CPU_SPARC here, this header is used outside JS.
> + * Bug 692267: Move page size definition to gc/Memory.h and include it
> + *             directly once jsgc.h is no longer an installed header.

Hey, that's this bug! :)

Comment 16

[Terrence Cole [:terrence]]

Comment on attachment 673620 [details] [diff] [review]
create js/HeapAPI.h header file

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

Woot!  \o/

Comment 17

[Terrence Cole [:terrence]]

Comment on attachment 673621 [details] [diff] [review]
eliminate gc/Heap.h dependency

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

::: js/public/HeapAPI.h
@@ +16,5 @@
> + * are forced to support each platform with non-4096 pages as a special case.
> + * Note: The freelist supports a maximum arena shift of 15.
> + * Note: Do not use JS_CPU_SPARC here, this header is used outside JS.
> + * Bug 692267: Move page size definition to gc/Memory.h and include it
> + *             directly once jsgc.h is no longer an installed header.

My original plan was a bit less awesome than this. We could move this to gc/Memory.h, but then we'd have to expose that too.

I think we should just remove the note.

@@ +47,5 @@
> +
> +struct ArenaHeader
> +{
> +    JSCompartment *compartment;
> +};

Nice!

::: js/src/Makefile.in
@@ -185,5 @@
>  		BitArray.h \
>  		$(NULL)
>  
>  EXPORTS_gc = \
> -		Heap.h \

\o/

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/21a142b1a9d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fa2fe222ef

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21a142b1a9d8
https://hg.mozilla.org/mozilla-central/rev/05fa2fe222ef