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

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 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

6 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

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

Comment 2

6 years ago
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

6 years ago
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

6 years ago
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

6 years ago
Posted patch no-relocatable (obsolete) — Splinter Review
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

6 years ago
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

6 years ago
Depends on: 897655
Assignee

Comment 7

6 years ago
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

6 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

6 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

6 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

6 years ago
Landing RelocatablePtr patch ahead of time to help another bug:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91babde1804d
Whiteboard: [leave open]
Attachment #788204 - Flags: review?(sstangl) → review+
Assignee

Comment 19

6 years ago
Posted patch fix-auto-flush-cache (obsolete) — Splinter Review
What bug 900669 comment 8 said.
Attachment #791101 - Flags: review?(mrosenberg)
Assignee

Comment 20

6 years ago
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

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

Updated

6 years ago
Whiteboard: [games]
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.