1145491 - Stop using compileAndGo for deciding to output GNAME ops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This is bug 679939 comment 29 items 4 and 5.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Only do the fast path for JSOP_BINDGNAME when the script doesn't have a polluted global

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Only do the fast path for JSOP_GETGNAME when the script doesn't have a polluted global

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Only do the fast path for JSOP_SETGNAME and JSOP_STRICTSETGNAME when the script doesn't have a polluted global

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop checking compileAndGo before emitting GNAME ops

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8580514 [details] [diff] [review]
part 3.  Only do the fast path for JSOP_SETGNAME and JSOP_STRICTSETGNAME when the script doesn't have a polluted global

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

::: js/src/vm/Interpreter.cpp
@@ +2472,5 @@
>      static_assert(JSOP_SETGNAME_LENGTH == JSOP_STRICTSETGNAME_LENGTH,
>                    "setganem adn strictsetgname must be the same size");
> +    static_assert(JSOP_SETNAME_LENGTH == JSOP_SETGNAME_LENGTH,
> +                  "We're sharing the END_CASE so the lengths better match");
> +                  

trailing whitespace

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8580515 [details] [diff] [review]
part 4.  Stop checking compileAndGo before emitting GNAME ops

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +305,5 @@
> +    //    GNAME ops only if scopeChain is the global.
> +    // 3) Normal compiles: scopeChain is the global.
> +    // 4) Background compiles: scopeChain is null.
> +    //
> +    // Seems like we should really output GNAME ops in case 4, but we don't...

Hah, you noticed that too!  This is mostly only a problem for the top-level background script in practice since the nested functions will be lazy parsed and, when lazy-compiling, we're back to hasGlobalScope.  But it does seem like a bug.

So, given that, the only thing we need to do here is make sure we don't bind GNAME for direct eval inside a nested scope.  But we don't need scopeChain for this, we could just test:
  evalStaticScope->enclosingScopeForStaticScopeIter() == nullptr
We don't even need to do that here, since we can use bce->evalStaticScope in TryConvertFreeName (suitably null-tested), so we could kill hasGlobalName and the scopeChain arg.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Only do the fast path for JSOP_BINDGNAME when the script doesn't have a polluted global

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8580545 [details] [diff] [review]
part 1.  Only do the fast path for JSOP_BINDGNAME when the script doesn't have a polluted global

Er, sorry, I'd missed Luke reviewing the old part 1.  The only difference between what he reviewed and this is the js/src/jit/BaselineIC.cpp assertion change.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'll do comment 6 when I'm more awake and can think.  ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Emit JSOP_IMPLICITTHIS for JSOP_GETGNAME as well, because otherwise bareword calls in polluted-global scripts won't work right

This got caught by some of the browser tests, because some XBL bindings do bareword calls in event handlers to functions that hen expect 'this' inside the function to be the event target.  When we turned those barewords into GETGNAMEs suddenly the this-binding was busted.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8580673 [details] [diff] [review]
part 4.  Emit JSOP_IMPLICITTHIS for JSOP_GETGNAME as well, because otherwise bareword calls in polluted-global scripts won't work right

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

Ah makes sense.  So I guess the underlying chance here is that in !compileAndGo code we are emitting GNAME ops where we previously emitted NAME ops.  It'd be good to do a quick audit over other JSOP_*NAME* sites to see if there are other cases.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It'd be good to do a quick audit over other JSOP_*NAME* sites

Yeah, I thought I had.  ;)  I'll do another once-over.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Fix up various other places that check for JSOP_GET/SETNAME without checking for the GNAME versions too

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8580699 [details] [diff] [review]
part 5.  Fix up various other places that check for JSOP_GET/SETNAME without checking for the GNAME versions too

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

Good finds!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Stop checking compileAndGo before emitting GNAME ops

So now scopeChain is only used for MaybeCheckEvalFreeVariables

Comment 16

[Boris Zbarsky [:bzbarsky]]

Note that there is one outstanding place where NAME and GNAME ops are treated differently: EmitAssignment.  See bug 1145641.

One thing I could do in this bug is add the testcase I pasted into bug 1145641 and add a comment to js/src/jit-test/tests/basic/bug1106982.js saying that if it's changed because we start doing only one has() the other testcase needs to be fixed too... Or something.  Thoughts?

Comment 17

[Luke Wagner [:luke]]

Sounds good.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8580715 [details] [diff] [review]
part 6.  Stop checking compileAndGo before emitting GNAME ops

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

Nice!

::: js/src/frontend/Parser.cpp
@@ +5491,5 @@
>      // in syntax-parse mode. See bug 892583.
>      if (handler.syntaxParser) {
> +        // IMPORTANT: If you ever change this, update
> +        // js/src/jit-test/tests/basic/eval-scopes.js to some other way of
> +        // forcing non-lazy parsing inside eval.

Instead of this, how about adding a 'eagerParse' option to ParseCompileOptions (so you can pass it to evaluate())?  It should be just a few lines and it will enable more fuzzing.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Address review comments

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Fix script cloning to propagate the polluted-global-scope state to the lambda templates in the script

Comment 21

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80344fd40d6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/cddba378a563
https://hg.mozilla.org/integration/mozilla-inbound/rev/97b16da6169b
https://hg.mozilla.org/integration/mozilla-inbound/rev/151e4cdb34cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8066b21e74a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be39afdf528
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d14c6661f00

Comment 22

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

I had to back this out (along with bug 1145488) for ggc orange:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b7a4d9da546

https://treeherder.mozilla.org/logviewer.html#?job_id=7884565&repo=mozilla-inbound

Comment 23

[Jan de Mooij [:jandem]]

This regressed a lot of benchmarks on AWFY.

I'd start with Suspider math-partial-sums, it's a very small benchmark and got > 4x slower, so it shouldn't be too hard to figure out what caused it. Note that a2..a9 are globals:

var a1 = a2 = a3 = a4 = a5 = a6 = a7 = a8 = a9 = 0.0;

Comment 24

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.  The perf problem is that I reversed the sense of the boolean arg to the BytecodeEmitter constructor but only fixed _some_ of the callsites.

And in particular, we ended up not ever generating gname ops for lazy functions (or functions compiled with new Function, afaict; that's clearly a bug too).

Patch coming up that fixes this, and in fact improves the handling of lazy functions in global eval to generate gname ops (which we did not use to do).

I'm hoping that will also fix the ggc timeout.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix up the bits described in comment 24

Comment 26

[Boris Zbarsky [:bzbarsky]]

OK.  Turns out limiting lack of gname ops to non-global eval breaks js/src/jit-test/tests/closures/bug684489.js

The reason for that is that in that bug we have a global eval in strict mode, with a var inside the eval.  This apparently introduces a new scope for the var.  For non-lazy functions this is handled by TryConvertFreeName never using gname ops when bce->insideEval and the mode is strict.  But for lazy functions, we always pass false to the BytecodeEmitter constructor's insideEval argument (though we kinda make it _look_ like we're passing true).

Anyway, I'm going to put the lazy function thing back to something which doesn't try to use gname ops even in a global eval.  Filed bug 1146080 on making this all saner.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Without the more involved eval fixes

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Forgot some other additions to eval-scopes.js, which seem to not be tested anywhere else in our test suite, apparently...

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: More explicit bogus args, per IRC discussion

Comment 30

[Luke Wagner [:luke]]

Comment on attachment 8581682 [details] [diff] [review]
More explicit bogus args, per IRC discussion

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

Awesome, makes sense.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +647,5 @@
>          BytecodeEmitter funbce(/* parent = */ nullptr, &parser, fn->pn_funbox, script,
>                                 /* lazyScript = */ js::NullPtr(), /* insideEval = */ false,
>                                 /* evalCaller = */ js::NullPtr(),
>                                 /* evalStaticScope = */ js::NullPtr(),
> +                               false, options.lineno);

/* insideNonGlobalEval = */

Comment 31

[Boris Zbarsky [:bzbarsky]]

Done, and:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b68dc3654d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1228e5205f12
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9e79ad1715
https://hg.mozilla.org/integration/mozilla-inbound/rev/b722f12b507b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a838776f94
https://hg.mozilla.org/integration/mozilla-inbound/rev/72accc37764a
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f700adddec

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/e1b68dc3654d
https://hg.mozilla.org/mozilla-central/rev/1228e5205f12
https://hg.mozilla.org/mozilla-central/rev/0f9e79ad1715
https://hg.mozilla.org/mozilla-central/rev/b722f12b507b
https://hg.mozilla.org/mozilla-central/rev/d9a838776f94
https://hg.mozilla.org/mozilla-central/rev/72accc37764a
https://hg.mozilla.org/mozilla-central/rev/55f700adddec