885758 - Add ExclusiveContext class for off main thread contexts with exclusive access to their compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch (0dd5a67fd870)

Several operations we would like to start doing off the main thread - JS parsing, object serialization / deserialization --- need to be able to modify the VM but can be isolated to a specific compartment which the main thread will not need to enter while the operation is in progress.  This represents a different set of capabilities than JSContext (exclusive access to almost everything in the runtime) or ThreadSafeContext (no exclusive access to anything) and it would be good to capture operations doable with this level of access with a new ExclusiveContext class.

The attached patch adds this class and changes Parser and BytecodeEmitter and most parts of the VM transitively reachable from them to use it.  ExclusiveContext doesn't do anything special except act as a filter for what data can be fetched from the compartment/zone/runtime.  The split is pretty good but not perfect, and there are some dubious ExclusiveContext -> JSContext downcasts that will need to be addressed later, e.g. around string flattening.  While large, this patch doesn't (well, shouldn't) make any functional changes to the VM's behavior, and passes jit-tests and jstests.

Comment 1

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

Comment on attachment 765938 [details] [diff] [review]
patch (0dd5a67fd870)

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

It would be good to send an email to the mailing list so people know what these changes are for.

::: js/public/RootingAPI.h
@@ +517,5 @@
>   */
>  template <typename T>
>  class MOZ_STACK_CLASS Rooted : public js::RootedBase<T>
>  {
> +    template <typename CX>

Can you comment that CX is either ContextFriendFields or PerThreadDataFriendFields?

@@ +678,5 @@
>      const uint8_t *end;
>  
>      template <typename T>
> +    void init(ContextFriendFields *cx, const T *ptr, size_t count)
> +    {

The brace should still be on the same line as the method decl.

@@ +686,5 @@
>          *stack = this;
>          this->start = (const uint8_t *) ptr;
>          this->end = this->start + (sizeof(T) * count);
> +
> +        MOZ_GUARD_OBJECT_NOTIFIER_INIT;

I think this can be removed since the constructors take care of it.

@@ +741,5 @@
>      {
>          MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>      }
>  
> +    template <typename CX>

Something doesn't seem right. CX is unused.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1733,2 @@
>  {
> +    if (!cx->isJSContext())

I assume this isn't what we want to do long-term. If so, can you change it to an assertion or something so we don't forget to fix it?

::: js/src/frontend/ParseMaps.h
@@ +239,1 @@
>              

Can you fix this whitespace?

::: js/src/frontend/Parser.cpp
@@ +415,5 @@
>  
>  template <typename ParseHandler>
>  Parser<ParseHandler>::~Parser()
>  {
> +    context->asJSContext()->runtime()->activeCompilations++;

This should be -- rather than ++ !

::: js/src/ion/IonCaches.cpp
@@ +1668,5 @@
>          // Lock the context before mutating the cache. Ideally we'd like to do
>          // finer-grained locking, with one lock per cache. However, generating
>          // new jitcode uses a global ExecutableAllocator tied to the runtime.
>          LockedJSContext cx(slice);
> +        gc::AutoSuppressGC suppress(cx);

Could you have someone else take a look at this? I don't know enough about this code here.

::: js/src/ion/IonCaches.h
@@ +257,5 @@
>          this->script = script;
>          this->pc = pc;
>      }
>  
> +    void getScriptedLocation(JSScript **pscript, jsbytecode **ppc) {

I guess rooting is no longer needed here, but it's still hard for me to understand why it was removed.

::: js/src/jsapi.h
@@ +4394,5 @@
>  JS_EncodeStringToBuffer(JSContext *cx, JSString *str, char *buffer, size_t length);
>  
>  class JSAutoByteString
>  {
> +public:

This should be indented two spaces. It makes it easier to read diffs that way.

@@ +4404,5 @@
>          MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>      }
>  
>      JSAutoByteString(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM)
> +    : mBytes(NULL)

Indent is screwed up here too.

::: js/src/jscntxt.h
@@ +1515,5 @@
> + * compartments.
> + *
> + * - ExclusiveContext is used by threads operating in one compartment, where
> + * other threads may operate in other compartments, but *not* the one which
> + * the ExclusiveContext is in.

Can you remark that ExclusiveContexts can only ever enter the atoms compartment?

::: js/src/jscntxtinlines.h
@@ +620,5 @@
> +    return thing->compartment() == compartment_;
> +}
> +
> +inline void
> +js::ExclusiveContext::setCompartmentPrivate(JSCompartment *comp)

This is kind of a confusing name because compartment->data might be called the "private data" for the compartment. Maybe call this privateSetCompartment?

::: js/src/jscompartmentinlines.h
@@ +40,5 @@
>  
>  namespace js {
>  
>  /*
> + * Entering the atoms compartment is not possible with the AutoCompartment

Maybe remove the final 'the' as long as you're here.

::: js/src/jsinfer.cpp
@@ +6040,5 @@
>      return true;
>  }
>  
> +    js::types::TypeObject *getNewType(js::ExclusiveContext *cx,
> +                                      js::Class *clasp, JSFunction *fun = NULL);

This seems out of place.

::: js/src/jsobj.cpp
@@ +1297,5 @@
>  
>      if (CanBeFinalizedInBackground(allocKind, clasp))
>          allocKind = GetBackgroundAllocKind(allocKind);
>  
> +    NewObjectCache *cache = cx->isJSContext()

It might be cleaner if you enclose this whole block in
  if (JSContext *jscx = cx->maybeJSContext()) {

  }
Then you could avoid the later asJSContext calls. I guess entry's definition would still have to go outside.

@@ +1363,5 @@
>       * will flush the new object cache).
>       */
>      JSProtoKey protoKey = GetClassProtoKey(clasp);
>  
> +    NewObjectCache *cache = cx->isJSContext()

Same here too maybe.

::: js/src/jsreflect.cpp
@@ +3057,5 @@
>      size_t length = stable->length();
>      CompileOptions options(cx);
>      options.setFileAndLine(filename, lineno);
>      options.setCanLazilyParse(false);
> +    Parser<FullParseHandler> parser(cx, &cx->tempLifoAlloc(), options, chars.get(), length,

Could you maybe create a separate constructor that takes a JSContext and automatically uses tempLifoAlloc?

::: js/src/jsscript.cpp
@@ +67,5 @@
>      JS_ASSERT(!self->callObjShape_);
>      JS_ASSERT(self->bindingArrayAndFlag_ == TEMPORARY_STORAGE_BIT);
>  
>      if (numArgs > UINT16_MAX || numVars > UINT16_MAX) {
> +        if (cx->isJSContext()) {

It would be nice if we had a way to centralize how errors are handled off-thread. If the plan is to drop all errors, maybe we should just change the signatures of the error reporting functions to take ExclusiveContext. They would do nothing if given a non-JSContext.

::: js/src/jsscript.h
@@ +605,2 @@
>                                       js::frontend::BytecodeEmitter *bce);
> +    // inits a JSOP_STOP-only script

Can you make this a sentence (capitalize and period)?

::: js/src/vm/SelfHosting.cpp
@@ +669,5 @@
> +     * JSOP_NAME or JSOP_GNAME to access unbound variables. JSOP_CALLINTRINSIC
> +     * does a name lookup in a special object that contains properties
> +     * installed during global initialization and that properties from
> +     * self-hosted scripts get copied into lazily upon first access in a
> +     * global.

This seems oddly worded. Maybe split into multiple sentences?

@@ +673,5 @@
> +     * global.
> +     * As that object is inaccessible to client code, the lookups are
> +     * guaranteed to return the original objects, ensuring safe implementation
> +     * of self-hosted builtins.
> +     * Additionally, the special syntax _CallFunction(receiver, ...args, fun)

Also, can you put line breaks between paragraphs?

::: js/src/vm/Shape.cpp
@@ +446,5 @@
>  {
>      JS_ASSERT(!JSID_IS_VOID(id));
>  
>      if (!obj->isExtensible()) {
> +        obj->reportNotExtensible(cx->asJSContext());

I guess we don't expect non-extensible objects to be in thread-local compartments?

@@ -1390,4 @@
>      JS_ASSERT(p);
>  
>      InitialShapeEntry &entry = const_cast<InitialShapeEntry &>(*p);
> -    JS_ASSERT(entry.shape->isEmptyShape());

Could we still assert this on the main thread (though maybe moving it inside the branch below so it's clear why it's conditional)?

::: js/src/vm/String-inl.h
@@ +171,5 @@
>               typename js::MaybeRooted<JSString*, allowGC>::HandleType left,
>               typename js::MaybeRooted<JSString*, allowGC>::HandleType right,
>               size_t length)
>  {
> +    if (!validateLength(cx->asJSContext(), length))

Shu's patch does this without asJSContext.

@@ +287,4 @@
>  {
>      JS_ASSERT(chars[length] == jschar(0));
>  
> +    if (!validateLength(cx->asJSContext(), length))

Same here.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b674f0e40c8e

Comment 3

[Nicholas Nethercote [inactive]]

jscntxt.h has the following code:

  struct DtoaState;
        
  extern void
  js_ReportOutOfMemory(js::ThreadSafeContext *cx);

  extern void
  js_ReportAllocationOverflow(js::ThreadSafeContext *cx);
    
  extern void
  js_ReportOverRecursed(js::ThreadSafeContext *cx);

which is also present in vm/Runtime.h, which jscntxt.h #includes.  Could you remove it?  (I realize this is probably fallout from the merge you had to do when I split off vm/Runtime.h;  sorry about that.)

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b674f0e40c8e
https://hg.mozilla.org/mozilla-central/rev/6efe5b6904d0