864218 - IonMonkey: Cope better with closure var accesses in run-once closures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Right now, upvar accesses within a run-once closure are compiled the same way as any other run-once closure, always requiring type barriers and so forth.  This is less efficient than global variable accesses; it would be better if the two ran at the same speed.  While this doesn't hurt in microbenchmarks because of loop hoisting, it could have a more significant effect in larger programs.  This is particularly important for asm.js modules, which are run once closures where all typed array accesses go through closure vars.  On fannkuch(10) about 2.5% of executed LIR instructions are type barriers, though they don't seem to affect execution time that much.

Doing this would require some significant VM changes to accumulate type information on call objects, so putting it on the back burner until I see a test where these barriers really do cost a lot.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Well, after eliminating the other obvious differences between asm.js and normal execution on emscripten-fannkuch-10, this turns out to have a significant cost, hurting perf by about 8%.  Fixing this and using the backtracking allocator brings runtime for the demo down to about 2% slower with normal execution.  Ion executed instructions are a bit better in the normal case (better handling of the closure vars?) so I think the remaining difference is compilation / baseline / assorted VM overhead and better off tracked on more interesting benchmarks.

Anyways, this patch allows call objects in run once closures to have singleton types, and accesses on those call objects in Ion code are compiled the same way as global accesses.  The VM and baseline compiler need to keep track of updates they make to singleton call objects, and a JSOP_RUNONCE is added to deoptimize code if the closure ends up running multiple times.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 745592 [details] [diff] [review]
patch

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

Nice!  Seems like this could really help the whole module pattern.
r+ for everything but the Ion/Baseline changes (which I glanced over and looked fine too).

::: js/src/jsinterp.cpp
@@ +2717,5 @@
> +    ScopeObject &obj = regs.fp()->aliasedVarScope(sc);
> +
> +    // Avoid computing the name if no type updates are needed, as this may be
> +    // expensive on scopes with large numbers of variables.
> +    PropertyName *name = obj.hasSingletonType() ? ScopeCoordinateName(cx, script, regs.pc) : NULL;

Wow, do we still spend that much time in the interp after baseline?

@@ +3822,5 @@
> +
> +    // Force instantiation of the script's function's type to ensure the flag
> +    // is preserved in type information.
> +    if (!script->function()->getType(cx))
> +        return false;

From IRC: it'd be nice if MarkTypeObjectFlags did this itself when given a singleton type to make sure flags weren't lost.  Then this line (perhaps others for the same reason) could be removed.

::: js/src/vm/ScopeObject-inl.h
@@ +83,5 @@
>      return getSlot(fi.scopeSlot());
>  }
>  
>  inline void
> +CallObject::setAliasedVar(JSContext *cx, AliasedFormalIter fi, const Value &v)

For symmetry with the other setAliasedVar, could this take a PropertyName* and have the caller pass fi->name()?

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 745592 [details] [diff] [review]
patch

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

Sorry for the delay here.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2534,5 @@
> +    /*
> +     * Emit a prologue for run-once scripts which will deoptimize JIT code if
> +     * the script ends up running multiple times via foo.caller related
> +     * shenanigans. This is disabled for compartments in which JM code could
> +     * run, as it does not respect type information for call objects.

Nit: JM is gone.

::: js/src/ion/VMFunctions.cpp
@@ +427,2 @@
>      if (JS_LIKELY(!obj->getOps()->setProperty)) {
> +        unsigned defineHow = (jsop == JSOP_SETNAME) ? DNP_UNQUALIFIED : 0;

We have to check for JSOP_SETGNAME here as well (bug 690446 changed the other isSetName checks).

::: js/src/jsopcode.tbl
@@ +164,5 @@
> +/*
> + * Prologue emitted in scripts expected to run once, which deoptimizes code if
> + * it executes multiple times.
> + */
> +OPDEF(JSOP_RUNONCE,   71, "runonce",    NULL,         1,  0,  0,  0,  JOF_BYTE)

Nit: bump XDR_BYTECODE_VERSION.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9beff192aa2

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b9beff192aa2

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Can we consider backing this out?

This has at least caused topcrash bug 875757, and may also be responsible for another topcrash in bug 875790 and bug 875791.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I got a r=luke in-person to back this out so it can make tomorrow's nightly, bhackett wasn't online on IRC.

Tested by compiling a debug js shell (64-bit on Mac) successfully:

https://hg.mozilla.org/mozilla-central/rev/7a2f7a45819a

Comment 8

[Brian Hackett [Laid off!]]

Attachment: fuzz patch (946467115924)

Gary and decoder, can you fuzz this patch some to hopefully avoid such a catastrophic second landing?  This rolls up both the original patch and the fixes for bugs 875765 and 875774.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 755389 [details] [diff] [review]
fuzz patch (946467115924)

Didn't find anything terribly wrong after a round of overnight fuzzing. feedback+.

Comment 10

[Christian Holler (:decoder)]

Comment on attachment 755389 [details] [diff] [review]
fuzz patch (946467115924)

No additional regressions in 24 hours.

Comment 11

[Brian Hackett [Laid off!]]

Repushed with fixes for bugs 875765 and 875774.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5f190b9f9b

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0a5f190b9f9b

Comment 13

[Emanuel Hoogeveen [:ehoogeveen]]

Clang warns that the condition at https://hg.mozilla.org/mozilla-central/file/0a5f190b9f9b/js/src/frontend/BytecodeEmitter.cpp#l2573 will never hit, since the negation happens before the comparison. What is this condition supposed to be?

Comment 14

[Brian Hackett [Laid off!]]

The ! shouldn't be there.  I included a fix for this in:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0dfe6abef73

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

Excellent! I hope that means it makes the uplift :)