This bug is to add functions to serialize/deserialize an entire asm.js module (represented in memory by AsmJSModule) to/from a stream of bytes. This functionality, combined with bug 900255 on the network side should allow us to avoid the compilation cost of asm.js entirely on cache hits (which can be many seconds, esp. on mobile).

This bug will just add the raw functionality (with the initial use case being to use them to trivially clone an AsmJSModule (serialize->deserialize) to remove the current limitation that an asm.js module can only be linked once. Caching interactions will come after.

This is a minor preparatory refactoring patch to hoist out AsmJSModuleSourceDesc (was AsmJSModule::PostLinkFailureInfo) so that it can be passed to/from caching hooks.

Another refactoring patch to tidy up LinkAsmJS in preparation for handling the module.isLinked() case by cloning the module.

Comment on attachment 786481 [details] [diff] [review] hoist-source-desc Review of attachment 786481 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! r+ with the fixed destructor. ::: js/src/jsfriendapi.cpp @@ +1149,5 @@ > + > +AsmJSModuleSourceDesc::~AsmJSModuleSourceDesc() > +{ > + if (scriptSource_) > + scriptSource_->incref(); scriptSource_->decref(); ::: js/src/jsfriendapi.h @@ +1796,5 @@ > +struct AsmJSModuleSourceDesc > +{ > + ScriptSource *scriptSource_; > + uint32_t bufStart_; > + uint32_t bufEnd_; These fields will be public by default, as it's a struct and there is no visibility qualifier. You wrote some accessors though. Is there any chance that you wanted to use a class instead?

Comment on attachment 786483 [details] [diff] [review] factor-link-asmjs Review of attachment 786483 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: js/src/ion/AsmJSLink.cpp @@ +604,5 @@ > + return obj; > +} > + > +// Implements the semantics of an asm.js module function that has been successfully validated. > +static JSBool Regarding the last bugs about the topic, shouldn't it be |static bool| here?

Right on all counts; thanks Benjamin! Landing just these preparatory patches: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b241c4e3eec https://hg.mozilla.org/integration/mozilla-inbound/rev/7abe5be6f8d2

https://hg.mozilla.org/mozilla-central/rev/8b241c4e3eec https://hg.mozilla.org/mozilla-central/rev/7abe5be6f8d2

The above patch makes the IonContext's ionRuntime == NULL which causes the updateTop called by masm.executableCopy to crash. It looks like there was already a terrible hack "if (this == NULL)" in AutoFlushCache::update to handle the fact that the AutoFlushCache in ModuleCompiler doesn't push itself to the list (because it's IonRuntime argument is NULL), so the whole thing is kinda pointless. This patch just calls ExecutableAllocator::cacheFlush directly when there is no IonRuntime or no AutoFlushCache. As a bonus, the awful "if (this == NULL)" gets to go away.

Comment on attachment 791100 [details] [diff] [review] fix-auto-flush-cache Oops, wrong bug!

More preparatory cleanup: this patch wraps the memcpy'able members of the various AsmJSModule structs in a struct { } pod so that, in the later deserialization patch, we can simply memcpy(dst, &src.pod, sizeof(src.pod)) before handling the other fields that need special (de)serialization.

To simplify (de)serialization, this patch uses plain PropertyName* and MarkUnbarrieredString for the 3 PropertyNames stored in AsmJSModule. The reasoning and change here is the same as in cset 91babde1804d.

There is no performance reason to store direct pointers in AsmJSModule::{Exit, ExportedFunction} so, to simplify (de)serialization, this patch just lets these pointers stay offsets.

Comment on attachment 796207 [details] [diff] [review] un-heap-ptr Review of attachment 796207 [details] [diff] [review]: ----------------------------------------------------------------- This is certainly fine from a generational GC perspective, however, if AsmJSModule can live on the main thread without a SuppressGC, then incremental GC still needs barriers. I'm sure you've already thought of all that though, so r=me. ::: js/src/jit/AsmJSModule.h @@ +348,5 @@ > > + private: > + PropertyName * globalArgumentName_; > + PropertyName * importArgumentName_; > + PropertyName * bufferArgumentName_; Doesn't the * usually go all the way over next to the name?

(In reply to Terrence Cole [:terrence] from comment #13) > This is certainly fine from a generational GC perspective, however, if > AsmJSModule can live on the main thread without a SuppressGC, then > incremental GC still needs barriers. I'm sure you've already thought of all > that though, so r=me. Could you explain more? These aren't weak refs and they are only initialized so I don't see what barriers would be necessary for igc. > Doesn't the * usually go all the way over next to the name? Oops, I guess not in this file. (It looks terrible; it looks like mostly we just remove the alignment.)

Comment on attachment 796205 [details] [diff] [review] segregate-pods Review of attachment 796205 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Compilation won't work with --enable-perf flags though. r+ if you decide to keep the perfProfiledFunctions_ vector, otherwise there might be more changes to do. ::: js/src/jit/AsmJSModule.h @@ +229,5 @@ > > ExportedFunction(PropertyName *name, > PropertyName *maybeFieldName, > mozilla::MoveRef<ArgCoercionVector> argCoercions, > + ReturnType returnType) { Wouldn't we want a clear separation with the function body here, as the arguments are on several lines? There doesn't seem to be any rule for that in the style guidelines, so I imagine it's up to the developer. @@ +250,5 @@ > + ExportedFunction(mozilla::MoveRef<ExportedFunction> rhs) { > + name_ = rhs->name_; > + maybeFieldName_ = rhs->maybeFieldName_; > + argCoercions_ = mozilla::Move(rhs->argCoercions_); > + pod.returnType_ = rhs->pod.returnType_; What about using PodAssign here (or directly memcpy) instead of copying fields one by one? @@ -344,4 @@ > ProfiledFunctionVector profiledFunctions_; > #endif > #if defined(JS_ION_PERF) > - ProfiledFunctionVector perfProfiledFunctions_; I think the perfProfiledFunctions_ removal will prevent compilation in --enable-perf mode. The reason why there were two distinct vectors for Vtune and Perf is that you might want to compile the shell for both, but not use both at runtime, which would be the case if we'd share the vector of profiled functions.

(In reply to Luke Wagner [:luke] from comment #14) > (In reply to Terrence Cole [:terrence] from comment #13) > > This is certainly fine from a generational GC perspective, however, if > > AsmJSModule can live on the main thread without a SuppressGC, then > > incremental GC still needs barriers. I'm sure you've already thought of all > > that though, so r=me. > > Could you explain more? These aren't weak refs and they are only > initialized so I don't see what barriers would be necessary for igc. Only initialized is the piece I had forgotten. In that case, this is fine.

Comment on attachment 796224 [details] [diff] [review] rm-patch-functions Review of attachment 796224 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup!

Good points all around Benjamin! You'd think I'd always remember to --enable-profiling and build giving you a review by now...

Comment on attachment 796341 [details] [diff] [review] segregate-pods Review of attachment 796341 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/88a3198c7007 https://hg.mozilla.org/integration/mozilla-inbound/rev/1efaa2330cee https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb5a77c36a6

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c98af21f7072 - b2g ("arm-linux-androideabi-gcc (GCC) 4.4.3" apparently) said https://tbpl.mozilla.org/php/getParsedLog.php?id=27099845&tree=Mozilla-Inbound#error2

Oh for goodness sakes, I even try servered; just not on "B2G Emu Debug". I'm pretty sure this is a compiler bug involving unnamed structs. Let's see: https://tbpl.mozilla.org/?tree=Try&rev=f28e9bd4e6b6

Try green with named structs, relanding: https://hg.mozilla.org/integration/mozilla-inbound/rev/8057f2fc8817 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9369f9ae80e https://hg.mozilla.org/integration/mozilla-inbound/rev/25a5b706d2dc

https://hg.mozilla.org/mozilla-central/rev/8057f2fc8817 https://hg.mozilla.org/mozilla-central/rev/d9369f9ae80e https://hg.mozilla.org/mozilla-central/rev/25a5b706d2dc

> Oh for goodness sakes, I even try servered; just not on "B2G Emu Debug". I always build on all platforms, debug and opt, when doing try server runs, for exactly this reason :)

Ideally, we'd use the same post-compilation-linking code path for both initial compilation and after deserialization. This patch starts that process by creating an AsmJSModule::staticallyLink that takes a LinkData struct contains link information either created by compilation or (later) deserialization. The important thing is that LinkData isn't saved in the AsmJSModule.

This patch simplifies how we save the ScriptSource begin/end information used during link failure, making it easier for the later serialization patch.

Comment on attachment 805473 [details] [diff] [review] split-static-link Review of attachment 805473 [details] [diff] [review]: ----------------------------------------------------------------- Offsets, offsets everywhere. Nice patch. I am glad to see that the perf / valgrind sections have been also updated :) I didn't know this trick for the linked list of uses of a label, that's neat. I am also always surprised that the compiler knows how to handle all these type conversions (uint32_t / uint8_t / int / size_t are used equivalently, which is not obvious). Might be worth to reduce the number of types in the future, to make the code cleaner (maybe follow-up?). ::: js/src/jit-test/tests/asm.js/testFloatingPoint.js @@ +89,5 @@ > var f = asmLink(asmCompile(USE_ASM + "function f(i,j) { i=i|0;j=j|0; return +(((i|0)%(j|0))|0) } return f")); > assertEq(f(1,0), 0); > assertEq(f(-Math.pow(2,31),-1), 0); > > +var {f,g} = asmLink(asmCompile(USE_ASM + "function f() { return 3.5 } function g() { return 3.5 } return {f:f,g:g}")); Destructuring assignments FTW! Also, the fact that f and g both return 3.5 can't make us distinguish them in this test, e.g. if f is called instead of g in the two assertions below, the tests will still pass. Could you have g return another value, please? ::: js/src/jit/AsmJS.cpp @@ +1633,4 @@ > #ifdef JS_ION_PERF > // Fix up the code offsets. Note the endCodeOffset should not be filtered through > + // 'actualOffset' as it is generated using 'size()' rather than a > + // label. It seems like the line fitted in less than 100 chars? @@ +1659,5 @@ > + // serialized/deserialized alongside the AsmJSModule. > + > + linkData->operationCallbackExitOffset = masm_.actualOffset(operationCallbackLabel_.offset()); > + > + // CodeLabels producing during codegen s/producing/produced? @@ +1662,5 @@ > + > + // CodeLabels producing during codegen > + for (size_t i = 0; i < masm_.numCodeLabels(); i++) { > + CodeLabel src = masm_.codeLabel(i); > + int almostPatchAtOffset = src.dest()->offset(); Shouldn't almostPatchAtOffset, targetOffset and patchAtOffset be uint8_t or size_t? @@ +1699,5 @@ > AsmJSGlobalAccess a = globalAccesses_[i]; > masm_.patchAsmJSGlobalAccess(a.offset, code, module_->globalData(), a.globalDataOffset); > } > +#else > + JS_ASSERT(globalAccesses_.empty()); That's cleaner than length() == 0 :) ::: js/src/jit/AsmJSModule.cpp @@ +122,5 @@ > + } > + > + // Initialize global data segment > + > + for (unsigned i = 0; i < exits_.length(); i++) { How about |size_t i|? ::: js/src/jit/AsmJSModule.h @@ +40,5 @@ > > +// Static-link data is used to patch a module either after it has been > +// compiled or deserialized with various absolute addresses (of code or > +// data in the process) or relative addresses (of code or data in the same > +// AsmJSModule). Since StaticLinkData can be serialized alongside the s/StaticLinkData/AsmJSStaticLinkData

(In reply to Benjamin Bouvier [:bbouvier] from comment #28) Thanks! > Also, the fact that f and g both return 3.5 can't make us distinguish them > in this test, e.g. if f is called instead of g in the two assertions below, > the tests will still pass. Could you have g return another value, please? The goal of this test was to use the same constant in two functions (to produce the chained-use behavior of a double constant). But I guess I can make the functions distinguishable by having one return i+3.5. > It seems like the line fitted in less than 100 chars? For comments, we still use 80-width columns. > Shouldn't almostPatchAtOffset, targetOffset and patchAtOffset be uint8_t or > size_t? LabelBase::offset() returns an int32_t (but I guess I should use int32_t instead of 'int', even though we globally assume int is at least 32 bits).

Comment on attachment 805475 [details] [diff] [review] change-source-desc Review of attachment 805475 [details] [diff] [review]: ----------------------------------------------------------------- I like how this patch deletes a lot of code and replaces it with a few small chunks instead. Nice! ::: js/src/jit/AsmJSModule.cpp @@ +140,3 @@ > AsmJSModule::~AsmJSModule() > { > + scriptSource_->decref(); The previous code had a test for scriptSource_ != NULL, but it seems that it actually cannot be the case, right? ::: js/src/jit/AsmJSModule.h @@ +413,5 @@ > + uint32_t charsBegin() const { > + return charsBegin_; > + } > + void initCharsEnd(uint32_t charsEnd) { > + pod.charsLength_ = charsEnd - charsBegin_; Should we assert charsEnd >= charsBegin_? otherwise the unsigned subtract could give crazy results.

(In reply to Benjamin Bouvier [:bbouvier] from comment #30) > The previous code had a test for scriptSource_ != NULL, but it seems that it > actually cannot be the case, right? That's correct: scriptSource_ went from being initialized at the end of compilation (which thus may not occur before destruction) to being initialized with a non-null value at construction. > Should we assert charsEnd >= charsBegin_? otherwise the unsigned subtract > could give crazy results. Good idea

This patch switches x86 to use RelativeLink for patching global loads/stores.

This patch actually gives teeth to ImmPtr/AbsoluteAddress by making them assert !IsCompilingAsmJS(). The remaining ImmPtr/AbsoluteAddress that are used by asm.js are converted AsmJSImmPtr/AsmJSAbsoluteAddress which take a symbolic identifier of the pointee that can be patched to the right address after deserialization. With this patch, asm.js code should be serialization-safe (using staticallyLink to patch after deserialization).

Oops, I forgot to say: Marty: can you review the ARM changes? Benjamin, can you review the rest?

WIP patch for how serialization/deserialization look. Pretty simple after all the preceding patches. The patch doesn't save/match the source, so it's flagrantly wrong. It also needs thorough testing support.

https://hg.mozilla.org/integration/mozilla-inbound/rev/870fd016efc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b599cc91f5a0

Comment on attachment 806284 [details] [diff] [review] asmjs-imm-ptr sr?jandem for overall Ion sanity check

(In reply to Luke Wagner [:luke] from comment #27) > Created attachment 805475 [details] [diff] [review] > change-source-desc > > This patch simplifies how we save the ScriptSource begin/end information > used during link failure, making it easier for the later serialization patch. Please update occurrences of "module.sourceDesc().scriptSource()->filename()" to "module.scriptSource()->filename()" in AsmJSLink.cpp for compiling with JS_ION_PERF :).

Oops, sorry about that! You'd think I'd check before every push by now... https://hg.mozilla.org/integration/mozilla-inbound/rev/20829c9daea7

Comment on attachment 806274 [details] [diff] [review] fix-global-refs Review of attachment 806274 [details] [diff] [review]: ----------------------------------------------------------------- Nice! For an even lighter patch, we could avoid to record the CodeOffsetLabel in the struct and just record the offset. ::: js/src/jit/shared/Assembler-shared.h @@ +614,5 @@ > }; > > +struct AsmJSGlobalAccess > +{ > + CodeOffsetLabel patchAt; We could just use the offset here.

The reason for CodeOffsetLabel is that I'm trying to use CodeOffsetLabel to mean "an offset produced by labels" since labels on x86/x64 store the offset immediately following the to-be-patched immediatem. OTOH, AsmJSStaticLinkData::RelativeLink::patchAtOffset is a raw uint32_t since it is the offset to the beginning of the word to be patched. (offsets offsets everywhere indeed!)

https://hg.mozilla.org/mozilla-central/rev/870fd016efc7 https://hg.mozilla.org/mozilla-central/rev/b599cc91f5a0 https://hg.mozilla.org/mozilla-central/rev/20829c9daea7

Comment on attachment 806284 [details] [diff] [review] asmjs-imm-ptr Review of attachment 806284 [details] [diff] [review]: ----------------------------------------------------------------- A few nits but looks good to me. Serialization seems very close now :) ::: js/src/jit/AsmJS.cpp @@ +3815,2 @@ > case AsmJSMathBuiltin_sqrt: return CheckMathSqrt(f, callNode, retType, def, type); > + case AsmJSMathBuiltin_sin: arity = 1; callee = AsmJSImm_SinD; break; And in a near future (hopefully), a "F" suffix. @@ +6454,5 @@ > return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Disabled by lack of floating point support"); > > +#ifdef JS_CPU_ARM > + if (!hasMOVWT()) > + return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Disabled by lack of ARM movw/t instructions"); OOC, do you know if this frequent on ARM devices not to have this instruction? ::: js/src/jit/AsmJSModule.h @@ +71,5 @@ > {} > + > + size_t serializedSize() const; > + uint8_t *serialize(uint8_t *cursor) const; > + const uint8_t *deserialize(ExclusiveContext *cx, const uint8_t *cursor); nits: declared but never defined. ::: js/src/jit/RegisterSets.h @@ +813,3 @@ > > public: > + AsmJSHeapAccess() {} nit: this constructor looks unused. ::: js/src/jit/shared/Assembler-shared.h @@ +142,5 @@ > > template <class R, class A1, class A2> > explicit ImmPtr(R (*pf)(A1, A2)) > : value(JS_FUNC_TO_DATA_PTR(void *, pf)) > + { nit: trailing whitespace @@ +181,5 @@ > ImmGCPtr() : value(0) {} > }; > > // Used for immediates which require relocation and may be traced during minor GC. > struct ImmMaybeNurseryPtr : public ImmGCPtr Could we assert for ImmMaybeNurseryPtr too? @@ +693,5 @@ > + AsmJSImmKind kind_; > + public: > + AsmJSImmKind kind() const { return kind_; } > + AsmJSImmPtr(AsmJSImmKind kind) : kind_(kind) {} > + AsmJSImmPtr() {} Would it be useful to assert IsCompilingAsmJS() here and in the following structs? It sounds consistent. @@ +698,5 @@ > +}; > + > +// Pointer to be embedded as an immediate that is loaded/stored from by an > +// instruction in asm.js code. > +class AsmJSAbsoluteAddress It is the same implementation as the class above. I'd want to inherit from a class that has this implementation and from which AsmJSImmPtr would inherit too (and the inherited classes would just be type markers), but that's kind of overkill maybe... up to you then. ::: js/src/jit/shared/Assembler-x86-shared.h @@ +196,5 @@ > > + size_t numAsmJSAbsoluteLinks() const { > + return asmJSAbsoluteLinks_.length(); > + } > + AsmJSAbsoluteLink asmJSAbsoluteLink(size_t i) const { nit: return by reference ::: js/src/jit/x64/MacroAssembler-x64.h @@ +100,5 @@ > } > void call(ImmPtr target) { > call(ImmWord(uintptr_t(target.value))); > } > + void call(AsmJSImmPtr target) { Could you make a reference to the comment in Assembler-x86.h, please? Also, this method is in MacroAssembler for x64 but in Assembler for x86, so one of them might be wrong. ::: js/src/jscntxt.h @@ +268,5 @@ > FreeOp *defaultFreeOp() { return runtime_->defaultFreeOp(); } > bool useHelperThreads() { return runtime_->useHelperThreads(); } > size_t helperThreadCount() { return runtime_->helperThreadCount(); } > + void *runtimeAddressForJit() { return runtime_; } > + void *addressOfStackLimitForJit(StackKind kind) { return &runtime_->mainThread.nativeStackLimit[kind]; } What about stackLimitAddress? I don't know if the ForJit suffix is necessary (it has never been used so far), but it's up to you :)

(In reply to Benjamin Bouvier [:bbouvier] from comment #43) > Comment on attachment 806284 [details] [diff] [review] ... > @@ +6454,5 @@ > > return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Disabled by lack of floating point support"); > > > > +#ifdef JS_CPU_ARM > > + if (!hasMOVWT()) > > + return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Disabled by lack of ARM movw/t instructions"); > > OOC, do you know if this frequent on ARM devices not to have this > instruction? All ARMv6 devices! It would be very unfortunate if asm.js were no longer supported on ARMv6 devices. They might be old, but they are cheaper, and the really low end devices need the efficiency of asm.js the most. I have been testing on ARMv6 devices all along - it's rather broken at present due to constant pool issues, but there were plans to address these. Gone is the possibility of using FF with asm.js to deliver apps on the Raspberry Pi. It should be possible to place any patchable constants in the constant pools and to continue to support the ARMv6. Such support would even help the ARMv7 as it would allow some of the global context constant data to be placed into the constant pools and patched into the code when linking. For example the global context pointer and the heap base could be usefully patched into the constant pools freeing up these registers.

Global access x86 patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/368ed994192f

(In reply to Benjamin Bouvier [:bbouvier] from comment #43) Thanks, great suggestions! > > +#ifdef JS_CPU_ARM > > + if (!hasMOVWT()) > > + return Warn(parser, JSMSG_USE_ASM_TYPE_FAIL, "Disabled by lack of ARM movw/t instructions"); > > OOC, do you know if this frequent on ARM devices not to have this > instruction? Sounds like a non-trivial amount. I'll add !hasMOVWT() support in a new patch. > > +class AsmJSAbsoluteAddress > > It is the same implementation as the class above. I'd want to inherit from a > class that has this implementation and from which AsmJSImmPtr would inherit > too (and the inherited classes would just be type markers), but that's kind > of overkill maybe... up to you then. The problem is that then AsmJSAbsoluteAddress would implicitly cast to AbsoluteAddress which could lead to confusing bugs. > > + void call(AsmJSImmPtr target) { > > Could you make a reference to the comment in Assembler-x86.h, please? > Also, this method is in MacroAssembler for x64 but in Assembler for x86, so > one of them might be wrong. In the case of x86, there is the inefficiency. However, in the case of x64, because you can't have 64-bit immediates in the call instruction, in general you need the mov+call (that is what the ImmPtr version does too).

Comment on attachment 806284 [details] [diff] [review] asmjs-imm-ptr Review of attachment 806284 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Pretty exciting :) Can we add some jit-tests to test that deserialized code runs correctly? ::: js/src/jit/RegisterSets.h @@ +792,5 @@ > AnyRegister reg() const { return kind_ == GPR ? AnyRegister(gpr()) : AnyRegister(fpu()); } > }; > > +// Summarizes a heap access made by asm.js code that needs to be patched later > +// and/or lookuped up by the asm.js signal handlers. Different architectures Nit: looked up

(In reply to Jan de Mooij [:jandem] from comment #47) > Can we add some jit-tests to test that deserialized code runs correctly? The patch you are reviewing doesn't do the deserialization, it just preserves existing behavior while avoiding ImmPtr/AbsoluteAddress. OTOH, the deserialization patch will (WIP) add a nestedShell(code) testing function that forks a nested JS process to run 'code'. This allows asmCompile (the basis of all the asm.js unit tests) to test serialization/deserialization on every asm.js unit test! I'm also planning to land bullet.js in jit_tests to test large-scale caching.

This tiny patch just converts a few more void* in arm to ImmPtr, which ended up catching a few missing callWithABIs.

This patch does what I should have done in the first place: just reuse ma_movPatchable and Assembler::patchDataWithValueCheck. This covers both MOVWT and LDR. Carrying forward jan's and benjamin's r+ since the only changes are in arm.

Oops, ignore the changes to testBug893364.js, those have been reverted locally.

(In reply to Luke Wagner [:luke] from comment #48) > OTOH, the deserialization patch will (WIP) add a nestedShell(code) testing > function that forks a nested JS process to run 'code'. This allows > asmCompile (the basis of all the asm.js unit tests) to test > serialization/deserialization on every asm.js unit test! I'm also planning > to land bullet.js in jit_tests to test large-scale caching. Sweet :)

Comment on attachment 807397 [details] [diff] [review] immptr-in-arm Review of attachment 807397 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/MacroAssembler-arm.cpp @@ +3299,5 @@ > rs = L_MOVWT; > else > rs = L_LDR; > > + ma_movPatchable(dest, CallReg, Always, rs); is ImmPtr compatabile with Imm32? I guess if it compiles, it should be fine.

(In reply to Marty Rosenberg [:mjrosenb] from comment #54) Not compatible, but the patch has: -MacroAssemblerARM::ma_call(void *dest) +MacroAssemblerARM::ma_call(ImmPtr dest)

https://hg.mozilla.org/integration/mozilla-inbound/rev/df368eed61f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ac62fceb9362

https://hg.mozilla.org/mozilla-central/rev/df368eed61f9 https://hg.mozilla.org/mozilla-central/rev/ac62fceb9362

This patches seem to be breaking the builds for MacOSX with clang 3.4 1:08.96 Assertion failed: ((E->getType()->isAnyPointerType() || E->getType()->isBlockPointerType() || E->getType()->isObjCQualifiedIdType()) && "EvalAddr only works on pointers"), function EvalAddr, file SemaChecking.cpp, line 3982.

I don't see a SemaChecking.cpp in the tree; is this an internal compiler assertion?

It seems so. Here is the full error log: 1:05.56 IonFrames.o 1:08.96 Assertion failed: ((E->getType()->isAnyPointerType() || E->getType()->isBlockPointerType() || E->getType()->isObjCQualifiedIdType()) && "EvalAddr only works on pointers"), function EvalAddr, file SemaChecking.cpp, line 3982. 1:08.96 0 libLLVM-3.4svn.dylib 0x0000000103d9f745 llvm::sys::PrintStackTrace(__sFILE*) + 37 1:08.96 1 libLLVM-3.4svn.dylib 0x0000000103d9fca9 _ZL13SignalHandleri + 681 1:08.96 2 libsystem_c.dylib 0x00007fff90232cfa _sigtramp + 26 1:08.96 3 libsystem_c.dylib 000000000000000000 _sigtramp + 1876742944 1:08.96 4 libLLVM-3.4svn.dylib 0x0000000103d9f9e6 abort + 22 1:08.96 5 libLLVM-3.4svn.dylib 0x0000000103d9f9a5 __assert_rtn + 53 1:08.96 6 clang 0x00000001020183cb _ZL8EvalAddrPN5clang4ExprERN4llvm15SmallVectorImplIPNS_11DeclRefExprEEEPNS_4DeclE + 683 1:08.96 7 clang 0x0000000102017cf6 clang::Sema::CheckReturnStackAddr(clang::Expr*, clang::QualType, clang::SourceLocation) + 134 1:08.96 8 clang 0x000000010222475a clang::Sema::ActOnReturnStmt(clang::SourceLocation, clang::Expr*) + 1994 1:08.96 9 clang 0x0000000101fa17cc clang::Parser::ParseReturnStatement() + 508 1:08.97 10 clang 0x0000000101f9c8fe clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, bool, clang::SourceLocation*, clang::Parser::ParsedAttributesWithRange&) + 1646 1:08.97 11 clang 0x0000000101f9c1fa clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, bool, clang::SourceLocation*) + 154 1:08.97 12 clang 0x0000000101fa3473 clang::Parser::ParseCompoundStatementBody(bool) + 1267 1:08.97 13 clang 0x0000000101fa651e clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) + 190 1:08.97 14 clang 0x0000000101fb6cab clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) + 2555 1:08.98 15 clang 0x0000000101fa92ae clang::Parser::ParseSingleDeclarationAfterTemplate(unsigned int, clang::Parser::ParsedTemplateInfo const&, clang::ParsingDeclRAIIObject&, clang::SourceLocation&, clang::AccessSpecifier, clang::AttributeList*) + 3950 1:08.98 16 clang 0x0000000101fa7f1d clang::Parser::ParseTemplateDeclarationOrSpecialization(unsigned int, clang::SourceLocation&, clang::AccessSpecifier, clang::AttributeList*) + 845 1:08.98 17 clang 0x0000000101fa7a2e clang::Parser::ParseDeclarationStartingWithTemplate(unsigned int, clang::SourceLocation&, clang::AccessSpecifier, clang::AttributeList*) + 414 1:08.98 18 clang 0x0000000101f473a1 clang::Parser::ParseDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, unsigned int, clang::SourceLocation&, clang::Parser::ParsedAttributesWithRange&) + 465 1:08.98 19 clang 0x0000000101fb4c93 clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*) + 1715 1:08.98 20 clang 0x0000000101fb4557 clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&) + 311 1:08.98 21 clang 0x0000000101f3ad35 clang::ParseAST(clang::Sema&, bool, bool) + 325 1:08.98 22 clang 0x0000000101eee9c5 clang::CodeGenAction::ExecuteAction() + 1205 1:08.98 23 clang 0x0000000101cb216c clang::FrontendAction::Execute() + 124 1:08.98 24 clang 0x0000000101c8e51f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 1007 1:08.99 25 clang 0x0000000101c57eeb clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 3707 1:08.99 26 clang 0x0000000101c503af cc1_main(char const**, char const**, char const*, void*) + 847 1:08.99 27 clang 0x0000000101c5466f main + 479 1:08.99 28 clang 0x0000000101c50054 start + 52 1:08.99 29 clang 0x0000000000000077 start + 4265279575 1:08.99 Stack dump: 1:08.99 0. Program arguments: /opt/local/libexec/llvm-3.4/bin/clang -cc1 -triple x86_64-apple-macosx10.6.0 -emit-obj -disable-free -main-file-name AsmJSModule.cpp -mrelocation-model pic -pic-level 2 -mdisable-fp-elim -masm-verbose -munwind-tables -target-cpu core2 -target-linker-version 136 -gdwarf-2 -coverage-file /Users/ferjm/.ccache/1/5/a24dea8ab75fa14f65f3994184e4ca-2716756.o.tmp.legoBox.local.17612 -resource-dir /opt/local/libexec/llvm-3.4/bin/../lib/clang/3.4 -include ./js-confdefs.h -D ENABLE_PARALLEL_JS -D ENABLE_BINARYDATA -D IMPL_MFBT -D NO_NSPR_10_SUPPORT -D EXPORT_JS_API -D JS_HAS_CTYPES -D DLL_PREFIX="lib" -D DLL_SUFFIX=".dylib" -D USE_ZLIB -D MOZILLA_CLIENT -D DEBUG -D _DEBUG -D TRACING -D USE_SYSTEM_MALLOC=1 -D ENABLE_ASSEMBLER=1 -D ENABLE_JIT=1 -I ctypes/libffi/include -I /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/../../mfbt/double-conversion -I /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/../../intl/icu/source/common -I /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/../../intl/icu/source/i18n -I /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src -I . -I ./../../dist/include -I /Volumes/SSD/dev/mozilla/mozilla-inbound/obj-firefox/dist/include/nspr -I /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src -O3 -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -std=gnu++0x -fdeprecated-macro -fdebug-compilation-dir /Volumes/SSD/dev/mozilla/mozilla-inbound/obj-firefox/js/src -ferror-limit 19 -fmessage-length 0 -fvisibility hidden -pthread -mstackrealign -fblocks -fno-rtti -fobjc-runtime=macosx-10.6.0 -fobjc-dispatch-method=mixed -fobjc-default-synthesize-properties -fencode-extended-block-signature -fno-common -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops -vectorize-slp -o /Users/ferjm/.ccache/1/5/a24dea8ab75fa14f65f3994184e4ca-2716756.o.tmp.legoBox.local.17612 -x c++ /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp 1:09.02 1. /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp:186:43: current parser token ';' 1:09.02 2. /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp:185:1: parsing function body 'FuncCast' 1:09.02 3. /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp:185:1: in compound statement ('{}') 1:09.02 clang: error: unable to execute command: Illegal instruction: 4 1:09.02 clang: error: clang frontend command failed due to signal (use -v to see invocation) 1:09.02 clang version 3.4 (trunk 188250) 1:09.02 Target: x86_64-apple-darwin11.4.2 1:09.02 Thread model: posix 1:09.02 clang: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script. 1:09.02 clang: note: diagnostic msg: 1:09.02 ******************** 1:09.02 1:09.03 PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT: 1:09.03 Preprocessed source(s) and associated run script(s) are located at: 1:09.03 clang: note: diagnostic msg: /var/folders/4d/n9lc_85s551cvt4zy9qg7tkh0000gp/T/AsmJSModule-f59ac9.cpp 1:09.03 clang: note: diagnostic msg: /var/folders/4d/n9lc_85s551cvt4zy9qg7tkh0000gp/T/AsmJSModule-f59ac9.sh 1:09.03 clang: note: diagnostic msg: 1:09.03 1:09.03 ******************** 1:09.03 1:09.03 In the directory /Volumes/SSD/dev/mozilla/mozilla-inbound/obj-firefox/js/src 1:09.03 The following command failed to execute properly: 1:09.04 /opt/local/bin/ccache clang++ -fcolor-diagnostics -o AsmJSModule.o -c -fvisibility=hidden -DENABLE_PARALLEL_JS -DENABLE_BINARYDATA -DIMPL_MFBT -DNO_NSPR_10_SUPPORT -DEXPORT_JS_API -DJS_HAS_CTYPES -DDLL_PREFIX="lib" -DDLL_SUFFIX=".dylib" -DUSE_ZLIB -Ictypes/libffi/include -I/Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/../../mfbt/double-conversion -I/Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/../../intl/icu/source/common -I/Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/../../intl/icu/source/i18n -I/Volumes/SSD/dev/mozilla/mozilla-inbound/js/src -I. -I./../../dist/include -I/Volumes/SSD/dev/mozilla/mozilla-inbound/obj-firefox/dist/include/nspr -I/Volumes/SSD/dev/mozilla/mozilla-inbound/js/src -fPIC -Qunused-arguments -DMOZILLA_CLIENT -include ./js-confdefs.h -MD -MP -MF .deps/AsmJSModule.o.pp -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-common -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -O3 -fno-stack-protector -fno-omit-frame-pointer -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DENABLE_JIT=1 /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp 1:09.04 make[4]: *** [AsmJSModule.o] Error 1 1:09.04 make[4]: *** Waiting for unfinished jobs.... 1:56.07 make[3]: *** [libs] Error 2 1:56.07 make[2]: *** [default] Error 2 1:56.07 make[1]: *** [realbuild] Error 2 1:56.07 make: *** [build] Error 2 1:56.09 224 compiler warnings present.

Interesting. Could you bisect to find the line that causes the ICE (by #if'ing out progressively smaller potions of AsmJSModule.cpp)?

This patch passes with each asmCompile() forking a nested process as described in comment 48. It also works on bullet (with results verified). Unfortunately, this makes the tests 100x slower, so I'm thinking I'll only run a small set using a nested shell as well as bullet.js.

I'm also seeing the compiler error mentioned above on my machines now. ASan builds on RelEng are not affected because the compiler has assertions disabled. I wouldn't want to keep it like that though because it could lead to miscompilations. I'm not sure if the version of Clang we use for the regular OSX builds is also affected (the Clang we use for ASan is a bit newer).

Righto. If one of you could bisect to the offending line of C++ code, we can frob it until it stops asserting.

Aren't lines 185-185 the ones causing the error? 1:09.02 1. /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp:186:43: current parser token ';' 1:09.02 2. /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp:185:1: parsing function body 'FuncCast' 1:09.02 3. /Volumes/SSD/dev/mozilla/mozilla-inbound/js/src/jit/AsmJSModule.cpp:185:1: in compound statement ('{}')

Ok, since I can't test, could you try: - taking off 'inline' - adding an explicit template arg to FuncCast in all the remaining cases where it is missing (e.g., convert FuncCast(js_HandleExecutionInterrupt) to FuncCast<bool (JSContext*)>(js_HandleExecutionInterrupt)) ?

No luck so far. I took off 'inline' from [1], added an explicit template arg to FuncCast where needed and it is still throwing the same build error. I also tried commenting lines from [2] to [3], same error. [1] https://mxr.mozilla.org/mozilla-central/source/js/src/jit/AsmJSModule.cpp#183 [2] https://mxr.mozilla.org/mozilla-central/source/js/src/jit/AsmJSModule.cpp#197 [3] https://mxr.mozilla.org/mozilla-central/source/js/src/jit/AsmJSModule.cpp#248

(In reply to Luke Wagner [:luke] from comment #50) > Created attachment 807400 [details] [diff] [review] > asmjs-imm-ptr v.2 > > This patch does what I should have done in the first place: just reuse > ma_movPatchable and Assembler::patchDataWithValueCheck. This covers both > MOVWT and LDR. > > Carrying forward jan's and benjamin's r+ since the only changes are in arm. FWIW the ARMv6 builds are still working well after this has landed. For example ammo asm.js demo still runs well. Thank you.

This patch adds caching to the shell (browser in a new bug) in the form of jsapi.h hooks that are implemented in shell/js.cpp. With all the prereq patches in this bug (all of which have landed), the code to implement AsmJSModule::serialize/deserialize is quite simple (by the time I was able to test caching on the awfy/asmjs-apps, everything worked the first time). Most of the work was actually setting up all the testing infrastructure and dealing with Window's non-POSIX-ness. Here's what I settled on: - I added a shell flag --js-cache=path. Caching is off unless this flag is specified. 'path' is a directory in which the caching hooks create a single asmjs.cache file. The idea is that other JS caching (e.g., bug 900789) can use the same directory and share the same test harness infra. - jit_tests.py specifies the dir to be js/src/jit-test/.js-cache and always rm -rf .js-cache before running. - in a parallel build, you don't want all the parallel jobs sharing the same cache. Even if you flock the file so it's thread-safe, you can't assert anything about cache hits/misses because other tasks are clobbering your cache. I also didn't want to have to create/rm a js-cache dir for *every* test. Instead, I added an additional --js-cache-per-process flag. This tells the shell to create/rm a per-pid subdir of the js-cache dir (which it does lazily, i.e., only when asm.js is compiled). - I added bullet as a test. That's why this patch is 1.9MB :) Importantly, runBullet() asserts that the final states of simulated objects. - bullet.js takes like 40 seconds to compile in debug builds, but this was almost all because of graph and regalloc coherence assertions, so I added a TestingFunctions builtin to dynamically disable assertions. With these turned off, it only takes 6 seconds.

Seems to be green on try: https://tbpl.mozilla.org/?tree=Try&rev=37843a63481f

The build error reported in comment 58 is fixed by upgrading to clang 5.0

Comment on attachment 812347 [details] [diff] [review] (shell) caching Review of attachment 812347 [details] [diff] [review]: ----------------------------------------------------------------- This is probably the clearest patch of such a size that I've seen. Every edge case I thought of is handled clearly, and there is nothing interesting for me to report back other than "lgtm". What beautiful code! (For future patches that include a large testcase, it would be nice to leave the testcase as a separate patch, because large patches apparently bring the reviewing interface to a crawl.) ::: js/src/configure.in @@ +2328,1 @@ > # Only run this test with clang on non-Windows platforms, because clang If this test is just busted with clang on all platforms, should the test be only against CLANG_CC? ::: js/src/frontend/Parser.h @@ +642,5 @@ > + // parsing any tokens. It returns the canonical offset to be used as the > + // start of the asm.js module. We use the offset in the char buffer > + // immediately after the "use asm" processing directive statement (which > + // includes any semicolons or newlines that end the statement). > + uint32_t offsetOfAsmJSModule() const { return tokenStream.currentToken().pos.end; } Perhaps "offsetOfCurrentAsmJSModule()"? ::: js/src/shell/js.cpp @@ +3663,5 @@ > + > + // As a special case, if the caller passes "--js-cache", replace that > + // with "--js-cache=$(jsCacheDir)" > + if (!strcmp(argv.back(), "--js-cache")) { > + char *newArg = JS_smprintf("--js-cache=%s", jsCacheDir); Does this break later if the executable filename is "--js-cache"? There are no interesting bugs for me to find in any of this code.

(In reply to Sean Stangl [:sstangl] from comment #72) Thanks! > If this test is just busted with clang on all platforms, should the test be > only against CLANG_CC? Oops, I didn't mean to commit this. It's only necessary to build with CFLAGS="-fPIE" and LDFLAGS="-pie" which is useful for testing ASLR on linux. It may be good to land, but I'd do a separate bug. > Does this break later if the executable filename is "--js-cache"? There are > no interesting bugs for me to find in any of this code. Haha, you are correct sir. This was a bit hackish and I considered a jsCacheDir() shell function but ultimately got lazy...

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5838290c705 I'll take [leave open] off to keep this bug scoped to the JS engine changes. New bug for Gecko changes to implement the AsmJSCacheOps in the browser.

Backed out for rootanalysis bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=29160676&tree=Mozilla-Inbound in https://hg.mozilla.org/integration/mozilla-inbound/rev/65262f9e7580

I'm fairly certain the rootanalysis bustage is unrelated (this patch only touches the asm.js compilation path and the failing test contains no asm.js). Unfortunately I can't reproduce locally. However, sfink says that the whole rootanalysis test is about to change in a day or two, so I'll sit tight until then.

Terrence says he fixed a ggc bug hit by splice-check-steps.js and sfink has evidence of a green try push with my patch, so I'll push again; hopefully no more sleeping dragons: https://hg.mozilla.org/integration/mozilla-inbound/rev/f75226d2273f