902506 - OdinMonkey: make "use asm" compatible with off-thread compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Bug 897655 will use off-thread compilation for startup code.  The next step is to use off-thread compilation for <script> tags.  However, "use asm" currently fails:
  js> offThreadCompileScript("function f() { 'use asm' }")
  js> Assertion failure: isJSContext(), at ../jscntxt.h:177
because CompileAsmJS currently must be run on the JSRuntime's main thread.  From a quick look at AsmJS.cpp, this is a pretty simple task, esp. once bug 902095 gives ExclusiveContext a compartment().

Comment 1

[Luke Wagner [:luke]]

Following patches apply on top of the two in bug 864220 and 902096.

Comment 2

[Luke Wagner [:luke]]

Attachment: hoist cx->options from JSContext to ExclusiveContext

This patch hoists cx->options from JSContext to ExclusiveContext so that options may be accessed during off-thread compilation.  In the off-thread case, this works by having ExclusiveContext takes a snapshot of the options in the JSContext when the ExclusiveContext is created.

Comment 3

[Luke Wagner [:luke]]

Attachment: dont-depend-on-ion-compartment

This patch cuts all dependency of Odin compilation on IonCompartment/IonRuntime/ExecutableAllocator by simply mmap'ing/VirtualAlloc'ing the memory itself.   Normally I'd like to avoid duplicating OS-specific calls but these calls are very simple and overall the code is simpler (e.g., we can now rely on page-alignment).  The next patch will remove JSC::ASMJS_CODE and deal with about:memory reporting.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix/enhance asm.js memory reporting

The last patch stopped using ExecutableAllocator which means that asm.js code isn't reported.  This code adds it back but improves things: asm.js code is now reported per-compartment along with the size of the AsmJSModule itself (which can be large due to exits and heap-access bookkeeping).  Lastly, the patch makes a proper JSObject subclass for AsmJSModuleObject so that is<>/as<> work.

Comment 5

[Luke Wagner [:luke]]

Attachment: no-relocatable

RelocatablePtr asserts it is only used on the main thread and AsmJSModule (which contains several) is now build off the main thread.  Ultimately, the PropertyName* fields are pretty simple (initialized, read-only) so I think I can just use MarkStringUnbarriered on them and store raw PropertyName*.  Is that right terrence or am I forgetting a barrier?

Comment 6

[Luke Wagner [:luke]]

Attachment: make CompileAsmJS take an ExclusiveContext

This final patch makes all of AsmJS.cpp use an ExclusiveContext, which is pretty simple.  With this, I can compile all the asm.js unit tests with offThreadCompileScript().  There is one unrelated assert that fires (in AutoLockForExclusiveAccess) involving numExclusiveThreads, but I'm pretty sure this is fixed by bug 897655 (since numExclusiveThreads isn't incremented anywhere on trunk, but is by bug 897655).

Comment 7

[Luke Wagner [:luke]]

Attachment: no-relocatable

This version asserts that the given string is tenured and thus no writeBarrierPost is needed on initialization.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 788264 [details] [diff] [review]
no-relocatable

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

Nice! r=me

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 788207 [details] [diff] [review]
fix/enhance asm.js memory reporting

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

Looks good!  Just a couple of nits.

::: js/src/jit/AsmJSModule.cpp
@@ +94,5 @@
>          js_delete(functionCounts(i));
>  }
> +
> +void
> +AsmJSModule::sizeOfMisc(mozilla::MallocSizeOf mallocSizeOf, JS::ObjectsExtraSizes *sizes)

I'm ambivalent about passing in ObjectsExtraSizes here, when you only modify two fields.  You could instead have two outparams, one for code and one for data -- that would be less concise, but expose less data unnecessarily.  I'll let you decide whether to change.

@@ +96,5 @@
> +
> +void
> +AsmJSModule::sizeOfMisc(mozilla::MallocSizeOf mallocSizeOf, JS::ObjectsExtraSizes *sizes)
> +{
> +    sizes->asmJSModuleData = sizeof(AsmJSModule) +

First, this method should be in AsmJSModuleObject.  See PropertyIteratorObject and RegExpStaticsObject for comparison.

Second, we avoid sizeof(T) in memory reporters for things allocated on the heap, because (a) it doesn't measure slop bytes, and (b) it doesn't let DMD know that we've measured the heap block.  Use |mallocSizeOf(module())| instead.

::: js/src/jsobj.cpp
@@ +5625,5 @@
>          sizes->regExpStatics = as<RegExpStaticsObject>().sizeOfData(mallocSizeOf);
>      } else if (is<PropertyIteratorObject>()) {
>          sizes->propertyIteratorData = as<PropertyIteratorObject>().sizeOfMisc(mallocSizeOf);
> +    } else if (is<AsmJSModuleObject>()) {
> +        as<AsmJSModuleObject>().module().sizeOfMisc(mallocSizeOf, sizes);

Once you've moved sizeOfMisc() into AsmJSModuleObject, you won't need the module() call here and this case will look just like the three above it.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1989,5 @@
> +                 "Memory used by AOT-compiled asm.js code.");
> +
> +    ZCREPORT_BYTES(cJSPathPrefix + NS_LITERAL_CSTRING("objects/malloc-heap/asm.js-module-data"),
> +                   cStats.objectsExtra.asmJSModuleData,
> +                   "Memory allocated for an asm.js module not including executable code.");

Change "not including executable code" to just "data"?

Comment 10

[Nicholas Nethercote [inactive]]

I forgot to say:  reporting the additional data is great, as is the creation of AsmJSModuleObject so that is<> and as<> can be used.  Thanks!

Comment 11

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
Thanks!

> You could instead have two outparams, one for code and one for
> data -- that would be less concise, but expose less data unnecessarily. 
> I'll let you decide whether to change.

I had the same internal debate.  Since it bugged you too, I'll go with the two-outparam form.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 788200 [details] [diff] [review]
hoist cx->options from JSContext to ExclusiveContext

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

Which options do you need from the cx to compile asm.js code?  Just JSOPTION_ASMJS?  I'm worried about this patch as it seems to make cx->options even less meaningful than before as it doesn't really even reflect the current state of things.  Ideally these options should just be bits set on the runtime (or better yet, not exist at all) but they couldn't be read from off thread without racing.

CompileOptions already has an asmJSOption member which is propagated from its initial JSContext, and these options are around when parsing off thread.  Could those options be passed in to CompileAsmJS?

Comment 13

[Brian Hackett [Laid off!]]

Comment on attachment 788213 [details] [diff] [review]
make CompileAsmJS take an ExclusiveContext

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

::: js/src/jit/AsmJS.cpp
@@ +4817,5 @@
> +    // If 'cx' isn't a JSContext, then we are already off the main thread so
> +    // off-thread compilation must be enabled.
> +    if (!cx->isJSContext())
> +        return true;
> +    return OffThreadCompilationEnabled(cx->asJSContext());

I think this method should also check the number of available threads.  If there is only one worker thread then any jobs it triggers won't get done until the original parse finishes, and CheckFunctionsParallelImpl will iiuc block forever.

::: js/src/jsworkers.cpp
@@ +34,5 @@
> +    // If 'cx' is not a JSContext, we are already off the main thread and the
> +    // worker threads would have already been initialized.
> +    if (!cx->isJSContext()) {
> +        JS_ASSERT(cx->workerThreadState() != NULL);
> +        return false;

return true

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 788200 [details] [diff] [review]
hoist cx->options from JSContext to ExclusiveContext

(In reply to Brian Hackett (:bhackett) from comment #12)
> CompileOptions already has an asmJSOption member which is propagated from
> its initial JSContext, and these options are around when parsing off thread.
> Could those options be passed in to CompileAsmJS?

Hah, I hadn't noticed you had added that.  Yes, that that would work perfectly; indeed JSOPTION_ASMJS is all I need and I have the Parser object.

Comment 15

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #13)
> I think this method should also check the number of available threads.  If
> there is only one worker thread then any jobs it triggers won't get done
> until the original parse finishes, and CheckFunctionsParallelImpl will iiuc
> block forever.

Good point.  IIRC, there can be only one off-main-thread parsing task?  If so, then it seems like heperThreadCount() >= 2 would be sufficient.  If we ever wanted to be able to use N threads for parsing (which seems reasonable if we can avoid the atoms lock by not creating real atoms-compartment att=oms as you suggested (which I think could help general parsing perf if we LifoAlloc'd them instead of using real GC allocation), though, we'd have to do something more (maintain some "numActiveParseTasks" and compare that against helperThreadCount).  Do you think it makes sense to do the more general guard now?

Comment 16

[Brian Hackett [Laid off!]]

(In reply to Luke Wagner [:luke] from comment #15)
> (In reply to Brian Hackett (:bhackett) from comment #13)
> > I think this method should also check the number of available threads.  If
> > there is only one worker thread then any jobs it triggers won't get done
> > until the original parse finishes, and CheckFunctionsParallelImpl will iiuc
> > block forever.
> 
> Good point.  IIRC, there can be only one off-main-thread parsing task?  If
> so, then it seems like heperThreadCount() >= 2 would be sufficient.  If we
> ever wanted to be able to use N threads for parsing (which seems reasonable
> if we can avoid the atoms lock by not creating real atoms-compartment
> att=oms as you suggested (which I think could help general parsing perf if
> we LifoAlloc'd them instead of using real GC allocation), though, we'd have
> to do something more (maintain some "numActiveParseTasks" and compare that
> against helperThreadCount).  Do you think it makes sense to do the more
> general guard now?

I don't think the general guard is necessary right now, though I'll make sure to add a note in WorkerThreadState::canStartParseTask when landing bug 897655 this week so we don't end up with bad behavior should this be relaxed.

Comment 17

[Luke Wagner [:luke]]

Landing RelocatablePtr patch ahead of time to help another bug:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91babde1804d

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/91babde1804d

Comment 19

[Luke Wagner [:luke]]

Attachment: fix-auto-flush-cache

What bug 900669 comment 8 said.

Comment 20

[Luke Wagner [:luke]]

Attachment: fix-auto-flush-cache

Oops, forgot to remove AutoFlushCache from ModuleCompiler.

Comment 21

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b827d6e26380
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5ef6e3d1e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/79edb4566926
https://hg.mozilla.org/integration/mozilla-inbound/rev/a63f47fcbe98

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b827d6e26380
https://hg.mozilla.org/mozilla-central/rev/dc5ef6e3d1e6
https://hg.mozilla.org/mozilla-central/rev/79edb4566926
https://hg.mozilla.org/mozilla-central/rev/a63f47fcbe98