Closed Bug 890686 Opened 11 years ago Closed 10 years ago

Move intrinsicsHolder to the compartment

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Ms2ger, Unassigned)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
      No description provided.
Attachment #771820 - Flags: review?(till)
Do we need any sort of write barrier here?
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #1)
> Do we need any sort of write barrier here?

<jonco> Ms2ger: looks ok to me
(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #1)

It looks like the intrinsics holder is always created tenured, so we don't need write barrier when we set this.
Comment on attachment 771820 [details] [diff] [review]
Patch v1

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

Great, thank you for doing this!

::: js/src/ion/IonBuilder.cpp
@@ +7075,5 @@
>          return true;
>      }
>  
> +    // inlined not constant not supported, yet.
> +    return abort("NYI inlined not constant get argument element");

What's changed here? I honestly can't tell.

::: js/src/jscompartment.h
@@ +14,5 @@
>  #include "jsgc.h"
>  #include "jsobj.h"
>  
>  #include "gc/Zone.h"
> +#include "js/RootingAPI.h"

Mmh, what is this include needed for?

@@ +351,5 @@
>  
> +  public:
> +    bool initializeIntrinsicsHolder(JSContext *cx, js::Handle<js::GlobalObject*> global);
> +
> +    JSObject *intrinsicsHolder() const {

I think we should be able to use fromMarkedLocation and return a Handle, here. Pre-existing, I know, but would be neat.

::: js/src/vm/GlobalObject.cpp
@@ +379,5 @@
>      if (!JSObject::preventExtensions(cx, throwTypeError))
>          return NULL;
>      self->setThrowTypeError(throwTypeError);
>  
> +    if (!compartment()->initializeIntrinsicsHolder(cx, self))

In theory, I'd prefer for this to happen in JSCompartment::init, I think. However, I'm not sure it would work even now, and it certainly won't work once we land bug 825199 or something similar, it certainly won't. No, this is not an actionable comment, sorry.
Attachment #771820 - Flags: review?(till) → review+
Comment on attachment 771820 [details] [diff] [review]
Patch v1

Removing the r+ for now, as we have to figure out the rooting situation.
Attachment #771820 - Flags: review+
Attachment #771820 - Attachment is obsolete: true
Attached patch intrinsics-gcSplinter Review
Here's a patch to make things work from a GC point of view.

We need to keep the new intrinsics holder alive while the global is alive, but without adding a reference to the holder from the global, because the whole point of this is remove things from the global's slots.

So this adds markIteratively() for JSCompartment which marks the holder if the global is marked.

I didn't really want to add any more iterative marking - maybe there is a better way to do this?
Attachment #785057 - Flags: review?(wmccloskey)
Comment on attachment 785057 [details] [diff] [review]
intrinsics-gc

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

Why are we doing this? If it's just a one-off thing for intrinsics, then this seems fine and I'll r+. If we're planning to move more stuff from the global to the compartment, then I think this is the wrong approach. A better way would be to require global objects to have a trace hook that calls a JSAPI function to trace this stuff. It would be annoying to change all 10 global classes or so, but I think it would be worth it.

::: js/src/jscompartment.cpp
@@ +516,5 @@
> +        /*
> +         * As an optimization, if we know the global is going to be marked, mark
> +         * the intrinsics holder straight away.
> +         */
> +        if (intrinsicsHolder_) {

No braces needed.

@@ +559,5 @@
>          sweepNewTypeObjectTable(newTypeObjects);
>          sweepNewTypeObjectTable(lazyTypeObjects);
>          sweepCallsiteClones();
>  
> +        if (global_ && IsObjectAboutToBeFinalized(global_.unsafeGet())) {

Again, no braces.

@@ +564,3 @@
>              global_ = NULL;
> +        }
> +        if (intrinsicsHolder_ && IsObjectAboutToBeFinalized(intrinsicsHolder_.unsafeGet())) {

Same.

@@ +909,5 @@
>          holder = NewObjectWithClassProto(cx, &JSObject::class_, NULL, global, TenuredObject);
>          if (!holder)
>              return false;
>      }
> +    fprintf(stderr, "init holder for %p\n", this);

Remove.
(In reply to Bill McCloskey (:billm) from comment #8)
> Comment on attachment 785057 [details] [diff] [review]
> intrinsics-gc
> 
> Review of attachment 785057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are we doing this? If it's just a one-off thing for intrinsics, then
> this seems fine and I'll r+. If we're planning to move more stuff from the
> global to the compartment, then I think this is the wrong approach. A better
> way would be to require global objects to have a trace hook that calls a
> JSAPI function to trace this stuff. It would be annoying to change all 10
> global classes or so, but I think it would be worth it.

This is required for the new DOM bindings (see the blocked bug 890684). So no, it's not a one off for the intrinsics holder: we need to move All The Things from slots on the global object to properties (or *something*) on the compartment.

Jonco and I talked about maybe creating a list of these objects on the compartment that can be traced in one go. I don't know if that would be easier than creating properties for all of them.
(In reply to Till Schneidereit [:till] from comment #9)
> This is required for the new DOM bindings (see the blocked bug 890684). So
> no, it's not a one off for the intrinsics holder: we need to move All The
> Things from slots on the global object to properties (or *something*) on the
> compartment.

Bug 890684 doesn't provide much context either :-). Is the idea that the JSClass for the global would be generated? That would make it easy to trace this stuff automatically. I'm sure we'd still have non-IDL-generated globals, though, so we still need a way to deal with those.

> Jonco and I talked about maybe creating a list of these objects on the
> compartment that can be traced in one go. I don't know if that would be
> easier than creating properties for all of them.

That would make it easier to trace these things, but it wouldn't solve the problem of needing iterative tracing.

What are the precise requirements for using WebIDL for globals? Are we not allowed to have any reserved slots on the global? It would make things a lot easier if we could hang *something* off the global, even if we can't fit all the goop we have now.
Flags: needinfo?(bzbarsky)
(In reply to Bill McCloskey (:billm) from comment #10)
> Bug 890684 doesn't provide much context either :-).

Oops, I should've checked. Sorry about that.

> Is the idea that the
> JSClass for the global would be generated? That would make it easy to trace
> this stuff automatically. I'm sure we'd still have non-IDL-generated
> globals, though, so we still need a way to deal with those.

It's unfortunate that we started tackling the topic in this bug, instead of the way more informative bug 760095. 

Basically, for Paris bindings, we want all reserved-slots usage gone from the global object. Given that bz and Waldo already talked about this, I added a needinfo for Waldo, in the hopes that we then have everyone required to figure out a final plan.
Flags: needinfo?(jwalden+bmo)
Bug 760095, which is the bug I originally filed on this, has all the context.

But to summarize, the bindings code currently assumes that it can use the first 3 reserved slots on the object, basically, for things it stores.  To the extent that this is not possible with globals, that code can't work with globals.  See bug 760095, which also has some discussion of possible solutions to the problem.

In fact, even the fact that XPConnect works with globals right now is a total accident: it uses reserved slot 0, which is technically used for canonical proto/constructor stuff, but is unused in practice (because Null does not have a constructor).

It's possible to add more branching in the WebIDL code to handle globals having a different slot layout, but it will be on hot enough paths that it will make the code slower, which I would like to avoid if possible.

Note that in Servo's proposed setup having stuff in the global's slots would not be great either...  but WebIDL certainly doesn't need _everything_ out of slots; it just wants three slots.  At least for now.
Flags: needinfo?(bzbarsky)
mmh, so this is probably a dumb question, but why aren't we just moving all the things in global objects's slots from `JSProto_LIMIT * 3` to `JSProto_LIMIT * 3 + 3`?
We would need to move all the jsprotos by 3 too.  Which I think would be fine, personally...
(In reply to Boris Zbarsky (:bz) from comment #14)
> We would need to move all the jsprotos by 3 too.  Which I think would be
> fine, personally...

That sounds nicer to me. Perhaps we could just clean up the global slots without eliminating them (for example, storing the protos in an array that hangs off the global would simplify the numbering).

Anyway, if we do decide to do the change in this bug, I think it would be better to require every global to call a special trace function when it's traced; that function would mark stuff on the compartment. That seems much nicer and easier to understand than doing weak marking. We have way too much of that as it is.
I don't think we should be imposing extra requirements on what constitutes a valid global JSClass.  We have JSCLASS_GLOBAL_FLAGS right now, and we actually want to get rid of the reserved-slots aspect of that (which actually gets rid of JSCLASS_GLOBAL_FLAGS entirely).  Removing the reserved-slots aspect, but introducing a trace-hook aspect, seems like one step forward, one step backward to me.  Let's not go there.

The system in the original patch, where the compartment assumed responsibility for tracing/marking/etc. all its fields, seems like the right solution to me.  The engine itself would be responsible for calling the compartment trace function at the appropriate times.  The global's trace hook should be responsible for tracing only the stuff defined by the global class -- nothing more, nothing less.

I *think* that addresses the needinfo concerns here, but honestly this bug reads as a bit of a muddle to me, so maybe I didn't get the intent of it right.
Flags: needinfo?(jwalden+bmo)
I don't like just randomly bumping the global slots requirement of globals -- for all JSAPI uses -- just because Gecko wants it.  That's nicer for Gecko than having the zero-global-slots thing it currently has.  But there's no conceptual difference between that setup and the current one.  We're still enforcing some number of reserved slots on all globals, when really we should not be requiring *any* particular number of reserved slots on *any* global.
Sorry, I guess it's not clear that the first patch in this bug won't work. That's why Jon posted the second patch.

The problem is that there's currently no notion of "marking a compartment". Let's pretend that there are a bunch of objects hanging off the compartment instead of off the global. It's hard for the GC to make the decision about when to mark those objects. If we unconditionally mark them, then they'll leak. There are a couple bad options that we could go with here:

1. Stick an extra branch on the GC object marking path that checks if the object is a global and then calls a special mark function on the compartment. I don't want to do this, though, because that path is extremely hot. Checking if the object is a global would cost us couple loads and a branch for every object traced by the GC.

2. We could choose some other criteria for marking the compartment. For example, whenever we mark a BaseShape or JSScript, we could decide that its compartment is live and try to mark it. The marking code for BaseShapes and JSScripts is less hot, so this sorta solves the problem above. However, it would be weird to decide to trace the compartment's builtin objects the first time we hit some arbitrary GC thing in the compartment. The consequence is that GC dumps would show all the builtin compartment objects as being children (i.e., outgoing edges) of some random BaseShape or JSScript. This seems grosser to me than having a few extra reserved slots in globals.

3. We could do what Jon's patch does. Basically, once all GC marking is done, it does another pass through every compartment. If a compartment's global is marked, then we mark any builtin compartment data. This, of course, could cause other marking to happen, so we have to iterate to a fixed point. All this code runs non-incrementally, so it affects our GC pause times pretty badly. It's also rather complicated, and it will get worse as we move more slots out fo the global.

4. We could add an extra trace hook to globals like I suggested. The trace hook would trace the global's compartment.

5. We could reserve a few slots on the global for the DOM. We already do this for proxies, so I don't see what the big deal is.
Assignee: Ms2ger → general
(In reply to PTO Bill McCloskey (:billm) (away until 9/9) from comment #18)
> 3. We could do what Jon's patch does. Basically, once all GC marking is
> done, it does another pass through every compartment. If a compartment's
> global is marked, then we mark any builtin compartment data. This, of
> course, could cause other marking to happen, so we have to iterate to a
> fixed point. All this code runs non-incrementally, so it affects our GC
> pause times pretty badly. It's also rather complicated, and it will get
> worse as we move more slots out fo the global.

Does this need to be done at the end? Couldn't you scan through the compartments at the end of every incremental slice, and mark the intrinsics holders of all marked globals? It doesn't seem like scanning through all the compartments and doing a couple of loads to check the mark bits of each <global,intrinsics> pair would be *too* bad. The actual recursive marking would be done incrementally on the subsequent slices then, wouldn't it?
(In reply to Steve Fink [:sfink] from comment #19)
> Does this need to be done at the end? Couldn't you scan through the
> compartments at the end of every incremental slice, and mark the intrinsics
> holders of all marked globals? It doesn't seem like scanning through all the
> compartments and doing a couple of loads to check the mark bits of each
> <global,intrinsics> pair would be *too* bad. The actual recursive marking
> would be done incrementally on the subsequent slices then, wouldn't it?

I agree that this could work. However, weak marking just seems like a bad way to deal with this problem. Fundamentally, there's no reason why marking a global couldn't mark stuff that hangs off the compartment as well. We just need an extra trace hook. It doesn't seem like that big of a deal to me.
I agree.  I think adding a trace hook to global objects makes sense when viewing JSCompartment as "the global object's private".  Since early CPG, one of the "would be nice" goals was to remove JSCompartment from JSAPI, rename it internally to js::GlobalPrivate, and predominately pass around a GlobalObject*s.  I'd much rather do this than any sort of additional GC phase whose invariants I need to worry about not breaking.  I do not think sacrificing this sensible scheme for the abstract aesthetics of non-Gecko embeddings makes sense.

I do think we should have a reliable assertion to catch mistakes early, though.  Ideally we'd just have a JS_PUBLIC_API JS_GlobalObjectTraceHook and we'd assert clasp->trace == JS_GlobalObjectTraceHook in JS_NewGlobalObject.  This would not be good enough if the embedding already had their own custom trace hook, though.  To address this, we could put a JSTraceOp in CompartmentOptions (which would get called from JS_GlobalObjectTraceHook).  (More generally, I think it would be an improvement if we didn't even take a js::Class argument to JS_NewGlobalObject, but rather the js::Class was fixed internally and all available hooks were passed via CompartmentOptions.  Then, at least the requirements of what hooks you were and were not allowed to override would be clearly delimited instead of what we have now.)
No longer blocks: 890684
Do we even need to do this anymore? I guess not, right?
Only if we really want to.
Comment on attachment 8401489 [details] [diff] [review]
Add JS_GlobalObjectTraceHook, and install it appropriately

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

This looks great! I have one idea that would be nice to have but is entirely optional. It's annoying having these nullptr values littered throughout our JSClasses. I think it would be nice to have definitions like this in jsapi.h:

const FinalizeOpType NullFinalizeOp = nullptr;
const JSNative *NullCallOp = nullptr;
const JSHasInstanceOp *NullHasInstanceOp = nullptr;
const JSNative *NullConstructOp = nullptr;

Then if we use these in place of nullptr in the JSClass definitions, we'll get better type checking (I think?). If you don't want to do this here, though, that's fine.

::: js/src/jsapi.cpp
@@ +2520,5 @@
> +    JS_ASSERT(global->is<GlobalObject>());
> +
> +    JSTraceOp trace = global->compartment()->options().getTrace();
> +    if (trace)
> +        trace(trc, global);

This all looks good. I would suggest adding a JSCompartment::mark(JSTracer*) function that is empty (for now). Then you could call that from here. That way people have a convenient place to hang all their tracing code.

::: js/src/jsapi.h
@@ +2704,5 @@
>                     JS::OnNewGlobalHookOption hookOption,
>                     const JS::CompartmentOptions &options = JS::CompartmentOptions());
>  
>  extern JS_PUBLIC_API(void)
> +JS_GlobalObjectTraceHook(JSTracer *trc, JSObject *global);

We really need a comment here and in CompartmentOptions that explains what's going on.

::: js/src/jsworkers.cpp
@@ +178,5 @@
>      JS_ConvertStub,   nullptr
>  };
>  
> +bool
> +js::IsWorkerGlobalClass(const Class *clasp)

Please remove "const" from here. We had a big problem with "const Shape *" parameters a few years ago, and I've been trying to dissuade people from using const parameters in cases like this.

@@ +317,5 @@
>      JS::CompartmentOptions compartmentOptions(cx->compartment()->options());
>      compartmentOptions.setZone(JS::FreshZone);
>      compartmentOptions.setInvisibleToDebugger(true);
>      compartmentOptions.setMergeable(true);
> +    compartmentOptions.setTrace(nullptr);

This seems unnecessary.

::: js/src/vm/GlobalObject.cpp
@@ +549,5 @@
>  GlobalObject *
>  GlobalObject::create(JSContext *cx, const Class *clasp)
>  {
>      JS_ASSERT(clasp->flags & JSCLASS_IS_GLOBAL);
> +    JS_ASSERT_IF(!IsWorkerGlobalClass(clasp), clasp->trace == JS_GlobalObjectTraceHook);

Do we really need to do anything different for the worker global? Your comment in jsowrkers.cpp doesn't really explain the rationale for it (aside from "worker globals are weird"). I think it might be possible to trace a worker global in between creating an off-thread parse task and actually running it. In that case, we could be creating subtle correctness bugs in the future by not calling the appropriate marking machinery for that compartment.
Attachment #8401489 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/68984d5a66a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.