1144802 - Stop using the compileAndGo boolean to decide whether to output JSOP_IMPLICITTHIS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a JSOP_GIMPLICITTHIS which acts like JSOP_IMPLICITTHIS when the script hasPollutedScope and JSOP_UNDEFINED otherwise

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add JIT and interpreter fast paths for JSOP_GIMPLICITTHIS when the script doesn't have a polluted scope

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Output JSOP_GIMPLICITTHIS whenever we have a JSOP_GETNAME in call context and don't know for a fact that we need an implicit this

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop returning true from ByteCodeEmitter::needsImplicitThis based on the compileAndGo flag

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Flag eval scripts as having a polluted scopechain when inside a with scope

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Remove the scopechain walk in BytecodeEmitter::needsImplicitThis, since consumers should now set hasPollutedScope as needed

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Remove the scopechain walk in the FunctionBox constructor, since consumer should now set hasPollutedScope as needed

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Drop the scopechain member from GlobalSharedContext, since it's not needed anymore

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8579831 [details] [diff] [review]
part 2.  Add JIT and interpreter fast paths for JSOP_GIMPLICITTHIS when the script doesn't have a polluted scope

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

Looks good.

(Unrelated to this part, but don't forget to bump XDR_BYTECODE_VERSION_SUBTRAHEND in vm/Xdr.h when you change the bytecode format :)

::: js/src/jit/IonBuilder.cpp
@@ +639,5 @@
>                case JSOP_UNDEFINED:
>                  type = MIRType_Undefined;
>                  break;
> +              case JSOP_GIMPLICITTHIS:
> +                MOZ_ASSERT(!script()->hasPollutedScope());

analyzeNewLoopTypes is called before we generate MIR for the loop (where we will abort), so I don't think this always holds. You can just do:

if (!script()->hasPollutedScope())
    type = MIRType_Undefined;
break;

Comment 10

[Boris Zbarsky [:bzbarsky]]

> (Unrelated to this part, but don't forget to bump XDR_BYTECODE_VERSION_SUBTRAHEND

Er, yes, I meant to do that in part 1.

But now that I think about it, which part should I bump it in?  The one where I introduce the new opcode, the one where I start using it, or the one where I change exactly when I'm using it?  Or all three?

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8579830 [details] [diff] [review]
part 1.  Add a JSOP_GIMPLICITTHIS which acts like JSOP_IMPLICITTHIS when the script hasPollutedScope and JSOP_UNDEFINED otherwise

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

::: js/src/vm/Opcodes.h
@@ +1865,5 @@
>       */ \
> +    macro(JSOP_TOSTRING,    228, "tostring",       NULL,  1,  1,  1,  JOF_BYTE)  \
> +    /*
> +     * Pushes the implicit 'this' value for calls to the associated name onto
> +     * the stack; only used when we know we're not inside a with block.

I see some JSOP_UNUSED after JSOP_STRICTSETGNAME, perhaps you could put this there?  Also, for symmetry with the descriptions of the other GNAME ops, maybe just say "push 'this' for a call in the global scope" w/o mentioning 'with', because that's sortof a detail.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8579834 [details] [diff] [review]
part 5.  Flag eval scripts as having a polluted scopechain when inside a with scope

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

::: js/src/builtin/Eval.cpp
@@ +323,5 @@
> +        }
> +
> +        bool hasPollutedScope =
> +            isInsideWith ||
> +            (evalType == DIRECT_EVAL && callerScript->hasPollutedScope());

I think it'd be a tiny bit more readable with a \n after this decl.

@@ +413,5 @@
> +            if (scope->is<DynamicWithObject>()) {
> +                isInsideWith = true;
> +                break;
> +            }
> +        }

Can you hoist this loop out into a static function shared by both Evals?

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8579835 [details] [diff] [review]
part 6.  Remove the scopechain walk in BytecodeEmitter::needsImplicitThis, since consumers should now set hasPollutedScope as needed

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2158,5 @@
>  bool
>  BytecodeEmitter::needsImplicitThis()
>  {
> +    if (sc->isFunctionBox() && sc->asFunctionBox()->inWith)
> +        return true;

So you've effectively hoisted this scope chain analysis from inside the compiler (bad) to the eval caller (good) via setHasPollutedScope().  Can you then remove the scopeChain argument to CompileScript entirely instead of me doing bug 1143793 comment 15?

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8579836 [details] [diff] [review]
part 7.  Remove the scopechain walk in the FunctionBox constructor, since consumer should now set hasPollutedScope as needed

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

Mmmmmmm.

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8579837 [details] [diff] [review]
part 8.  Drop the scopechain member from GlobalSharedContext, since it's not needed anymore

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

lol, you already did it.  That just leaves the hasGlobalObject use case which I can kill independently and then we can kill the scopeChain arg to CompileScript.

Comment 16

[Luke Wagner [:luke]]

(In reply to Not doing reviews right now from comment #10)
If you're going for maximum bisectability, then all three (since all three would invalidate the XDR cache).  (This is why I just use buildid for the asm.js cache :)

Comment 17

[Boris Zbarsky [:bzbarsky]]

>I think it'd be a tiny bit more readable with a \n after this decl.
>Can you hoist this loop out into a static function shared by both Evals?

Done and done.

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8580bcdd658
https://hg.mozilla.org/integration/mozilla-inbound/rev/228171a9cab8
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa6b17cb3d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9ce50ad442
https://hg.mozilla.org/integration/mozilla-inbound/rev/12a39a29eb7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62854da1c2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe880eb124d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7971c5d94a15

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e8580bcdd658
https://hg.mozilla.org/mozilla-central/rev/228171a9cab8
https://hg.mozilla.org/mozilla-central/rev/daa6b17cb3d4
https://hg.mozilla.org/mozilla-central/rev/bb9ce50ad442
https://hg.mozilla.org/mozilla-central/rev/12a39a29eb7b
https://hg.mozilla.org/mozilla-central/rev/f62854da1c2c
https://hg.mozilla.org/mozilla-central/rev/ebe880eb124d
https://hg.mozilla.org/mozilla-central/rev/7971c5d94a15