Closed Bug 692267 Opened 8 years ago Closed 7 years ago

Remove jsgc.h from INSTALLED_HEADERS

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
How do you deal with the dependence of XPCJSRuntime::TraceJS on GCMarker?
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
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.
The dependence of XPCJSRuntime::TraceJS at least will be fixable by splitting XPConnect's black and gray tracing.
Depends on: 699230
Blocks: 554088
This is one of the few remaining big exposed privates.  Uninstalling jsgc.h would also free up jsgcchunk, jsgcstats, and ds/BitArray.
Looks like it can be removed from nsNodeUtils without any changes, oddly enough.
Ah, unless that imports xpcpublic or something...
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.
No longer depends on: 699230
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.
Attached patch remove some stuff (obsolete) — Splinter Review
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.
Where is CellCompartment defined?
It isn't.  That's the issue on comment 0 I said I wasn't dealing with. ;)
Depends on: 724398
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).
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.
Assignee: general → wmccloskey
Status: NEW → ASSIGNED
Attachment #673620 - Flags: review?(terrence)
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.
Attachment #673621 - Flags: review?(terrence)
Attachment #592599 - Attachment is obsolete: true
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 on attachment 673620 [details] [diff] [review]
create js/HeapAPI.h header file

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

Woot!  \o/
Attachment #673620 - Flags: review?(terrence) → review+
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/
Attachment #673621 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/21a142b1a9d8
https://hg.mozilla.org/mozilla-central/rev/05fa2fe222ef
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.