Closed Bug 1045529 Opened 10 years ago Closed 10 years ago

Ion-compile non-CNG functions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
Since bug 939562, the JITs and TI have been enabled for chrome code and this helped some addons a lot (bug 916464 comment 8). Unfortunately, a lot of chrome scripts are non-CNG and they still don't get Ion compilation.

Since compartment-per-global, non-CNG is pretty similar to CNG for the JITs. I think there's only a problem for global scripts, because we can't assume the scope chain is the global object.

This patch allows us to compile non-CNG functions. Handling global scripts is more work and probably not worth it for Ion (Baseline does though).

Even with Ion compilation, non-CNG code will be slower than CNG code due to name lookups being less efficient (*NAME ops instead of *GNAME ops). Ideally Gecko would stop using non-CNG, but that seems a lot harder.
Attachment #8463887 - Flags: review?(bhackett1024)
To wit, with a bit of work, I think we could remove CNG.  It gives us a guarantee that the global won't change, but we can just as well check this dynamically in Execute() and invalidate the JIT code when run with a global different than the one compiled with.  It's even simpler for function scripts because their scope chain is fixed.  Another property CNG offers (I think) is that there are no random objects between the outermost lexical scope and the global scope, so we'd need to check for this too when optimizing.  In theory, we could remove the GNAME ops and rely on the NAME ops being fully specialized but this might measurably hurt interp (and maybe baseline?) perf.
Can we clone compileAndGo scripts? I'd always thought we couldn't, but it looks like the main places where we clone scripts (XBL) don't unset compileAndGo.

If we can in fact clone them, then this comment [1] is wrong, and we could potentially remove that callsite.

We still need to support DOM objects on the scope chain [2], though it's totally fine for that property to be baked into the script.

Those are the only two callsites in Gecko that I can see.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameMessageManager.cpp#1536

[2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#1081
We cannot clone CNG scripts; that would effectively allow running the (cloned) bytecode against a second global (which could have different properties).  We dynamically guard against this changing globals of compileAndGo functions in JS_CloneFunctionObject.  ISTR a plan with bz to even remove JS_CloneFunctionObject by smarter handling of event handlers...
(In reply to Luke Wagner [:luke] from comment #3)
> We cannot clone CNG scripts; that would effectively allow running the
> (cloned) bytecode against a second global (which could have different
> properties).  We dynamically guard against this changing globals of
> compileAndGo functions in JS_CloneFunctionObject.

Where? I only see us guarding against parenting compileAndGo functions to non-globals. Can you link me?

>  ISTR a plan with bz to
> even remove JS_CloneFunctionObject by smarter handling of event handlers...

We don't clone for event handlers anymore (I fixed that), but we do for XBL.
(In reply to Bobby Holley (:bholley) from comment #4)
> Where? I only see us guarding against parenting compileAndGo functions to
> non-globals. Can you link me?

Gooooood question.  It used to, but I can't find it anymore.  The cloning path has got to be one of the most obtuse paths through SM...

> >  ISTR a plan with bz to
> > even remove JS_CloneFunctionObject by smarter handling of event handlers...
> 
> We don't clone for event handlers anymore (I fixed that), but we do for XBL.

Nice!  What would it take to remove cloning from XBL?
(In reply to Luke Wagner [:luke] from comment #5)
> > We don't clone for event handlers anymore (I fixed that), but we do for XBL.
> 
> Nice!  What would it take to remove cloning from XBL?

We would need to remove all of the consumers of xpc::GetCompilationScope, which are XUL and XBL. Both of them precompile scripts in the compilation global and then clone them as-needed into the scopes where they run, presumably because that's much faster (or used to be much faster) than compiling the scripts from scratch every time.

Per comment 5 though, are you _sure_ that compile-and-go scripts can't be cloned, or might that have changed?
Well, the basic point remains that if you compile with CNG on global A and then clone into global B with a different slot layout, you will get crashes.  I'd have to do some hg annotate archaeology to see when this recent leniency entered.  It might be a bug.
(In reply to Luke Wagner [:luke] from comment #7)
> I'd have to do some hg annotate archaeology to see when this recent leniency
> entered.  It might be a bug.

Please do!
(In reply to Luke Wagner [:luke] from comment #1)
> In theory, we could remove the GNAME ops and rely on the NAME
> ops being fully specialized but this might measurably hurt interp (and maybe
> baseline?) perf.

Actually the interpreter (and Baseline more or less) already use the same code paths for NAME and GNAME ops. Ion OTOH has fast paths for GNAME ops and NAME ops always use an IC.

(In reply to Luke Wagner [:luke] from comment #7)
> Well, the basic point remains that if you compile with CNG on global A and
> then clone into global B with a different slot layout, you will get crashes.

Are you sure? Unlike the GETGLOBAL/SETGLOBAL ops that were removed a long time ago, GNAME ops don't store the actual slot number, just a PropertyName.
(In reply to Jan de Mooij [:jandem] from comment #9)
> Actually the interpreter (and Baseline more or less) already use the same
> code paths for NAME and GNAME ops. Ion OTOH has fast paths for GNAME ops and
> NAME ops always use an IC.

Hm no, NameOperation checks IsGlobalOp. But if we remove GNAME ops, Ion will need some other way to determine a NAME op will always hit the global.
(In reply to Jan de Mooij [:jandem] from comment #9)
> > Well, the basic point remains that if you compile with CNG on global A and
> > then clone into global B with a different slot layout, you will get crashes.
> 
> Are you sure? Unlike the GETGLOBAL/SETGLOBAL ops that were removed a long
> time ago, GNAME ops don't store the actual slot number, just a PropertyName.

Yeah "slot layout" was the wrong term, but won't bad things happen if the PropertyName assumed by the GNAME op isn't there?  I assume we're compiling this down to a single unguarded load instruction?  Do we already guard on the global object's identity when entering jit code?

(In reply to Jan de Mooij [:jandem] from comment #10)
> Hm no, NameOperation checks IsGlobalOp. But if we remove GNAME ops, Ion will
> need some other way to determine a NAME op will always hit the global.

Ah, I forgot that GNAME ops tell us that the name definitely does not resolve to anything before the global scope; NAME ops can hit 'with' objects and scopes touched by eval, etc.  Technically, the JIT could rediscover this info, but it's much simpler/faster to bake that knowledge into the bytecode since we already know it there and it's static.

So, if we removed CNG, GNAME ops would effectively just change their meaning to "a name lookup that starts its lookup on the first non-scope object of the scope chain" and we optimize for the common case that the first non-scope object is the same global we compiled with.
(In reply to Luke Wagner [:luke] from comment #11)
> Yeah "slot layout" was the wrong term, but won't bad things happen if the
> PropertyName assumed by the GNAME op isn't there?  I assume we're compiling
> this down to a single unguarded load instruction?  Do we already guard on
> the global object's identity when entering jit code?

When the script is cloned, I assume we don't use/clone the JIT code of the original script. So when we Ion-compile the cloned JSScript, we will look at its (new) global and base all our TI stuff that allows us to emit a single unguarded load instruction on this global's TypeObject and things should work. I've no idea if this is actually happening atm, it's just what I'd expect.

> (In reply to Jan de Mooij [:jandem] from comment #10)
> So, if we removed CNG, GNAME ops would effectively just change their meaning
> to "a name lookup that starts its lookup on the first non-scope object of
> the scope chain" and we optimize for the common case that the first
> non-scope object is the same global we compiled with.

Ah yeah, that makes a lot of sense.

FWIW, I'm willing to do some more work on removing CNG if we can come up with a good alternative. For add-on developers in particular it sucks when their crypto code or whatever does not get Ion or asm.js compilation.
(In reply to Luke Wagner [:luke] from comment #7)
> Well, the basic point remains that if you compile with CNG on global A and
> then clone into global B with a different slot layout, you will get crashes.
> I'd have to do some hg annotate archaeology to see when this recent leniency
> entered.  It might be a bug.

This change came in bug 462300.  Till, based on my above reasoning (which may be stale) this seems like an unsafe change.  It might be fine in current Mozilla b/c we don't even try to clone CNG functions.
Flags: needinfo?(till)
(In reply to Luke Wagner [:luke] from comment #11)
> So, if we removed CNG, GNAME ops would effectively just change their meaning
> to "a name lookup that starts its lookup on the first non-scope object of
> the scope chain" and we optimize for the common case that the first
> non-scope object is the same global we compiled with.

This doesn't seem like it would work very well, these semantics are kind of tricky and checking the first non-scope object is a global would require a loop to run in more or less every execution of an IonScript containing global accesses.  I think the bailout / recompilation mechanism would need to be able to cope with this too and it just seems way more complicated than what we are doing now.

I think for now we need to have a notion of whether a script can run against non-global objects, but it would be cool if this could be contained to just CompileOptions / BytecodeEmitter, and used in deciding whether to emit NAME vs. GNAME ops, but not propagated to the generated scripts.  It sounds like there are some things preventing that from happening, like the IonBuilder scope chain issue mentioned in this patch.  Whether or not these are resolved I think compileAndGo should be renamed to something with some apparent meaning, like hasNonGlobalScope().
Attachment #8463887 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #14)
> This doesn't seem like it would work very well, these semantics are kind of
> tricky and checking the first non-scope object is a global would require a
> loop to run in more or less every execution of an IonScript containing
> global accesses. I think the bailout / recompilation mechanism would need
> to be able to cope with this too and it just seems way more complicated than
> what we are doing now.

I don't think it's quite that bad; I think the only two pinchpoints we'd need to guard on would be JS::ExecuteScript and JS_CloneFunctionObject (checking that 'obj' == script->global(), invalidating all JIT code on the script and nested scripts on mismatch).
(In reply to Luke Wagner [:luke] from comment #15)
> I don't think it's quite that bad; I think the only two pinchpoints we'd
> need to guard on would be JS::ExecuteScript and JS_CloneFunctionObject
> (checking that 'obj' == script->global(), invalidating all JIT code on the
> script and nested scripts on mismatch).

Well, after invalidating we'd need to set a bit somewhere so that when the code gets recompiled it does so with weaker assumptions about NEWGNAME ops, and the presence of that bit would effectively trigger the compilation behavior we'll have (after this patch) with non-CNG scripts.  The existing NAME vs. GNAME semantics just seem to capture the distinction between these free variable accesses elegantly, and I think it would be pretty hard to replace this with something simpler and more efficient.
(In reply to Brian Hackett (:bhackett) from comment #16)
> (In reply to Luke Wagner [:luke] from comment #15)
> Well, after invalidating we'd need to set a bit somewhere so that when the
> code gets recompiled it does so with weaker assumptions about NEWGNAME ops,
> and the presence of that bit would effectively trigger the compilation
> behavior we'll have (after this patch) with non-CNG scripts.

Exactly.

> The existing NAME vs. GNAME semantics just seem to capture the distinction between these
> free variable accesses elegantly,

As I said in comment 11, I do want to keep the GNAME ops for the reason you stated.

> and I think it would be pretty hard to
> replace this with something simpler and more efficient.

The simplification I'm interested in is removing the CNG flag.  Granted, with CPG, the meaning of CNG is simpler than it used to be; but it's still one more weird frob.  Another benefit is that, with these trivial checks, the only code that ever got the name IC for global access would be code that actually ran with a non-globals on the scope chain.  What I don't know is how much chrome JS is actually run without CNG.
(In reply to Luke Wagner [:luke] from comment #17)
> As I said in comment 11, I do want to keep the GNAME ops for the reason you
> stated.

Hmm, from my reading of comment 11 I thought you wanted to keep the GNAME ops but were proposing weakening their semantics so that the CNG flag could be removed, along with (I think) all distinctions between scripts running in non-global vs. global scopes.  I'm mainly concerned with keeping not just the ops but also their existing semantics.  Doing this would require the BytecodeEmitter to still know whether a script can run in non-global scope, but that doesn't seem so bad compared to the invalidation mechanism sketched in comments 15 and 16.
(In reply to Brian Hackett (:bhackett) from comment #18)
> Hmm, from my reading of comment 11 I thought you wanted to keep the GNAME
> ops but were proposing weakening their semantics so that the CNG flag could
> be removed, along with (I think) all distinctions between scripts running in
> non-global vs. global scopes.

Well, making the distinction dynamic.

> I'm mainly concerned with keeping not just
> the ops but also their existing semantics.  Doing this would require the
> BytecodeEmitter to still know whether a script can run in non-global scope,
> but that doesn't seem so bad compared to the invalidation mechanism sketched
> in comments 15 and 16.

I can't imagine the invalidation mechanism would be more than, as you sketched, another bit on the script and a few if's.  Ultimately, I don't think it's a big deal either way so I don't care to push on it, I just wanted to point out that this is another option with the minor benefits I already listed.
https://hg.mozilla.org/mozilla-central/rev/81ce8a74aba1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: needinfo?(till)
You need to log in before you can comment on or make changes to this bug.