1243808 - Allow modules to be compiled off main thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

The browser can compile scripts on a helper thread and hence reduce jank for large scripts.  We want to be able to do that for modules too.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1243808-off-thread-compilation

This works very like off-main-thread-compilation for scripts.  There are a couple of wrinkles:
 - there are new prototypes to fix up after merging compartments
 - the module scope chain needs fixing after merging too
 - you can't currently freeze objects if you only have an ExclusiveContext so this is deferred until we're on the main thread again

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1243808-off-thread-compilation v2

Updated patch in light of recent static scope changes.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8716355 [details] [diff] [review]
bug1243808-off-thread-compilation v2

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

Looks fine to me; thanks for the refactoring. Sorry it took so long.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +771,5 @@
>      CompileOptions options(cx, optionsInput);
>      options.maybeMakeStrictMode(true); // ES6 10.2.1 Module code is always strict mode code.
>      options.setIsRunOnce(true);
>  
>      Rooted<StaticScope*> staticScope(cx, &cx->global()->lexicalScope().staticBlock());

Since this function can now be called from off thread, please copy the warning about passing in the current context's global lexical scope being incorrect if we start optimizing global bindings.

@@ +781,5 @@
> +
> +    // This happens in GlobalHelperThreadState::finishModuleParseTask() when a
> +    // module is compiled off main thread.
> +    if (cx->isJSContext() && !ModuleObject::FreezeArrayProperties(cx->asJSContext(), module))
> +        return nullptr;

Hm I don't really like this duplication.

As a followup, we should take a look at splitting out a NativeObject-only version of SetIntegrityLevel that takes an ExclusiveContext*. From a quick reading, I think most of the calls in that function that takes a JSContext* have to do with proxies.

::: js/src/shell/js.cpp
@@ +3448,5 @@
>      const char16_t* chars = stableChars.twoByteRange().start().get();
>      SourceBufferHolder srcBuf(chars, scriptContents->length(),
>                                SourceBufferHolder::NoOwnership);
>  
> +    RootedObject module(cx, frontend::CompileModule(cx, options, srcBuf));

Huh. The global argument was unused?

::: js/src/vm/HelperThreads.cpp
@@ +1147,5 @@
>  
>      // Make sure we have all the constructors we need for the prototype
>      // remapping below, since we can't GC while that's happening.
>      Rooted<GlobalObject*> global(cx, &cx->global()->as<GlobalObject>());
> +    if (!EnsureParserCreatedClasses(cx, kind)) {

Shouldn't this already be done from the call to |CreateGlobalForOffThreadParse| when we started the parse task?

@@ +1254,5 @@
> +
> +        // Module objects don't have standard prototypes either.
> +        JSObject* moduleProto = parseGlobal->maybeGetModulePrototype();
> +        JSObject* importEntryProto = parseGlobal->maybeGetImportEntryPrototype();
> +        JSObject* exportEntryProto = parseGlobal->maybeGetExportEntryPrototype();

Making sure I understand correctly: these calls to get the module-related protos are using the "maybe" variant because they will only be created when doing off-thread parsing of modules, not global scripts?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Shu-yu Guo [:shu] from comment #3)

> Since this function can now be called from off thread, please copy the
> warning about passing in the current context's global lexical scope being
> incorrect if we start optimizing global bindings.

Done.

> As a followup, we should take a look at splitting out a NativeObject-only
> version of SetIntegrityLevel that takes an ExclusiveContext*. From a quick
> reading, I think most of the calls in that function that takes a JSContext*
> have to do with proxies.

Sure.  I filed bug 1247194 for this.

> Huh. The global argument was unused?

Yes, it gets everything it needs from the context.

> Shouldn't this already be done from the call to
> |CreateGlobalForOffThreadParse| when we started the parse task?

Yes I think so.  I'll change this for assertions.
 
> Making sure I understand correctly: these calls to get the module-related
> protos are using the "maybe" variant because they will only be created when
> doing off-thread parsing of modules, not global scripts?

Yes, we don't create them if we're not parsing a module.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Jon Coppeard (:jonco) (Away until 22nd Feb?) from comment #4)

> > Shouldn't this already be done from the call to
> > |CreateGlobalForOffThreadParse| when we started the parse task?
> 
> Yes I think so.  I'll change this for assertions.

This caused the debug/bug-1238610.js jit-test to fail due to missing array prototype at this point, so I'll leave this as it was.

Comment 6

[Pulsebot]

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

Comment 7

[Carsten Book [:Tomcat]]

caused bustage and so backed out for https://treeherder.mozilla.org/logviewer.html#?job_id=21420628&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b671fd0a624e

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd8573b9dd8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/30df761ce11d for hazard build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=21458492&repo=mozilla-inbound

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48b7c3fa5914

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48b7c3fa5914