Closed Bug 851421 Opened 11 years ago Closed 11 years ago

Don't emit bytecode for asm.js functions unless linking fails

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [gdc2013])

Attachments

(2 files, 5 obsolete files)

clone extended slots
4.32 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
(part 2) - Don't emit bytecode for asm.js functions unless linking fails.
61.70 KB, patch
luke
: review+
Details | Diff | Splinter Review 
We can speed up asm.js a lot if we don't emit bytecode unless we have to, i.e.
if linking fails.
This draft patch works on simple cases where linking succeeds.
Comment on attachment 725273 [details] [diff] [review]
(DRAFT) Don't emit bytecode for asm.js functions unless linking fails.

Luke, any feedback would be welcome.  I'll return to this on Monday.
Attachment #725273 - Flags: feedback?(luke)
Comment on attachment 725273 [details] [diff] [review]
(DRAFT) Don't emit bytecode for asm.js functions unless linking fails.

It looks like you're on the right track.

One thing: can we avoid calling into EmitFunctionScript at all?  Given that we're changing the funbox->function() to be a native, it seems like we wouldn't have any use for a script.
Attachment #725273 - Flags: feedback?(luke) → feedback+
> One thing: can we avoid calling into EmitFunctionScript at all?  Given that
> we're changing the funbox->function() to be a native, it seems like we
> wouldn't have any use for a script.

That's the part of the patch I was least certain about.  I'll try avoiding it and see what happens.
I'm working now on handling linking failures.  I've got to the point where I have the source code for the asm.js function;  now I need to parse and compile it.  This task overlaps a lot with bug 678037... bhackett, I'm thinking about stealing CompileLazyFunction() from that bug's patch -- is that a good idea?
Flags: needinfo?(bhackett1024)
(In reply to Nicholas Nethercote [:njn] from comment #5)
> I'm working now on handling linking failures.  I've got to the point where I
> have the source code for the asm.js function;  now I need to parse and
> compile it.  This task overlaps a lot with bug 678037... bhackett, I'm
> thinking about stealing CompileLazyFunction() from that bug's patch -- is
> that a good idea?

CompileLazyFunction depends on a fair bit else in that patch and from bug 678037's dependent bugs in order to behave correctly.  So I would recommend waiting for bug 678037 to land.  If linking fails, is the parser still on the stack?  If so it seems you could just seek() the token stream back to the beginning of the asm.js function and reparse everything from scratch.
Flags: needinfo?(bhackett1024)
Given that we have a much simpler problem here (no upvars), it seems like we shouldn't need to reuse much machinery other than just CompileFunctionBody.
Whiteboard: [gdc2013]
This patch causes SpiderMonkey to only emit bytecode for asm.js functions if
linking fails.  

For normal functions, we also avoid emitting a script for asm.js functions;
for functions created with Function() we still emit the script because the
control flow makes it harder to avoid, though with some effort it should be
doable (look for the "XXX" comment).

The patch removes JSOP_LINKASMJS and JSScript::asmJS.

I've used the name ASM_MODULE_FUNCTION_MODULE_SLOT for the extended slot, which
is a horrible name.  We already have ASM_MODULE_SLOT for functions within the
asm.js module.  We should probably rename the latter to better distinguish the
asm.js module function from the functions within it, but I'm not sure what the
exact terminology is.

I changed some offsets from size_t to uint32_t, which is the correct type for
them.

I added a test that at one point was causing an assertion failure when none of
the existing tests were.

The patch passes jit-tests and jstests in the shell.  I haven't tried it in a
browser build yet.
Attachment #727560 - Flags: review?(luke)
Attachment #725273 - Attachment is obsolete: true
Comment on attachment 727560 [details] [diff] [review]
Don't emit bytecode for asm.js functions unless linking fails.

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

This patch is great, nice style, and I think you've found all the right cut points.  A couple of non-nits below, so I'll clear r? and take another pass after they are fixed:

::: js/src/frontend/BytecodeCompiler.cpp
@@ +370,5 @@
>      argsbody->makeEmpty();
>      fn->pn_body = argsbody;
>  
> +    // XXX: ideally, standaloneFunctionBody() wouldn't need a JSScript, and
> +    // then this could be skipped when the function is asm.js code.

IIUC, this script just becomes a small amount of garbage and so this isn't a huge loss.  In that case, it seems like we could leave off the comment.

@@ +422,5 @@
> +            return false;
> +
> +        if (moduleObj) {
> +            generateBytecode = false;
> +            fun.set(link);      // replace the existing function with the LinkAsmJS native

Could you change CompileAsmJS to *only* export the function (perhaps renamed moduleFun); it seems moduleObj is only used as a boolean, so we could just test whether moduleFun was non-null instead.

::: js/src/ion/AsmJS.cpp
@@ +4991,5 @@
> +    if (!link)
> +        return false;
> +
> +    link->setExtendedSlot(ASM_MODULE_FUNCTION_MODULE_SLOT, OBJECT_TO_JSVAL(moduleObj));
> +    funbox->object = link;

I think we can remove the 'funbox' argument by having the caller just setting the exported 'link' function to funbox->object.  This is something you want to see at the callsite (to know that the mutation occurred) and you already assert this equality at one callsite, so it'd be simpler just to do it.

The uses of 'funbox' and 'fun' which is derived from it can be replaced with:
 - fun->atom() can be replaced with FunctionName(fn) (which is generally used all over AsmJS.cpp)
 - fun->getParent() isn't necessary and we can remove the 'parent' from the PostLinkFailureInfo; just use cx->global() to create the new function in LinkAsmJS.
 - fun->fun->nargs can be replaced with FunctionArgsList(fn, &numFormals) (or, add a FunctionNumFormals helper right next to FunctionArgsList that does the obvious thing)

::: js/src/ion/AsmJSLink.cpp
@@ +372,5 @@
>  
> +    // If linking fails, recompile the function (including emitting bytecode)
> +    // as if it's normal JS code.
> +    if (!DynamicallyLinkModule(cx, args, module)) {
> +        if (cx->isExceptionPending())

Could you hoist this whole failure branch into a new static function named something like DynamicLinkFailure?

@@ +379,5 @@
> +        const AsmJSModule::PostLinkFailureInfo &info = module.postLinkFailureInfo();
> +
> +        uint32_t length = info.bufEnd_ - info.bufStart_;
> +        const jschar *chars =
> +            info.scriptSource_->substring(cx, info.bufStart_, info.bufEnd_)->chars();

This JSFlatString returned by this function isn't otherwise rooted so, to be safe, I think you need a RootedFlatString.

@@ +384,5 @@
> +
> +        RootedObject parent(cx, info.parent_);
> +        Rooted<PropertyName*> name(cx, info.name_);
> +        RootedFunction fun(cx, NewFunction(cx, NullPtr(), NULL, 0, JSFunction::INTERPRETED,
> +                                           parent, name));

You can use cx->global() instead of 'parent' here.  Also, isntead of saving info.name_, I think you can just use fun->name() (that is, the name of the LinkAsmJS native should be what you need).

@@ +388,5 @@
> +                                           parent, name));
> +        if (!fun)
> +            return false;
> +
> +        AutoNameVector formals(cx);

Perhaps, to be shorter, you could .reserve(3) once and then do infallibleAppends?  Your call.

@@ +403,5 @@
> +                return false;
> +        }
> +
> +        frontend::CompileFunctionBody(cx, &fun, info.options_, formals, chars, length,
> +                                      /* isAsmJSRecompile = */ true);

Need failure-check here.

::: js/src/ion/AsmJSModule.h
@@ +35,5 @@
>  };
>  
> +// When an asm.js module function is compiled to a native, it holds a reference
> +// to the AsmJSModule in an extended slot.
> +static const unsigned ASM_MODULE_FUNCTION_MODULE_SLOT = 0;

Could you instead make a new function AsmJSModuleFunctionToModule and put it next to AsmJSModuleObjectToModule (further down)?  This avoids exposing the slot implementation detail.

@@ +267,5 @@
> +    struct PostLinkFailureInfo
> +    {
> +        CompileOptions      options_;
> +        HeapPtrPropertyName name_;
> +        HeapPtrObject       parent_;

I think you need a PostLinkFailureInfo::trace to Mark these, called by AsmJSModule::trace.

::: js/src/jsapi.cpp
@@ -4833,5 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_CANT_CLONE_OBJECT);
>          return NULL;
>      }
>  
> -    if (fun->hasScript() && fun->nonLazyScript()->asmJS) {

If the new LinkAsmJS native function flows through here, then, tracing through CloneFunctionObject, it'll get cloned into a new LinkAsmJS native function whose extended slots are 'undefined' which is a bad state.  Instead, could you explicitly filter for fun->isNative() && fun->native() == LinkAsmJS?
Attachment #727560 - Flags: review?(luke)
> A couple of non-nits below, so I'll clear r? and take another pass
> after they are fixed:

Feel free to r- me in that case... or if that feels to harsh, you can change it to an f+ :)

Thanks for the detailed review!  I'm working on your feedback, and that from try server, now.
Comment on attachment 727560 [details] [diff] [review]
Don't emit bytecode for asm.js functions unless linking fails.

For such a nice patch, r- wasn't appropriate.  But using f+ in such cases is a good idea.
Attachment #727560 - Flags: feedback+
> Could you instead make a new function AsmJSModuleFunctionToModule and put it
> next to AsmJSModuleObjectToModule (further down)?  This avoids exposing the
> slot implementation detail.

I couldn't get the references to work such that I could use a single function
for both getting and setting.  So I now have AsmJSModuleObject() and
SetAsmJSModuleObject().


> +    struct PostLinkFailureInfo
> +    {
> +        CompileOptions      options_;
> +        HeapPtrPropertyName name_;
> +        HeapPtrObject       parent_;
>
> I think you need a PostLinkFailureInfo::trace to Mark these, called by
> AsmJSModule::trace.

No longer!  You helped me remove the |name_| and |parent_| members :)


I addressed all your other comments.  I also modified stubs::DefFun in a
similar fashion to DefFunOperation.  This came up when testing with --no-ion
--always-mjit.

The patch passes unit tests, but crashes reliably when running citadel.  The
crash occurs in CloneFunctionObjectIfNotSingleton().  I've put a comment in
there explaining what goes wrong.  I don't know what's happening, though the
fact that we reject asm.js module functions in JS_CloneFunction() seems
relevant.  Here's the crashing stack trace:


Program received signal SIGSEGV, Segmentation fault.
0x00007ffff583b703 in js::CloneFunctionObjectIfNotSingleton (
    cx=0x7fffda6b2040, fun=..., parent=...)
    at /home/njn/moz/mi6/js/src/jsfuninlines.h:183
183                 script->hasBeenCloned = true;
(gdb) bt
#0  0x00007ffff583b703 in js::CloneFunctionObjectIfNotSingleton (
    cx=0x7fffda6b2040, fun=..., parent=...)
    at /home/njn/moz/mi6/js/src/jsfuninlines.h:183
#1  0x00007ffff583b3e4 in js::Lambda (cx=0x7fffda6b2040, fun=..., parent=...)
    at /home/njn/moz/mi6/js/src/jsinterp.cpp:3421
#2  0x00007ffff5832fde in js::Interpret (cx=0x7fffda6b2040, 
    entryFrame=<optimised out>, 
    interpMode=<error reading variable: Cannot access memory at address 0x2>)
    at /home/njn/moz/mi6/js/src/jsinterp.cpp:2740
#3  0x00007ffff582ec3a in js::RunScript (cx=<optimised out>, 
    fp=<optimised out>) at /home/njn/moz/mi6/js/src/jsinterp.cpp:341
#4  0x00007ffff5838d11 in js::ExecuteKernel (cx=0x7fffda6b2040, 
    scopeChainArg=..., thisv=..., type=<optimised out>, result=0x0, 
    script=..., evalInFrame=...) at /home/njn/moz/mi6/js/src/jsinterp.cpp:531
#5  0x00007ffff5838e39 in js::Execute (cx=0x7fffda6b2040, scopeChainArg=..., 
    rval=0x0, script=...) at /home/njn/moz/mi6/js/src/jsinterp.cpp:570
#6  0x00007ffff57ca26f in JS::Evaluate (cx=0x7fffda6b2040, options=..., 
    chars=<optimised out>, length=<optimised out>, rval=0x0, obj=...)
    at /home/njn/moz/mi6/js/src/jsapi.cpp:5568
#7  0x00007ffff47ac5cc in nsJSContext::EvaluateString (this=0x7fffd9b9e510, 
    aScript=..., aScopeObject=..., aOptions=..., aCoerceToString=false, 
    aRetValue=0x0) at /home/njn/moz/mi6/dom/base/nsJSEnvironment.cpp:1297
#8  0x00007ffff454e8c4 in nsScriptLoader::EvaluateScript (
    this=<optimised out>, aRequest=<optimised out>, aScript=...)
    at /home/njn/moz/mi6/content/base/src/nsScriptLoader.cpp:847
#9  0x00007ffff454e499 in nsScriptLoader::ProcessRequest (
    this=0x7fffd95421d0, aRequest=0x7fffd9a89880)
    at /home/njn/moz/mi6/content/base/src/nsScriptLoader.cpp:737
#10 0x00007ffff454ebbf in nsScriptLoader::ProcessPendingRequests (
    this=0x7fffd95421d0)
    at /home/njn/moz/mi6/content/base/src/nsScriptLoader.cpp:879
#11 0x00007ffff454f2a8 in nsScriptLoader::OnStreamComplete (
    this=0x7fffd95421d0, aLoader=<optimised out>, aContext=<optimised out>, 
    aStatus=<optimised out>, aStringLen=<optimised out>, 
    aString=<optimised out>)
Attachment #728084 - Flags: review?(luke)
Attachment #727560 - Attachment is obsolete: true
Comment on attachment 728084 [details] [diff] [review]
Don't emit bytecode for asm.js functions unless linking fails.

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

This looks excellent; would be done except for the issue you uncovered and my resulting comment below.  I'll help in determining the right fix.

::: js/src/ion/AsmJS.h
@@ +50,5 @@
> +// exception thrown when executing GetProperty on the arguments) is pending.
> +extern JSBool
> +LinkAsmJS(JSContext *cx, unsigned argc, JS::Value *vp);
> +
> +bool IsNativeAsmJSModule(JSFunction *fun);

Could you add a short comment explaining what a "NativeAsmJSModule" is?

::: js/src/jsfuninlines.h
@@ +182,5 @@
> +
> +        // njn: crash because |script| is NULL here;  in a debug build,
> +        // getOrCreateScript() would assert at the JS_ASSERT(hasScript()).
> +        // fun->flags looks bogus, taking on different values each run, such as
> +        // 0xd3b6 and 0xdb60.

From the callstack, we're coming from a JSOP_LAMBDA which is what you'd get if you execute the statement:
  var f = (function() { "use asm" ... });
Incidentally, this is what Emscripten generates in newer builds which is why I think you're seeing this.  Thus, we're in the same situation as with JSOP_DEFFUN.

The fix is to ensure that this LinkAsmJS function does not have a singleton type.  I'll look into that...

::: js/src/jsinterp.cpp
@@ +3453,5 @@
>       * windows, and user-defined JS functions precompiled and then shared among
>       * requests in server-side JS.
>       */
>      RootedFunction fun(cx, funArg);
> +    if (fun->isInterpreted() && fun->environment() != scopeChain) {

I think we actually *should* call CloneFunctionObjectIfNotSingleton here: each evaluation of a function statement should yield a new object (whose identity is observable with === or by adding properties).  Once we make the aforementioned fix (making sure LinkAsmJS does not have a singleton type), I think this won't be necessary.  (Same goes for stubs::DefFun.)
> From the callstack, we're coming from a JSOP_LAMBDA which is what you'd get
> if you execute the statement:
>   var f = (function() { "use asm" ... });
> Incidentally, this is what Emscripten generates in newer builds which is why
> I think you're seeing this.  Thus, we're in the same situation as with
> JSOP_DEFFUN.
> 
> The fix is to ensure that this LinkAsmJS function does not have a singleton
> type.  I'll look into that...

Great, thanks.  It's Saturday here so my hacking time is very limited until Monday.  But my try run from yesterday shows there's one remaining build failure on Android, so I'll try to work on that while you work on this.


> I think we actually *should* call CloneFunctionObjectIfNotSingleton here:
> each evaluation of a function statement should yield a new object (whose
> identity is observable with === or by adding properties).  Once we make the
> aforementioned fix (making sure LinkAsmJS does not have a singleton type), I
> think this won't be necessary.  (Same goes for stubs::DefFun.)

Ok.  Once you have it working, if you could post a new patch that applies on top of mine that would be great.  Then I can combine the two, add a test for the JSOP_LAMBDA case, do the other minor fixes, do another try run, and land!  Probably on Monday (my time).
The Android failures are because I've broken --disable-ion -- IsAsmJSModuleFunction() is defined in Ion-only code.  I'll wait to see how you deal with the CloneObject stuff before I fix it, because there might be some overlap.
This patch can apply below/independent of all the patches in this bug.  It fixes the basic problem that calling CloneFunctionObject on a FunctionExtended does not copy the extended slots.  In the cross-compartment clone case, I kept the meaning the same as before.

Pushed just this patch to try: https://tbpl.mozilla.org/?tree=Try&rev=6a0c3b9c0dea
Attachment #728548 - Flags: review?(wmccloskey)
Attached patch tweaks on top of njn's patch (obsolete) — — Splinter Review 
This patch does the tweaks I was alluding to in the above comments: it clones the LinkAsmJS native on each execution just like normal functions.  I also added a test for some of these identity properties.

Notes:
 - CloneFunctionObjectIfSingleton that all singletons are scripted functions so I added an exception in NewFunction to keep LinkAsmJS from getting a singleton type.
 - I fixed an assertion found by the testcase I added.
Attached patch Final part. (obsolete) — — Splinter Review 
Luke, this applies on top of your patch.  It fixes up the --disable-ion
bustage.  I'll fold them all together before landing.

The first patch is still in the r? state, too.

I've started a try run with all the patches at
https://tbpl.mozilla.org/?tree=Try&rev=2aac7ea39dc6.
Attachment #728596 - Flags: review?(luke)
Luke, your patch is causing this on try:

jsfun.obj : error LNK2005: "public: static enum js::gc::AllocKind const JSFunction::FinalizeKind" (?FinalizeKind@JSFunction@@2W4AllocKind@gc@js@@B) already defined in jsalloc.obj

jsfun.obj : error LNK2005: "public: static enum js::gc::AllocKind const JSFunction::ExtendedFinalizeKind" (?ExtendedFinalizeKind@JSFunction@@2W4AllocKind@gc@js@@B) already defined in jsalloc.obj

   Creating library mozjs.lib and object mozjs.exp

mozjs.dll : fatal error LNK1169: one or more multiply defined symbols found


I tried removing the definitions from jsfun.cpp and have started another (build-only) try run:  https://tbpl.mozilla.org/?tree=Try&rev=4407bfc96aa8
> jsfun.obj : error LNK2005: "public: static enum js::gc::AllocKind const
> JSFunction::FinalizeKind" (?FinalizeKind@JSFunction@@2W4AllocKind@gc@js@@B)
> already defined in jsalloc.obj
> 
> I tried removing the definitions from jsfun.cpp and have started another
> (build-only) try run:  https://tbpl.mozilla.org/?tree=Try&rev=4407bfc96aa8

That worked.  Now we have another test failure:  testModuleFunctions.js is asserting with --ion-eager.  Stack trace is below;  it's deep in IonMonkey and thus above my paygrade.


warning: Successfully compiled asm.js code
warning: Successfully compiled asm.js code
warning: Successfully compiled asm.js code
Assertion failure: hasScript(), at ../../jsfun.h:215

Program received signal SIGSEGV, Segmentation fault.
JSFunction::nonLazyScript (this=0x7ffff6343b20) at ../../jsfun.h:215
215	        JS_ASSERT(hasScript());
(gdb) bt
#0  JSFunction::nonLazyScript (this=0x7ffff6343b20) at ../../jsfun.h:215
#1  0x0000000000d06e66 in js::ion::CodeGenerator::emitLambdaInit (
    this=0xf52670, output=..., scopeChain=..., fun=0x7ffff6343b20)
    at /home/njn/moz/mi6/js/src/ion/CodeGenerator.cpp:596
#2  0x0000000000d06c77 in js::ion::CodeGenerator::visitLambda (this=0xf52670, 
    lir=0xfe5600) at /home/njn/moz/mi6/js/src/ion/CodeGenerator.cpp:571
#3  0x0000000000ba0b06 in js::ion::LLambda::accept (this=0xfe5600, 
    visitor=0xf52670) at /home/njn/moz/mi6/js/src/ion/LIR-Common.h:2396
#4  0x0000000000d10015 in js::ion::CodeGenerator::generateBody (this=0xf52670)
    at /home/njn/moz/mi6/js/src/ion/CodeGenerator.cpp:2190
#5  0x0000000000d1e82f in js::ion::CodeGenerator::generate (this=0xf52670)
    at /home/njn/moz/mi6/js/src/ion/CodeGenerator.cpp:4469
#6  0x0000000000a9c41d in js::ion::GenerateLIR (mir=0xf36808, maybeMasm=0x0)
    at /home/njn/moz/mi6/js/src/ion/Ion.cpp:1106
#7  0x0000000000a9c6eb in js::ion::CompileBackEnd (mir=0xf36808, 
    maybeMasm=0x0) at /home/njn/moz/mi6/js/src/ion/Ion.cpp:1120
#8  0x0000000000a9c919 in js::ion::SequentialCompileContext::compile (
    this=0x7fffffff6658, builder=0xf36808, graph=0xf36748, autoDelete=...)
    at /home/njn/moz/mi6/js/src/ion/Ion.cpp:1315
#9  0x0000000000aa66a5 in js::ion::IonCompile<js::ion::SequentialCompileContext> (cx=0xf3b3c0, script=0x7ffff6337340, fun=0x7ffff6345280, osrPc=0x0, 
    constructing=false, compileContext=...)
    at /home/njn/moz/mi6/js/src/ion/Ion.cpp:1248
#10 0x0000000000a9d5d6 in js::ion::Compile<js::ion::SequentialCompileContext>
    (cx=0xf3b3c0, script=..., fun=..., osrPc=0x0, constructing=false, 
    compileContext=...) at /home/njn/moz/mi6/js/src/ion/Ion.cpp:1468
#11 0x0000000000a9daf4 in js::ion::CanEnter (cx=0xf3b3c0, 
    script=0x7ffff6337340, fp=..., isConstructing=false)
    at /home/njn/moz/mi6/js/src/ion/Ion.cpp:1567
#12 0x00000000005d5321 in js::RunScript (cx=0xf3b3c0, fp=0x7ffff6502140)
    at /home/njn/moz/mi6/js/src/jsinterp.cpp:314
(In reply to Nicholas Nethercote [:njn] from comment #19)
> Luke, your patch is causing this on try:
> 
> jsfun.obj : error LNK2005: "public: static enum js::gc::AllocKind const
> JSFunction::FinalizeKind" (?FinalizeKind@JSFunction@@2W4AllocKind@gc@js@@B)
> already defined in jsalloc.obj

Lame; I added them because of the opposite link error on linux.  What seems to happen is that if you use a static const initialized variable in a ?: expression, GCC emits an external reference (instead of just inlining the constant) which wants a definition.  I thought this was required and I don't understand why this would cause a linker error (see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42101).

(In reply to Nicholas Nethercote [:njn] from comment #20)
> That worked.  Now we have another test failure:  testModuleFunctions.js is
> asserting with --ion-eager.  Stack trace is below;  it's deep in IonMonkey
> and thus above my paygrade.

I'll have a look; I think I just need to bail Ion compilation in the case of a JSOP_LAMBDA (or JSOP_DEFFUN) for a native function.
Comment on attachment 728596 [details] [diff] [review]
Final part.

I think it made more sense when the predicate was on the JSFunction*.  Can you put just have:

#if JS_ASMJS
bool
IsAsmJSModule(JSFunction *fun);
#else
static inline bool
IsAsmJSModule(JSFunction *fun)
{
    return false;
}
#endif

in AsmJS.h?  Which would allow removal of the #ifdef from the definition in AsmJSLink.cpp and allow putting the definition inside the #ifdef JS_ASMJS like the rest of the code.
Attachment #728596 - Flags: review?(luke) → review?
> I think it made more sense when the predicate was on the JSFunction*. 

But in NewFunction() we have just the native, not the JSFunction...
Ion assert fixed with:

diff --git a/js/src/ion/IonBuilder.cpp b/js/src/ion/IonBuilder.cpp
--- a/js/src/ion/IonBuilder.cpp
+++ b/js/src/ion/IonBuilder.cpp
@@ -6918,16 +6918,18 @@ IonBuilder::jsop_object(JSObject *obj)
 bool
 IonBuilder::jsop_lambda(JSFunction *fun)
 {
     JS_ASSERT(script()->analysis()->usesScopeChain());
     if (fun->isArrow())
         return abort("bound arrow function");
+    if (IsNativeAsmJSModule(fun))
+        return abort("asm.js module function");
 
@@ -6952,16 +6954,18 @@ IonBuilder::jsop_defvar(uint32_t index)
 bool
 IonBuilder::jsop_deffun(uint32_t index)
 {
     RootedFunction fun(cx, script()->getFunction(index));
+    if (IsNativeAsmJSModule(fun))
+        return abort("asm.js module function");
Comment on attachment 728596 [details] [diff] [review]
Final part.

Oops, I missed that.  Carry on (but could you put IsAsmAsmJSModule (perhaps we could namve is IsAsmJSModuleNative?) in AsmJS.h/AsmJS.cpp as described?
Attachment #728596 - Flags: review? → review+
> (but could you put IsAsmAsmJSModule (perhaps
> we could namve is IsAsmJSModuleNative?) in AsmJS.h/AsmJS.cpp as described?

Will do.  And you need to go to bed, your typo rate is through the roof :)
I've folded all the patches together.  This should hopefully be the final
version.  It contains all fixes and all requested changes.  A try run is going
at https://tbpl.mozilla.org/?tree=Try&rev=f08d56f2d997.

BTW, I measured Citadel start-up.  On my Linux64 box, it reduces the time taken
to show the world from about 38 seconds to about 27 seconds, and reduces
"explicit" memory consumption at that point from 1858 MiB to 1749 MiB.  That
matches fairly well with the measurements Luke took.
Attachment #728615 - Flags: review?(luke)
Attachment #728084 - Attachment is obsolete: true
Attachment #728084 - Flags: review?(luke)
Attachment #728551 - Attachment is obsolete: true
Attachment #728596 - Attachment is obsolete: true
BTW, the "clone extended slots" patch looks fine to me, I'm happy to give r+ if you're happy to take it.
Attachment #728548 - Flags: review?(wmccloskey) → review?(n.nethercote)
Comment on attachment 728615 [details] [diff] [review]
(part 2) - Don't emit bytecode for asm.js functions unless linking fails.

Great!
Attachment #728615 - Flags: review?(luke) → review+
(In reply to Nicholas Nethercote [:njn] from comment #26) 
> Will do.  And you need to go to bed, your typo rate is through the roof :)

lol, so it was :)

(In reply to Nicholas Nethercote [:njn] from comment #27)
> to show the world from about 38 seconds to about 27 seconds, and reduces
> "explicit" memory consumption at that point from 1858 MiB to 1749 MiB.  That
> matches fairly well with the measurements Luke took.

excellent!
Attachment #728548 - Flags: review?(n.nethercote) → review+
https://hg.mozilla.org/mozilla-central/rev/82ea4e6d7349
https://hg.mozilla.org/mozilla-central/rev/30b977b2b911
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 854212
After this landed, I'm unable to build a debug linux x64 shell or browser. It fails during linking on the following code introduced in this commit:

../libjs_static.a(jsinterp.o): In function `js::CloneFunctionObjectIfNotSingleton(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>)':
/.../mozilla-inbound/js/src/jsfuninlines.h:194: undefined reference to `JSFunction::ExtendedFinalizeKind'
/.../mozilla-inbound/js/src/jsfuninlines.h:194: undefined reference to `JSFunction::FinalizeKind'
/usr/bin/ld: js: hidden symbol `_ZN10JSFunction12FinalizeKindE' isn't defined
> After this landed, I'm unable to build a debug linux x64 shell or browser.

I'm about to post a fix for this in bug 854212.
Depends on: 857700
Depends on: 878520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: