1143793 - Drop support for non-global compilation scopes when compiling toplevel scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now the Compile and JS_Compile*Script APIs take an obj argument.  In practice, in Gecko, this is the global or null.

This argument is used for the following things (see frontend::CompileScript):

1)  Setting the scopeChain_ member of GlobalSharedContext.  This is only used to detect being inside a With scope, so null and global have the same behavior here.

2)  Setting the hasGlobalScope member of BytecodeEmitter.  As far as I can tell this only matters when the script is compileAndGo (and in particular, the one other use is to set up a new BytecodeEmitter, using a JSScript with the same compile-and-go flag as the script of the original bce).  Null and global would be equivalent here if, for now, all the callers that pass null passed the global and did not set compileAndGo.

3)  As an argument to MaybeCheckEvalFreeVariables which assumes that argument is non-null (but also bails out early if !evalCaller, which is why I expect it all works our right now).

So it looks to me like we can switch the people who pass null to passing the global (after ensuring they don't set compile-and-go as needed) and then nix the argument entirely, since the global lives on cx.

This is basically the implementation (or at least start of it) of bug 679939 comment 29 step 2 first asterisk: toplevel scripts are only compiled against a global.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, so as usual the subscript loader is the problem child.  If we end up in ReadScript, we can end up compiling against some non-global object (target_obj, which afaict can be non-global even in the !reuseGlobal case) and then evaluating in the same compartment.

I guess we could modify our stuff in bug 679939 to set the hasDynamicScopeChain script flag during compilation, if compiling against a non-global?  Or can we somehow make subscriptloader saner?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #1)
> OK, so as usual the subscript loader is the problem child.  If we end up in
> ReadScript, we can end up compiling against some non-global object
> (target_obj, which afaict can be non-global even in the !reuseGlobal case)

It's an option in the API, though for now it looks like it's only used in test_chrometoSource.xul. We could remove that, but there's still the reuseGlobal case.

> I guess we could modify our stuff in bug 679939 to set the
> hasDynamicScopeChain script flag during compilation, if compiling against a
> non-global?  Or can we somehow make subscriptloader saner?

You mean like "don't allow subscripts to be evaluated against non-globals"? I don't see how we could do that in the context of global reuse.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> We could remove that, but there's still the reuseGlobal case.

The reuseGlobal case uses a function, not a script, right?  So it doesn't have this problem at all.

Basically, can we get to a point for the subscriptLoader where reuseGlobal is true if and only if we plan to execute against a non-global scope?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #3)
> > We could remove that, but there's still the reuseGlobal case.
> 
> The reuseGlobal case uses a function, not a script, right?  So it doesn't
> have this problem at all.
> 
> Basically, can we get to a point for the subscriptLoader where reuseGlobal
> is true if and only if we plan to execute against a non-global scope?

AFAICT, targetObj is initially computed by FindTargetObject, which only returns a non-global in the case of FakeBackstagePass (reuseGlobal stuff). The only other special case seems to be when an explicit scope is set on LoadSubScriptOptions, and per comment 2 that's only test code.

So I'd say "yes".

Comment 5

[Boris Zbarsky [:bzbarsky]]

> though for now it looks like it's only used in test_chrometoSource.xul.

Really?  I see lots of loadSubscript() in the tree and all those pass some target object, no?

Some of them are passing a global, but many are passing whatever random junk.

I guess my real question is why we set reusingGlobal based on the return value of FindTargetObject, not based on what our actual scope chain will look like when executing.  

And I guess bug 1100461 covers some related (but not identical) issues, and even in the case of only scripts it seems like we can have effective "cache poisoning" issues if we compile a script against a global but then read it from cache and execute against a non-global...

I'd really like to understand what the actual constraints are here on when we do/don't want to use the cache and when we want to use a script vs a function.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Not doing reviews right now from comment #1)
> I guess we could modify our stuff in bug 679939 to set the
> hasDynamicScopeChain script flag during compilation, if compiling against a
> non-global?  Or can we somehow make subscriptloader saner?

To be honest, I would  prefer if we could avoid make the bytecode depend on a specific global, as well as avoid having the bytecode object literals being allocated once.

The reason why I think this is desirable is that we are adding a start-up cache which depends on the fact that we are able to save the bytecode after having run it, and we are also going to have more and more workers which might share common pieces of code [1].

Having the bytecode dependent on a specific global offer some optimization that we can do in the front-end, such as baking-in the global and object literals, but this would prevent us from sharing/saving the JSScript content in the future.  Baking-in such constants can be done in the JIT compilers.

I still think having a flag such as OCMPILE_N_GO is a bad thing, but I disagree on what should be the default.

[1] https://wiki.mozilla.org/Gaia/Architecture_Proposal

Comment 7

[Luke Wagner [:luke]]

nbp: IIUC (bz tell me if I'm forgetting something), after CnG-removal, the bytecode would be completely independent of the global, the *only* thing that matters is the to-be-added JSScript::hasDynamicScopeChain flag which determines how/if we optimize the JSOP_GNAME ops in the JITs (and I guess interpreter).

In fact, you could imagine that we *always* started with hasDynamicScopeChain=false and simply cloned (even same-compartment) on demand in ExecuteScript()/CloneFunction() when a non-empty dynamic scope chain was passed.  From this pov, setting hasDynamicScopeChain=true at compile time is just an optimization to avoid an inevitable CloneScript().

Given all this, bz: could we replace the scope argument to JS::CompileScript() with a CompileOptions::willExecuteWithDynamicScopeChain (that we assert on in ExecuteScript)?  Then this loadSubscript case could set the flag to !IsGlobal(target).  Alternatively, I wonder if any of these callers to loadSubscript *really* want target_obj on the scope chain and would be happy with target_obj->compartment()->global().

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm kinda hoping the bytecode is already independent of the global.  What it _is_ dependent on right now is a funky combination of compileAndGo and what the compile-time scope object and its enclosing scopes happen to be.  Ignoring for the moment eval, which is a special snowflake, and the GNAME stuff that we have a solution for via the hasDynamicScopeChain boolean, the main dependency seems to be whether BytecodeEmitter::needsImplicitThis() is true and hence whether JSOP_GETNAME in callContext gets a JSOP_IMPLICITTHIS to go with it.  

Right now we do JSOP_IMPLICITTHIS if !compileAndGo or if there is a DynamicWithObject on the scope chain provided at _compile_ time (as opposed to runtime; I guess compileAndGo claims those must be identical).

> could we replace the scope argument to JS::CompileScript() with a
> CompileOptions::willExecuteWithDynamicScopeChain

I have to ask: how is this different from having the compileAndGo CompileOptions argument, except for the slightly clearer naming?  What's our end goal here?

> I wonder if any of these callers to loadSubscript *really* want target_obj on the scope
> chain

I believe they do, yes.  That's the whole point of loadSubscript.  See documentation at <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader#loadSubScript%28%29>.  The ones who are OK with the global would simply not pass the optional second argument...

Comment 9

[Luke Wagner [:luke]]

(In reply to Not doing reviews right now from comment #8)
> the main dependency seems to be whether BytecodeEmitter::needsImplicitThis() is
> true and hence whether JSOP_GETNAME in callContext gets a JSOP_IMPLICITTHIS
> to go with it.  

Hrm, interesting.  So this is a case where, if we removed compileAndGo, the bytecode *would* depend on the scope chain (by the inclusion or not of JSOP_IMPLICITTHIS).  It seems to me that we could use the same proposed strategy as JSOP_GNAME ops: always emit the IMPLICITTHIS op and use hasDynamicScopeChain to determine whether we have to go slow or not.  Incidentally, jandem and I have been discussing a significant simplification of this whole area; I'd be curious to hear what this might look like after that.

> > could we replace the scope argument to JS::CompileScript() with a
> > CompileOptions::willExecuteWithDynamicScopeChain
> 
> I have to ask: how is this different from having the compileAndGo
> CompileOptions argument, except for the slightly clearer naming?  What's our
> end goal here?

Hah, I asked myself the same thing when writing that which is what led me to the point that we could just CloneScript() on demand and leave compilation oblivious to all of this.  Perhaps we should just start with that; CloneScript() shouldn't even be super-expensive since it already reuses the SharedScriptData of the source script (which includes bytecode and source notes).

If we did that and the implicit-this thing mentioned above, then we'd be left with a pretty simple story: JSAPI compilation does not care about scope or even cx->global() and, from a JIT pov, hasDynamicScopeChain (perhaps hasNonSyntacticGlobalScope, since the chain isn't totally dynamic...) fills the same role as compileAndGo.

> > I wonder if any of these callers to loadSubscript *really* want target_obj on the scope
> > chain
> 
> I believe they do, yes.  That's the whole point of loadSubscript.

Hah, so it does; I should have RTFM.  Well, hopefully with the above, we don't have to care.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> CloneScript() shouldn't even be super-expensive

Could we quantify that, please?

Bill, do we have any performance measurements for frame scripts (both how fast loopy code in the script runs and the time it takes to do the initial toplevel eval for a script that has lots of code but doesn't run most of it up front)?

Comment 11

[Luke Wagner [:luke]]

OTOH, if we determine the extra cloning was potentially costly, it still seems like a CompileOptions::hasNonSyntacticGlobalScope that basically gets forwarded directly to JSScript (no interaction with compilation) is pretty simple to reason about.  Then dynamic-scope-chain-taking JSAPIs would simply assert (or dynamically error check) that this flag was set on the input script.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Not doing reviews right now from comment #10)
> > CloneScript() shouldn't even be super-expensive
> 
> Could we quantify that, please?
> 
> Bill, do we have any performance measurements for frame scripts (both how
> fast loopy code in the script runs and the time it takes to do the initial
> toplevel eval for a script that has lots of code but doesn't run most of it
> up front)?

To be honest, I don't think we have much (or any) loopy code in frame scripts. The only thing we do in them that's performance-sensitive is session store stuff, and most of the important code there runs in JSMs that are imported by the frame scripts.

We also don't have any really big frame scripts. The two "big" ones are content.js and content-sessionStore.js, which are ~1000 lines. I don't have any performance data about the cost of cloning or running them since it's never been a problem before.

I could try to collect some data, but I'd need more context about what you're looking for. Nothing being described in this bug sounds too scary to me. We already don't set CnG on frame scripts, and we always clone them to a new compartment before running them already.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Er, sorry, I was actually worrying about subscript loader scripts and then somehow mentally got derailed onto frame scripts... I agree that for frame scripts there is no difference.

So OK, I think I would be fine with just compiling always against a global and then cloning at execute time if the execute scopechain is not a global.  For now we can require any consumer who wants to execute against a non-global to not compileAndGo, which is the case anyway, I bet...

Comment 14

[Bobby Holley (:bholley)]

(In reply to Not doing reviews right now from comment #5)
> > though for now it looks like it's only used in test_chrometoSource.xul.
> 
> Really?  I see lots of loadSubscript() in the tree and all those pass some
> target object, no?

Oh yes, sorry. I forgot that loadSubScript also takes a scope object and funnels into LoadSubscriptWithOptions.

> I guess my real question is why we set reusingGlobal based on the return
> value of FindTargetObject, not based on what our actual scope chain will
> look like when executing.

Because |reusingGlobal| is supposed to indicate whether we're executing in a b2g JSM scope, and FindTargetObject (which checks our current scopechain for a FakeBackstagePass) is the way we figure that out. Not sure if I'm misunderstanding your question?
 
> And I guess bug 1100461 covers some related (but not identical) issues, and
> even in the case of only scripts it seems like we can have effective "cache
> poisoning" issues if we compile a script against a global but then read it
> from cache and execute against a non-global...
> 
> I'd really like to understand what the actual constraints are here on when
> we do/don't want to use the cache

We effectively don't use the cache on b2g because WriteCachedFunction is a no-op. You'll want to check in with the b2g memory czars if you want to change that.

> and when we want to use a script vs a
> function.

We want to compile as a function in the reuseGlobal case, because that's how we get the FakeBackstagePass on our scope chain.

Comment 15

[Luke Wagner [:luke]]

(In reply to Not doing reviews right now from comment #13)
> So OK, I think I would be fine with just compiling always against a global
> and then cloning at execute time if the execute scopechain is not a global. 
> For now we can require any consumer who wants to execute against a
> non-global to not compileAndGo, which is the case anyway, I bet...

Since all of this will page out of memory quickly: perhaps we could document this limited dependency of the compiler the scopeChain only in cases of eval by:
 - renaming GlobalSharedContext::scopeChain to evalScopeChain (well-defined to be fp->scopeChain() for direct eval) and passed nullptr for everything else
 - renaming BytecodeEmitter::hasGlobalScope to evalInGlobalScope (and only testing it when bce->insideEval)
This would match the other two eval* args to CompileScript.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #9)
> It seems to me that we could use the same proposed
> strategy as JSOP_GNAME ops: always emit the IMPLICITTHIS op and use
> hasDynamicScopeChain to determine whether we have to go slow or not.

Hmm, when I was working on optimizing non-CNG code in Ion (addons and stuff) I didn't realize those scripts use JSOP_IMPLICITTHIS in more cases than CNG code. IMPLICITTHIS right now is slow and not even Ion-compiled; seems like we can easily take care of that after this (it could just be a VM call).

> Incidentally, jandem and I have been discussing a significant simplification
> of this whole area; I'd be curious to hear what this might look like after
> that.

I don't think the JSOP_THIS/JSOP_CREATETHIS/ComputeThis changes we discussed affect this? IMPLICITTHIS is used to determine the appropriate this-value we push for a call like |g()|, we will still need that.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the obj argument of JS_CompileScript

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the obj argument of JS_CompileUCScript

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop passing null as the obj argument of JS::Compile

Note that this is not a behavior change in the aRunInGlobalScope case in
nsFrameMessageManager, because that case never does setCompileAndGo(true) anyway.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop supporting load.call(somerandomobj) in xpcshell

I guess I should have done this in bug 1097987.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Release-assert that a script being executed against a non-global scopechain is not compileAndGo

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Drop the obj argument of JS::Compile

This is technically a behavior change for the shell's disfile() function, but
I really doubt anyone is doing disfile.call(someObj).

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 8579610 [details] [diff] [review]
part 5.  Release-assert that a script being executed against a non-global scopechain is not compileAndGo

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

::: js/src/vm/Interpreter.cpp
@@ +663,5 @@
>  {
>      /* The scope chain could be anything, so innerize just in case. */
>      RootedObject scopeChain(cx, &scopeChainArg);
>      scopeChain = GetInnerObject(scopeChain);
>      if (!scopeChain)

After the next patch, I think you can assert innerization here.

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 8579611 [details] [diff] [review]
part 6.  Drop the obj argument of JS::Compile

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

\o/

Comment 25

[Boris Zbarsky [:bzbarsky]]

> After the next patch, I think you can assert innerization here.

Indeed.  I'll do that.

Comment 26

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1732c6f8d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2f78268d48
https://hg.mozilla.org/integration/mozilla-inbound/rev/909ca1afa40f
https://hg.mozilla.org/integration/mozilla-inbound/rev/55efc4756e69
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2b308acde0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c293e45278d

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f1732c6f8d5
https://hg.mozilla.org/mozilla-central/rev/ea2f78268d48
https://hg.mozilla.org/mozilla-central/rev/909ca1afa40f
https://hg.mozilla.org/mozilla-central/rev/55efc4756e69
https://hg.mozilla.org/mozilla-central/rev/7f2b308acde0
https://hg.mozilla.org/mozilla-central/rev/5c293e45278d