Closed Bug 1145491 Opened 5 years ago Closed 5 years ago

Stop using compileAndGo for deciding to output GNAME ops

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 5 obsolete files)

6.42 KB, patch
jandem
: review+
luke
: review+
Details | Diff | Splinter Review
5.56 KB, patch
jandem
: review+
luke
: review+
Details | Diff | Splinter Review
7.93 KB, patch
bzbarsky
: review+
jandem
: review+
Details | Diff | Splinter Review
2.99 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.64 KB, patch
luke
: review+
Details | Diff | Splinter Review
10.47 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.39 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.45 KB, patch
luke
: review+
Details | Diff | Splinter Review
11.88 KB, patch
luke
: review+
Details | Diff | Splinter Review
This is bug 679939 comment 29 items 4 and 5.
Attachment #8580512 - Flags: review?(luke) → review+
Attachment #8580513 - Flags: review?(luke) → review+
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
Attachment #8580514 - Flags: review?(luke) → review+
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.
Attachment #8580512 - Attachment is obsolete: true
Attachment #8580512 - Flags: review?(jdemooij)
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.
Attachment #8580545 - Flags: review?(luke) → review+
I'll do comment 6 when I'm more awake and can think.  ;)
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.
Attachment #8580673 - Flags: review?(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.
Attachment #8580673 - Flags: review?(luke) → review+
Attachment #8580545 - Flags: review?(jdemooij) → review+
Attachment #8580513 - Flags: review?(jdemooij) → review+
Attachment #8580514 - Flags: review?(jdemooij) → review+
> 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 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!
Attachment #8580699 - Flags: review?(luke) → review+
So now scopeChain is only used for MaybeCheckEvalFreeVariables
Attachment #8580715 - Flags: review?(luke)
Attachment #8580515 - Attachment is obsolete: true
Attachment #8580515 - Flags: review?(luke)
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?
Flags: needinfo?(luke)
Sounds good.
Flags: needinfo?(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.
Attachment #8580715 - Flags: review?(luke) → review+
Attachment #8580776 - Flags: review?(luke)
Attachment #8580776 - Flags: review?(luke) → review+
Attachment #8580846 - Flags: review?(luke) → review+
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;
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.
Flags: needinfo?(bzbarsky)
Attachment #8581230 - Flags: review?(luke)
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.
Attachment #8581230 - Attachment is obsolete: true
Attachment #8581230 - Flags: review?(luke)
Attachment #8581236 - Attachment description: luke → Without the more involved eval fixes
Attachment #8581236 - Flags: review?(luke)
Attachment #8581236 - Attachment is obsolete: true
Attachment #8581236 - Flags: review?(luke)
Blocks: 1146080
Attachment #8581246 - Attachment is obsolete: true
Attachment #8581246 - Flags: review?(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 = */
Attachment #8581682 - Flags: review?(luke) → review+
Depends on: 1147144
Depends on: 1148963
Depends on: 1375404
You need to log in before you can comment on or make changes to this bug.