Stop using the compileAndGo boolean to decide whether to output JSOP_IMPLICITTHIS

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Depends on: 1 bug)

Trunk
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(8 attachments)

3.63 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.88 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.27 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.26 KB, patch
luke
: review+
Details | Diff | Splinter Review
3.22 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.30 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.98 KB, patch
luke
: review+
Details | Diff | Splinter Review
4.94 KB, patch
luke
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8579830 [details] [diff] [review]
part 1.  Add a JSOP_GIMPLICITTHIS which acts like JSOP_IMPLICITTHIS when the script hasPollutedScope and JSOP_UNDEFINED otherwise
Attachment #8579830 - Flags: review?(luke)
(Assignee)

Comment 2

4 years ago
Created 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
Attachment #8579831 - Flags: review?(jdemooij)
(Assignee)

Comment 3

4 years ago
Created attachment 8579832 [details] [diff] [review]
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
Attachment #8579832 - Flags: review?(luke)
(Assignee)

Comment 4

4 years ago
Created attachment 8579833 [details] [diff] [review]
part 4.  Stop returning true from ByteCodeEmitter::needsImplicitThis based on the compileAndGo flag
Attachment #8579833 - Flags: review?(luke)
(Assignee)

Comment 5

4 years ago
Created attachment 8579834 [details] [diff] [review]
part 5.  Flag eval scripts as having a polluted scopechain when inside a with scope
Attachment #8579834 - Flags: review?(luke)
(Assignee)

Comment 6

4 years ago
Created attachment 8579835 [details] [diff] [review]
part 6.  Remove the scopechain walk in BytecodeEmitter::needsImplicitThis, since consumers should now set hasPollutedScope as needed
Attachment #8579835 - Flags: review?(luke)
(Assignee)

Comment 7

4 years ago
Created attachment 8579836 [details] [diff] [review]
part 7.  Remove the scopechain walk in the FunctionBox constructor, since consumer should now set hasPollutedScope as needed
Attachment #8579836 - Flags: review?(luke)
(Assignee)

Comment 8

4 years ago
Created attachment 8579837 [details] [diff] [review]
part 8.  Drop the scopechain member from GlobalSharedContext, since it's not needed anymore
Attachment #8579837 - Flags: review?(luke)
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;
Attachment #8579831 - Flags: review?(jdemooij) → review+
> (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?
Flags: needinfo?(luke)

Comment 11

4 years ago
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.
Attachment #8579830 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8579832 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8579833 - Flags: review?(luke) → review+

Comment 12

4 years ago
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?
Attachment #8579834 - Flags: review?(luke) → review+

Comment 13

4 years ago
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?
Attachment #8579835 - Flags: review?(luke) → review+

Comment 14

4 years ago
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.
Attachment #8579836 - Flags: review?(luke) → review+

Comment 15

4 years ago
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.
Attachment #8579837 - Flags: review?(luke) → review+

Comment 16

4 years ago
(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 :)
Flags: needinfo?(luke)
>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.
Depends on: 1147900
You need to log in before you can comment on or make changes to this bug.