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

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [games])

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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().
(Assignee)

Comment 1

5 years ago
Following patches apply on top of the two in bug 864220 and 902096.
Depends on: 864220, 902095
(Assignee)

Comment 2

5 years ago
Created attachment 788200 [details] [diff] [review]
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.
Attachment #788200 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
Created attachment 788204 [details] [diff] [review]
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.
Attachment #788204 - Flags: review?(sstangl)
(Assignee)

Comment 4

5 years ago
Created attachment 788207 [details] [diff] [review]
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.
Attachment #788207 - Flags: review?(n.nethercote)
(Assignee)

Comment 5

5 years ago
Created attachment 788210 [details] [diff] [review]
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?
Attachment #788210 - Flags: review?(terrence)
(Assignee)

Comment 6

5 years ago
Created attachment 788213 [details] [diff] [review]
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).
Attachment #788213 - Flags: review?(bhackett1024)
(Assignee)

Updated

5 years ago
Depends on: 897655
(Assignee)

Comment 7

5 years ago
Created attachment 788264 [details] [diff] [review]
no-relocatable

This version asserts that the given string is tenured and thus no writeBarrierPost is needed on initialization.
Attachment #788210 - Attachment is obsolete: true
Attachment #788210 - Flags: review?(terrence)
Attachment #788264 - Flags: review?(terrence)
Comment on attachment 788264 [details] [diff] [review]
no-relocatable

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

Nice! r=me
Attachment #788264 - Flags: review?(terrence) → review+
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"?
Attachment #788207 - Flags: review?(n.nethercote) → review+
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!
(Assignee)

Comment 11

5 years ago
(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 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?
Attachment #788200 - Flags: review?(bhackett1024)
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
Attachment #788213 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 14

5 years ago
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.
Attachment #788200 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
(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?
(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.
(Assignee)

Comment 17

5 years ago
Landing RelocatablePtr patch ahead of time to help another bug:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91babde1804d
Whiteboard: [leave open]

Updated

5 years ago
Attachment #788204 - Flags: review?(sstangl) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 791101 [details] [diff] [review]
fix-auto-flush-cache

What bug 900669 comment 8 said.
Attachment #791101 - Flags: review?(mrosenberg)
(Assignee)

Comment 20

5 years ago
Created attachment 791105 [details] [diff] [review]
fix-auto-flush-cache

Oops, forgot to remove AutoFlushCache from ModuleCompiler.
Attachment #791101 - Attachment is obsolete: true
Attachment #791101 - Flags: review?(mrosenberg)
Attachment #791105 - Flags: review?(mrosenberg)
Attachment #791105 - Flags: review?(mrosenberg) → review+
Depends on: 907085
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Whiteboard: [games]
Target Milestone: --- → mozilla26

Updated

5 years ago
Blocks: 710398
You need to log in before you can comment on or make changes to this bug.