1167823 - Rewrite BytecodeEmitter::checkSideEffects to work by kind, not arity

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Another bit of code that heavily depends on arity to rewrite.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Begin rewriting BCE::checkSideEffects to work by kind, not arity

Initial mega-switch, you know the jazz.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Handle various nullary/unary nodes in BytecodeEmitter::checkSideEffects

s/jazz/drill/, Miss Malaprop.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Handle more nodes by kind in BytecodeEmitter::checkSideEffects, 1/N

At some point here my care in splitting these sensibly disappeared, so here begins undistinguished grab-bags of changes until everything's transitioned.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Handle more nodes by kind when checking for side effects, 2/N

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Check various binary operators for side effects, by node kind

Looks like I split out binary stuff explicitly for some reason.  Okay.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Handle more nodes by kind when checking for side effects, 3/N

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Handle more nodes by kind when checking for side effects, 4/N

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Handle try/catch by kind when checking for side effects

And again for weird sensible-splitting.  I think this one derives from my initially considering PNK_FUNCTION to be effectful and that breaking (!) functionality.  No idea why functions *must* be considered non-effectful for correctness, but there it is.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Handle various list-ful nodes by kind when checking for side effects

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Handle various other nodes by kind when checking for side effects, 5/N

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Handle functions by kind when checking for side effects

You are not expected to understand this comment.  (I don't, either!)  Once we start having multiple kinds of parse node kind, maybe this can be made more sensible as to why.

I'm not even sure I believe it, anyway -- function statements *should* have effect inside eval code (where such introduce new bindings, I think).  How anything at all works right now, I dunno.  (Thus was it ever.)

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Handle more nodes by kind when checking for side effects, 6/N

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Handle more nodes by kind when checking for side effects, 7/N

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Remove dead code for checking whether a parse tree node has side effects

I guess N === 7, then.  And, death to the old arity-testing code!

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Handle various nullary/unary nodes in BytecodeEmitter::checkSideEffects

Updated for the splitting of PNK_DELETE into PNK_DELETE{NAME,PROP,SUPERPROP,ELEM,SUPERELEM,EXPR} in bug 1169171.

Comment 16

[Shu-yu Guo [:shu]]

Comment on attachment 8610668 [details] [diff] [review]
Handle more nodes by kind in BytecodeEmitter::checkSideEffects, 1/N

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1926,5 @@
>          MOZ_ASSERT(pn->isArity(PN_NULLARY));
>          *answer = true;
>          return true;
>  
> +      // Watch out for getters!

Oo, good catch.

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8610669 [details] [diff] [review]
Handle more nodes by kind when checking for side effects, 2/N

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

Looks okay to me, but I'd like answers to the questions below.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1922,5 @@
>          *answer = false;
>          return true;
>  
> +      case PNK_BREAK:
> +      case PNK_CONTINUE:

Why are break and continue considered effectful?

@@ +2047,5 @@
> +      // Every part of a loop might be effect-free, but looping infinitely *is*
> +      // an effect.  (Language lawyer trivia: C++ says threads can be assumed
> +      // to exit or have side effects, C++14 [intro.multithread]p27, so a C++
> +      // implementation's equivalent of the below could set |*answer = false;|
> +      // if all loop sub-nodes set |*answer = false|!)

I'm not sure if it's in the spirit of JS to say that non-termination is a side effect. I mean, hell, it's not even considered a side effect in Haskell.

Comment 18

[Shu-yu Guo [:shu]]

Comment on attachment 8610670 [details] [diff] [review]
Check various binary operators for side effects, by node kind

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2025,5 @@
> +        }
> +        return true;
> +
> +      // Most other binary operations (parsed as lists in SpiderMonkey) may
> +      // perform conversions triggering side effects.

How can it trigger conversions? valueOf? If so, please document it as such.

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8610671 [details] [diff] [review]
Handle more nodes by kind when checking for side effects, 3/N

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2121,5 @@
> +        MOZ_ASSERT(pn->isArity(PN_TERNARY));
> +        if (!checkSideEffects(pn->pn_kid1, answer))
> +            return false;
> +        if (*answer)
> +            return true;

Do you need manual short circuiting here and below? Worried about an extra call? The top of checkSideEffects is already

    if (!pn || *answer)
        return true;

Comment 20

[Shu-yu Guo [:shu]]

Comment on attachment 8610674 [details] [diff] [review]
Handle try/catch by kind when checking for side effects

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2164,5 @@
> +        MOZ_ASSERT(pn->isArity(PN_TERNARY));
> +        if (!checkSideEffects(pn->pn_kid1, answer))
> +            return false;
> +        if (*answer)
> +            return true;

Ditto question about short circuiting here as my previous review.

If you're ensuring caller short-circuiting everywhere instead of callee short-circuiting, could you remove the short-circuiting logic at the top of checkSideEffects?

Comment 21

[Shu-yu Guo [:shu]]

Comment on attachment 8610682 [details] [diff] [review]
Handle functions by kind when checking for side effects

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2189,5 @@
>  
> +      case PNK_FUNCTION:
> +        MOZ_ASSERT(pn->isArity(PN_CODE));
> +        /*
> +         * A named function, contrary to ES3, is no longer useful, because we

Nit: effectful

@@ +2193,5 @@
> +         * A named function, contrary to ES3, is no longer useful, because we
> +         * bind its name lexically (using JSOP_CALLEE) instead of creating an
> +         * Object instance and binding a readonly, permanent property in it
> +         * (the object and binding can be detected and hijacked or captured).
> +         * This is a bug fix to ES3; it is fixed in ES3.1 drafts.

That's wack.

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 8610683 [details] [diff] [review]
Handle more nodes by kind when checking for side effects, 6/N

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2189,5 @@
>          *answer = true;
>          return true;
>  
> +      // Shorthands could trigger getters.
> +      case PNK_SHORTHAND:

What is shorthand again?

Comment 23

[Shu-yu Guo [:shu]]

Comment on attachment 8610685 [details] [diff] [review]
Remove dead code for checking whether a parse tree node has side effects

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ -1901,5 @@
>  {
>      JS_CHECK_RECURSION(cx, return false);
>  
> -    if (!pn || *answer)
> -        return true;

Oh, I see you did remove the short circuiting at the top. Ignore my previous short-circuiting comments.

Comment 24

[Shu-yu Guo [:shu]]

I think it would be good to see the feasibility of removing checkSideEffects entirely. The frontend shouldn't be doing this optimization now that we have 2 tiers of JITs, and it makes debugging possibly confusing.

Comment 25

[Jeff Walden [:Waldo]]

(In reply to Shu-yu Guo [:shu] from comment #17)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1922,5 @@
> >          *answer = false;
> >          return true;
> >  
> > +      case PNK_BREAK:
> > +      case PNK_CONTINUE:
> 
> Why are break and continue considered effectful?

"side effect" isn't quite the right name for what this is checking.  It's sort of more whether the node in question might do something that would change program semantics if you removed it.  But that's not quite right, either, because in |({ x: null })| the PNK_COLON node for |x| affects semantics if it's removed, but as its effect is only upon a node that we consider "not to have side effects", that's fine.  What should this be called?  Beats me.  I tried for a bit and came up blank, so I punted.  It's hard even to *describe* it for a comment, either.  :-(

Given that meaning, eliminating a break/continue could cause code in a loop (more precisely, code in a statement list as loop body) to fail to exit/resume at start appropriately.  And this matters even outside loops, because in very weird cases you can "break" in a named statement that's not a loop.  (Or maybe "continue", I can't remember.)  So these are "effectful".

It's insane.  ECMAScript is insane.  Recognition code 927, I am a potato.

> @@ +2047,5 @@
> > +      // Every part of a loop might be effect-free, but looping infinitely *is*
> > +      // an effect.  (Language lawyer trivia: C++ says threads can be assumed
> > +      // to exit or have side effects, C++14 [intro.multithread]p27, so a C++
> > +      // implementation's equivalent of the below could set |*answer = false;|
> > +      // if all loop sub-nodes set |*answer = false|!)
> 
> I'm not sure if it's in the spirit of JS to say that non-termination is a
> side effect. I mean, hell, it's not even considered a side effect in Haskell.

Not a "side effect", but iloops are supposed to happen because the spec says they do, and they're visible to code that tries to do something after an iloop.  Therefore they're effects in the sense of this method.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 8610669 [details] [diff] [review]
Handle more nodes by kind when checking for side effects, 2/N

See comment 25.

Comment 27

[Shu-yu Guo [:shu]]

Comment on attachment 8610669 [details] [diff] [review]
Handle more nodes by kind when checking for side effects, 2/N

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1922,5 @@
>          *answer = false;
>          return true;
>  
> +      case PNK_BREAK:
> +      case PNK_CONTINUE:

Why are break and continue considered effectful?

@@ +2047,5 @@
> +      // Every part of a loop might be effect-free, but looping infinitely *is*
> +      // an effect.  (Language lawyer trivia: C++ says threads can be assumed
> +      // to exit or have side effects, C++14 [intro.multithread]p27, so a C++
> +      // implementation's equivalent of the below could set |*answer = false;|
> +      // if all loop sub-nodes set |*answer = false|!)

I'm not sure if it's in the spirit of JS to say that non-termination is a side effect. I mean, hell, it's not even considered a side effect in Haskell.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/928b82c7e3f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3dadbf59a1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f2b05a4a5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d74cf23229b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3f6340e78a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/4de3e7d764dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e35358eb840
https://hg.mozilla.org/integration/mozilla-inbound/rev/5baf4bf01aa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8aa39b6518
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c3682b27301
https://hg.mozilla.org/integration/mozilla-inbound/rev/4461be1cd2cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ee40559aa3
https://hg.mozilla.org/integration/mozilla-inbound/rev/716036bb7ff0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69363c468e1

Comment 29

[Jeff Walden [:Waldo]]

(In reply to Shu-yu Guo [:shu] from comment #22)
> What is shorthand again?

var x = 5;
var obj = { x }; // as if { x: x }

Added a reminder comment with a little elaboration.

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/928b82c7e3f2
https://hg.mozilla.org/mozilla-central/rev/d3dadbf59a1a
https://hg.mozilla.org/mozilla-central/rev/a0f2b05a4a5f
https://hg.mozilla.org/mozilla-central/rev/6d74cf23229b
https://hg.mozilla.org/mozilla-central/rev/e3f6340e78a7
https://hg.mozilla.org/mozilla-central/rev/4de3e7d764dc
https://hg.mozilla.org/mozilla-central/rev/1e35358eb840
https://hg.mozilla.org/mozilla-central/rev/5baf4bf01aa2
https://hg.mozilla.org/mozilla-central/rev/6d8aa39b6518
https://hg.mozilla.org/mozilla-central/rev/2c3682b27301
https://hg.mozilla.org/mozilla-central/rev/4461be1cd2cd
https://hg.mozilla.org/mozilla-central/rev/22ee40559aa3
https://hg.mozilla.org/mozilla-central/rev/716036bb7ff0
https://hg.mozilla.org/mozilla-central/rev/b69363c468e1