With compartment-per-global, a jit-compiled JSScript is only ever run with a single global so jits have all they need optimize global access without any unnecessary guarding. Not only would this remove complexity (and interpreter opcodes) but this change is also necessary to be able to share the bytecode of a script between compartments (see dependent bug).

Yay! I think inference information would want to live in the same place as jitcode, which would also remove a lot of complexity and help precision a lot.

As of Bug 883154 comment 3, I'll take this bug to improve the transition time within Gaia application as they are likely to switch to iframes as way have a coarse grain memory management of panels [1]. As they want to prevent cross-compartment references, to avoid easy mistakes and simplify reviews, they need the platform to reload scripts in a fast way, and ideally without using more memory. Removing COMPILE_N_GO should enable Bug 679940 on all scripts. [1] https://groups.google.com/forum/?fromgroups=#!searchin/mozilla.dev.gaia/Proposition$20of$20an$20architectural$20change$20for$20Gaia$20apps|sort:relevance/mozilla.dev.gaia/Ls9U-CScxWA/1bcCvlG41U0J

The original biggest dependence on compileAndGo I know of was JSOP_(GET|SET)GLOBAL opcodes which baked in, at bytecode-compile-time, the slot in the global object to load from. These ops have been removed and replaced by the JSOP_(GET|SET)GNAME opcodes which are basically name lookups where we assume the lookup happens on the global object (and not any object before it on the scope chain) which we optimize using the global's TI to do efficient loads/stores. So half of this bug's goals are already done. Removing compileAndGo would break the GNAME assumption, though, if a JSScript was ever run again with a scope != script->global(). Event handlers are classically an example of this, but bz has plans to for how we can avoid doing this. With that fixed, I wonder: do we have any other cases where we need to run a JSScript with anything other than script->global (for global code) or fun->environment (for function code)? Said differently, could we remove the 'obj' parameter of JS_(Evaluate|Execute|Compile)Script (or repurpose it to mean only 'thisObj') as well as the 'parent' parameter to JS_CloneFunctionObj? (I'm guessing 'no, XBL!'. Can we handle XBL with a similar 'with' trick as event handlers?) If we could have this, then, effectively, *every* script would be compileAndGo and fully optimized (except for, ya know, the ones with all the 'with' ;). What a world it could be.

So the main things that are run across globals are XBL and XUL prototype stuff, right? The idea there is to have a compiled version, even saved/loaded with XDR, that can then be run across various globals. The goal being to compile it only once. Given bug 679940 is fixed, does that mean we can XDR only the shareable bits and then (cheaply) recreate the global-specific part as needed?

(In reply to Boris Zbarsky [:bz] from comment #4) > So the main things that are run across globals are XBL and XUL prototype > stuff, right? For the XUL prototype cache, we are actually (as a hack in JS_ExecuteScript) cloning the JSScript if obj->compartment != script->compartment(). Conceivably, we could remove this hack and do explicit cloning in the caller which would I think uphold the everything-is-compileAndGo property. I have no idea what XBL is doing. > Given bug 679940 is fixed, does that mean we can XDR only the shareable bits > and then (cheaply) recreate the global-specific part as needed? Yeah, that makes the abovementioned CloneScript cheaper.

What XBL is doing is XDR-ing individual method/getter/setter functions.

(In reply to Boris Zbarsky [:bz] from comment #6) > What XBL is doing is XDR-ing individual method/getter/setter functions. Are those method/getter/setter functions literally functions (produced by JS_DecodeInterpretedFunction)? If so, that's great; that means their scope will be cx->global() and so we have no problem from XBL. Overall, then, it is sounding to me like self-hosting event handler scoping using 'with' and tweaking how CloneScript happens for the XUL proto cache should allow us to assume compileAndGo everywhere. Do you see any other problems?

(In reply to Boris Zbarsky [:bz] from comment #6) > What XBL is doing is XDR-ing individual method/getter/setter functions. For serialization, yes. But it also clones them from the compilation scope into the target scope. Or is that what you're referring to? I'm not sure if JS_CloneFunction* uses XDR under the hood.

(In reply to Bobby Holley (:bholley) from comment #8) Is scope passed to the JS_CloneFunction* call a global object or something else? If it is something else, could we apply the same "random objects on the scope chain using 'with'" self-hosting strategy as event handlers?

(In reply to Luke Wagner [:luke] from comment #9) > (In reply to Bobby Holley (:bholley) from comment #8) > Is scope passed to the JS_CloneFunction* call a global object or something > else? If it is something else, could we apply the same "random objects on > the scope chain using 'with'" self-hosting strategy as event handlers? We definitely want to be cloning the functions in the XBL case. It's important that they run with the principals of the associated XBL scope.

(In reply to Bobby Holley (:bholley) from comment #10) Of course cloning, but I was asking about whether the scope of the cloned function object was cx->global and, if it wasn't, whether we could do the 'with' thing.

(In reply to Luke Wagner [:luke] from comment #11) > (In reply to Bobby Holley (:bholley) from comment #10) > Of course cloning, but I was asking about whether the scope of the cloned > function object was cx->global and, if it wasn't, whether we could do the > 'with' thing. Oh. The scope is generally the bound node, which we depend on in various cases. I don't see any reason why the |with| trick wouldn't work.

> Are those method/getter/setter functions literally functions (produced by > JS_DecodeInterpretedFunction)? For the XDR situation, yes. For the scoping situation XBL has several different kinds of function. 1) Methods. These are installed on the XBL proto, and their scope is whatever xpc::GetXBLScope() is returning. For chrome, I believe this is the global; for content xbl we can hopefully use the with trick? 2) Property getters/setters: same as methods. 3) ctors/dtors: scope is the bound node. With trick should work. 4) event handlers: scope is the event target. With trick should work.

(In reply to Boris Zbarsky [:bz] from comment #6) > What XBL is doing is XDR-ing individual method/getter/setter functions. FYI, anything which is not using compileAndGo at the moment is always executed by the interpreter and never Jitted. And anything which is using XDR at the moment implies that compileAndGo is not set. So, XBL's JavaScript is always running in the interpreter. (In reply to Bobby Holley (:bholley) from comment #8) > For serialization, yes. But it also clones them from the compilation scope > into the target scope. Or is that what you're referring to? I'm not sure if > JS_CloneFunction* uses XDR under the hood. No, it does not. And the way XDR is made, it is used for doing either encoding or decoding but not both at the same time. On the other hand, we have nice comments to keep the Clone functions in-sync with XDR one and the opposite.

(In reply to Boris Zbarsky [:bz] from comment #13) In all these different scoping situation, is the originally-compiled JSFunction being JS_CloneFunctionObject'd once into each scope? (In reply to Nicolas B. Pierron [:nbp] from comment #14) > So, XBL's JavaScript is always running in the interpreter. The baseline jit doesn't require compileAndGo. See for yourself with evaluate("while(true) {}", {compileAndGo:true}).

... of course I mean {compileAndGo:false} :)

> being JS_CloneFunctionObject'd once into each scope? No. Methods and property getters/setters are cloned once per scope (since they live on the proto). ctors/dtors/eventhandlers are cloned once per instance object.

(In reply to Boris Zbarsky [:bz] from comment #17) I didn't mean 'scope' as in XPC's scope as in global, I meant 'scope' as in your use of 'scope' in comment 13. With that meaning of 'scope', it sounds like your answer then is 'yes'?

Ah, yes. In that case the answer is yes for ctors and dtors but not for event handlers. Those are cloned every time the event fires. We could try to fix that...

(In reply to Boris Zbarsky [:bz] from comment #19) Wow, that's a lot of cloning! Cloning isn't particularly fast, it might be a good speedup to minimize cloning. Random question: will DOM mutations change the contents of the scope chain of a single event handler over time? (I don't mean change the variables visible, of course that will change, I mean which and how many DOM objects are on the scope chain.) That could be a problem for 'with'.

> will DOM mutations change the contents of the scope chain of a single event handler over > time? Per spec they're not supposed to. Currently they sometimes do (for the non-XBL case), but in those cases I believe we recompile the handlers anyway.

(In reply to Boris Zbarsky [:bz] from comment #21) > Per spec they're not supposed to. Interesting. So does that mean the spec is saying something like "take a snapshot of the DOM node parent chain at the time the event handler function is created and always use that"?

Yes. Specifically, what it says is this (at http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-handler-content-attributes ): When an event handler content attribute is set, if the element is owned by a Document that is in a browsing context, and scripting is enabled for that browsing context, the user agent must run the following steps to create a script after setting the content attribute to its new value: ... Lexical Environment Scope 1. Let Scope be the result of NewObjectEnvironment(the element's Document, the global environment). 2. If the element has a form owner, let Scope be the result of NewObjectEnvironment(the element's form owner, Scope). 3. Let Scope be the result of NewObjectEnvironment(the element's object, Scope). then you create a new Function, per the rules of "ECMAScript edition 5 section 13.2 Creating Function Objects".

(In reply to Boris Zbarsky [:bz] from comment #23) Cool, thanks for explaining! I suspect this exact definition was motivated by someone who already had done the self-hosting with with :) Since we use the live parent field of the document, I assume we break this now if you manipulate the DOM tree in a way that changes parent links?

We don't change parent links on JS objects once they're created. So no, we don't break this now, unless we give the node a new JSObject altogether. Which in this case only happens on adoption into a different document, and recompiles all the handlers anyway. (All this modulo the fact that we compile handlers lazily, which technically the spec does not allow you to do.)

Ah, thanks for explaining. Overall, then, it sounds like the general strategy of "clone the JSScript when you would have otherwise run the same JSScript with a different scope chain" could work to remove all compileAndGo. Nicolas was right to file bug 920322 as blocking this, though, since there are still some constructs in compileAndGo JSScripts that XDR can't serialize.

So I'm trying to pick this up again (see bug 1095015 with some semi-related things). I'd like to understand the proposal in comment 26, though. How does that allow us to avoid the compileAndGo flag? Say we have a script that we compile; it ends up using JSOP_GETGNAME. Then we want to run against a different scope, clone it... but that won't replace the bytecode, right? So we still have a JSOP_GETGNAME in the bytecode. But the scope chain might now have something with that name on it...

You're right; the invariants implied by GNAME would have to weaken to say "this is an unbound name" instead of "this is a name that resolves to the global object, if at all". At script-clone time, you know the new scope chain and whether it is script->global() or something else, so it seems like we could just set a bit on JSScripts saying "has non-global scope" that effectively turns GNAME ops into NAME ops. There is some recent discussion on this starting in bug 1045529 comment 11. What I don't know (and am hopeful about) is how much simpler the situation is with all the JSAPI changes you've made and are planning.

OK. So here's our plan from discussion with Luke just now: 1) Make bug 1095015 comment 0 item 2 happen. 2) Change JSAPI entry points to release-assert the following invariants that I'm pretty sure hold in Gecko: *) Toplevel scripts are only compiled against a global. *) Toplevel scripts are either executed against a global or an object that's not same-compartment with their compilation global. *) Functions are only cloned into a non-global scope cross-compartment. 3) Add a new flag called hasDynamicScopeChain to scripts. Set it in the API entrypoints that provide a dynamic scope chain, both compilation (for functions) and cloning. Since per item 2 we guarantee that we clone whenever we might be picking up an interesting scope chain, this is good enough. 4) Change the places where we special-case the GNAME ops to only do that if !hasDynamicScopeChain. 5) Stop using the compileAndGo flag for deciding whether to emit GNAME ops. Just emit them, and rely on the hasDynamicScopeChain checks doing the right thing. 6) Eliminate any remaining uses of compileAndGo that are left at that point. I'll start filing some bugs.

The only tweak I'd make is s/hasDynamiScopeChain/hasDynamicGlobalScope/ (or some other name to imply that the weirdness happens between the outermost lexical scope and the global object).

> 6) Eliminate any remaining uses of compileAndGo that are left at that point. These seem to be: 1) CanLazilyParse. This might be fixed once we do the above, since it basically depends on the GNAME behavior produced by CompileLazyFunction, which currently assumes compileAndGo == true. We may want to store hasDynamicGlobalScope on LazyScript. 2) Singleton function/context stuff in the bytecode emitter. Not sure where to go with these. 3) BytecodeEmitter::needsImplicitThis. This might be solved implicitly by making sure we only allow With scopes when we have a non-global on the scope chain.

OK, we're going to need to change the invariants from comment 29 step 2, like so: *) Toplevel scripts are either executed against a global or get cloned before execution. See bug 1143793 comment 7 and bug 1143793 comment 9. We also need to add handling of JSOP_IMPLICITTHIS similar to the GNAME bits described in comment 29.

Comment on attachment 8583153 [details] [diff] [review] part 3. Add a CompileOptions flag for indicating that the script should be compiled runOnce Review of attachment 8583153 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeCompiler.cpp @@ +291,5 @@ > if (!script) > return nullptr; > > + if (options.isRunOnce) > + script->setTreatAsRunOnce(); Since options are passed into JSScript::Create, can you initialize this field inside Create like the other options?

Comment on attachment 8583154 [details] [diff] [review] part 4. Set the isRunOnce compile flag as needed Review of attachment 8583154 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsScriptLoader.cpp @@ +1073,5 @@ > aOptions->setIntroductionType("scriptElement"); > aOptions->setFileAndLine(aRequest->mURL.get(), aRequest->mLineNo); > aOptions->setVersion(JSVersion(aRequest->mJSVersion)); > aOptions->setCompileAndGo(JS_IsGlobalObject(aScopeChain)); > + aOptions->setIsRunOnce(true); I wonder if perhaps we should change the default to 'true' since it's almost always what you want. Errors should be easily caught and fixed.

Comment on attachment 8583155 [details] [diff] [review] part 5. Stop using the compileAndGo script flag in the bytecode emitter Review of attachment 8583155 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.h @@ +246,5 @@ > > bool isInLoop(); > bool checkSingletonContext(); > + // Check whether our function is in a run-once context (a toplevel > + // run-one script or a run-once lambda). Can you put a \n before comment?

Comment on attachment 8583157 [details] [diff] [review] part 7. Drop the now-unused JSScript::compileAndGo Review of attachment 8583157 [details] [diff] [review]: ----------------------------------------------------------------- \o/

> can you initialize this field inside Create like the other options? OK, done. > I wonder if perhaps we should change the default to 'true' Hmm. I suppose we could, but maybe in a followup? I worry about various edge cases popping up... > Can you put a \n before comment? Done.

Comment on attachment 8583158 [details] [diff] [review] part 8. Drop the now-unused compileAndGo from CompileOptions Review of attachment 8583158 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeCompiler.cpp @@ +149,3 @@ > !options.hasPollutedGlobalScope && > !cx->compartment()->options().discardSource() && > !options.sourceIsLazy; Since you're touching, can you align the '!' with the 'o' in 'options' on the first line?

(In reply to Not doing reviews right now from comment #46) > > I wonder if perhaps we should change the default to 'true' > > Hmm. I suppose we could, but maybe in a followup? I worry about various > edge cases popping up... A follow-up would be fine. I was thinking that the situation with setIsRunOnce is similar to setHasPollutedScope where we default to the option that has better perf by limiting available JSAPI operations (which have safe runtime checks).

> Since you're touching, can you align the '!' with the 'o' in 'options' on the first line? Done. Also applied most of our discussion on IRC comments. Still waiting on the decision on what we want to do with XDR saving of a runOnce script.

Comment on attachment 8583270 [details] [diff] [review] Part 2 updated to discussion with nbp with the XDR changes Review of attachment 8583270 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +607,5 @@ > > + if (!fun && script->treatAsRunOnce()) { > + // This is a toplevel or eval script that's runOnce. We want to > + // make sure that we're not XDR-saving an object we emitted for > + // JSOP_OBJECT that then gor modified. So throw if we're not s/gor/got/

https://hg.mozilla.org/integration/mozilla-inbound/rev/872edd1aedf7 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3793e00b41f https://hg.mozilla.org/integration/mozilla-inbound/rev/4425a55312c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2d863a3df9 https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc68cf4610b https://hg.mozilla.org/integration/mozilla-inbound/rev/4cbcb40e0ef4 https://hg.mozilla.org/integration/mozilla-inbound/rev/5336ae0fe919 https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1a45bc093a

All backed out for various failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c068f0ce341

So this sucks. These patches are consistently failing an assert in the cgc test run: Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-compacting-000000/src/js/src/jsfun.h:449 TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/testLet.js | Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-compacting-000000/src/js/src/jsfun.h:449 (code -11, args "--ion-eager --ion-offthread-compile=off") INFO stderr 2> Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-compacting-000000/src/js There's no stack, unfortunately, like usual for the JS tests. I can't reproduce locally. I also can't reproduce running on a borrowed slave (!). sfink can't reproduce on his slave. I tried adding some printfs to at least see which of the mutableScript() callers is responsible, but then the cgc tests pass. So I really have no idea where to go from here. Certainly given the amount of time I actually have to spend on this.... :(

(In reply to Not doing reviews right now from comment #54) > I can't > reproduce locally. I also can't reproduce running on a borrowed slave (!). > sfink can't reproduce on his slave. I've had similar issues with the SM(r) builds, they can be very fragile. Sometimes the problem is "gone" after rebasing patches on a newer inbound revision; maybe that's worth a try. If it's possibly related to your patches, you could also request fuzzing or bisect on Try...

So this reproduces for me on Linux (I ran testLet.js with jit_test.py --tbpl and it fails with --baseline-eager and --baseline-eager --no-fpu, so different flags than the failures you got). Anyway here's the stack trace, let me know if you need more and I'd be happy to do some remote debugging on IRC.

Jan, thank you! That's perfect.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5301bef40a https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf1d6c78f97 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bde79bfc439 https://hg.mozilla.org/integration/mozilla-inbound/rev/c538f12b63bd https://hg.mozilla.org/integration/mozilla-inbound/rev/796283553115 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7948f507a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/fafbabc6147b https://hg.mozilla.org/integration/mozilla-inbound/rev/f25e7176b9d6 and now it looks like it has stuck.

https://hg.mozilla.org/mozilla-central/rev/fe5301bef40a https://hg.mozilla.org/mozilla-central/rev/6cf1d6c78f97 https://hg.mozilla.org/mozilla-central/rev/4bde79bfc439 https://hg.mozilla.org/mozilla-central/rev/c538f12b63bd https://hg.mozilla.org/mozilla-central/rev/796283553115 https://hg.mozilla.org/mozilla-central/rev/4c7948f507a9 https://hg.mozilla.org/mozilla-central/rev/fafbabc6147b https://hg.mozilla.org/mozilla-central/rev/f25e7176b9d6