1460154 - Handle TDZCheckCache in IfThenElseEmitter.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

Currently IfThenElseEmitter requires caller to use emitTreeInBranch if the branch may contain lexical variable access.

This should be handled in IfThenElseEmitter.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Rename IfThenElseEmitter to IfEmitter.

Part 2 requires additional parameter to the constructor.
as a preparation for it, to shorten the
line length of callsite, I shortened the class name from IfThenElseEmitter to IfEmitter.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Handle TDZCheckCache in IfEmitter.

This moves TDZCheckCache which has been instantiated in BytecodeEmitter::emitTreeInBranch to IfEmitter,
so that the caller don't have to worry about it and just use BytecodeEmitter::emitTree.

Then, in internal usecase we don't need TDZCheckCache since it's guaranteed that there's no name references in the branches.
For such usage, I've added 2nd parameter to IfEmitter ctor to specify whether the caller needs TDZCheckCache or not.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8980150 [details] [diff] [review]
Part 2: Handle TDZCheckCache in IfEmitter.

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

I can't review this because I don't understand TDZCheckCache sufficiently.

If you can document it further, I might be able to teach myself enough to review the patch. Otherwise, you should probably ask someone else.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1983,5 @@
>  //
>  class MOZ_STACK_CLASS IfEmitter
>  {
> +  public:
> +    enum class TDZCheckKind {

The acronym "TDZ" is not self-documenting. Could you add a small comment explaining what it's about?

@@ +2064,5 @@
>      State state_;
>  #endif
>  
>    public:
> +    explicit IfEmitter(BytecodeEmitter* bce, TDZCheckKind tdzCheckKind = TDZCheckKind::NeedsCheck)

Here also, could you document *when* we need a check and when we don't?

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Handle TDZCheckCache in IfEmitter.

Thank you for reviewing!

Added comments, and also renamed the enum values clearer (I think).

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8982127 [details] [diff] [review]
Part 2: Handle TDZCheckCache in IfEmitter.

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

Sorry, I've really tried, but my knowledge of the bytecode emitter is insufficient to review this patch :/

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1993,5 @@
> +    // lexical variables, which means they should have their own TDZCheckCache.
> +    // Basically TDZCheckCache should be created for each basic block, which
> +    // then and else clauses are, but for internally used branches which are
> +    // known not to touch lexical variables (but operate only on values on the
> +    // stack), we can skip creating TDZCheckCache for them.

Mmmh... How are lexical variables not "values on the stack"?

@@ +2177,5 @@
>          calculateOrCheckPushed();
>  
> +        // The end of TDZCheckCache for then-clause.
> +        if (kind_ == Kind::MayContainLexicalAccessInBranch)
> +            tdzCache_.reset();

Would an assertion that `tdzCache` is non-empty make sense?

(and vice-versa wherever we have an `emplace`)

Comment 6

[Tooru Fujisawa [:arai]]

Thanks again!

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #5)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1993,5 @@
> > +    // lexical variables, which means they should have their own TDZCheckCache.
> > +    // Basically TDZCheckCache should be created for each basic block, which
> > +    // then and else clauses are, but for internally used branches which are
> > +    // known not to touch lexical variables (but operate only on values on the
> > +    // stack), we can skip creating TDZCheckCache for them.
> 
> Mmmh... How are lexical variables not "values on the stack"?

the thing on the stack is value, not variable itself.
so, getting or modifying values on the stack doesn't involve with the variable.

we should check if the lexical variable is initialized only when getting the value from the lexical variable,
and when the lexical variable is initialized, we can get the value from it and push the value onto the stack.


> @@ +2177,5 @@
> >          calculateOrCheckPushed();
> >  
> > +        // The end of TDZCheckCache for then-clause.
> > +        if (kind_ == Kind::MayContainLexicalAccessInBranch)
> > +            tdzCache_.reset();
> 
> Would an assertion that `tdzCache` is non-empty make sense?
> 
> (and vice-versa wherever we have an `emplace`)

Yeah, makes sense, I'll add that.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Handle TDZCheckCache in IfEmitter.

* IfEmitter now handles TDZCheckCache for branches, and consumers don't have to use emitTreeInBranch
 * Added more comments to TDZCheckCache
 * Added Kind parameter to IfEmitter ctor, to skip creating TDZCheckCache for branches, in internal use which don't need TDZCheckCache

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Handle TDZCheckCache in IfEmitter.

it was old patch :P

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8982383 [details] [diff] [review]
Part 2: Handle TDZCheckCache in IfEmitter.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +75,5 @@
>             kind == ParseNodeKind::For ||
>             kind == ParseNodeKind::Function;
>  }
>  
> +// A cache that tracks superfluous Temporal Dead Zone (TDZ) checks.

"tracks TDZ (TDZ) checks, so that any use of a lexical variable that's dominated by an earlier use, or by evaluation of its declaration (which will initialize it, perhaps to |undefined|), doesn't have to redundantly check that the lexical variable has been initialized"

perhaps?

@@ +82,5 @@
>  // subclasses contain a TDZCheckCache.
> +//
> +// This class tracks the state of each lexical variable inside the basic block,
> +// to control whether to dynamically check uninitialized lexical variable at
> +// run time.

With the perhaps above, this paragraph isn't needed, I think.

@@ +87,5 @@
> +//
> +// When declaring a lexical variable, the variable is marked as CheckTDZ, and
> +// when the variable is initialized, it's marked as DontCheckTDZ.  When
> +// accessing the variable and if it's marked CheckTDZ, an opcode is emitted to
> +// check if the variable is initialized.

Mm.  There are a bunch of little nitpicks and English fixes and stuff I have here, and they piled up enough I should just write it all out.  How about:

"""
When a scope containing lexical variables is entered, all such variables are marked as CheckTDZ.  When a lexical variable is accessed, its entry is checked.  If it's CheckTDZ, a JSOP_CHECKLEXICAL is emitted and then the entry is marked DontCheckTDZ.  If it's DontCheckTDZ, no check is emitted because a prior check would have already failed.  Finally, because evaluating a lexical variable declaration initializes it (after any initializer is evaluated), evaluating a lexical declaration marks its entry as DontCheckTDZ.
"""

@@ +1991,5 @@
> +  public:
> +    // Whether the then and else clauses may contain declaration or access to
> +    // lexical variables, which means they should have their own TDZCheckCache.
> +    // Basically TDZCheckCache should be created for each basic block, which
> +    // then and else clauses are, but for internally used branches which are

A cache is created also for evaluating the if-else *conditions*, right?  So replace ", which...are," with " (if-else conditions, then-clauses, else-clauses)" perhaps?

@@ +1993,5 @@
> +    // lexical variables, which means they should have their own TDZCheckCache.
> +    // Basically TDZCheckCache should be created for each basic block, which
> +    // then and else clauses are, but for internally used branches which are
> +    // known not to touch lexical variables (but operate only on values on the
> +    // stack), we can skip creating TDZCheckCache for them.

The "(but...)" is not quite right, if memory serves.  Lexical variables can live on the stack if they're never captured.  So I think the parenthetical should just be removed.

@@ +2081,5 @@
>  
>    public:
> +    // See the coment above the `Kind` enum for when to specify otherwise.
> +    explicit IfEmitter(BytecodeEmitter* bce,
> +                       Kind kind = Kind::MayContainLexicalAccessInBranch)

I'm not really a fan of optional arguments when the default isn't super-obvious.

Modest proposal, interested what you think.  Make this constructor protected, make the |kind| argument mandatory.  Add a public constructor that takes no |kind| argument, merely delegates to the protected constructor passing may-contain.  Then define

class InternalIfElseEmitter final : public IfElseEmitter
{
  public:
    explicit InternalIfElseEmitter(BytecodeEmitter* bce) : IfElseEmitter(bce, may-not-contain) {}
};

and declare instances of that class in all the places where you pass in may-not-contain in this patch.

Thoughts?

@@ +2093,5 @@
>  #endif
>      {}
>  
>      ~IfEmitter()
>      {}

Could you remove this while you're here?  No need for extra noise.

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/233ac49a9f9afe6c0e1595eb367b0da618dcce09
Bug 1460154 - Part 1: Rename IfThenElseEmitter to IfEmitter. r=Yoric

https://hg.mozilla.org/integration/mozilla-inbound/rev/23afb1420cc0ed72cd475b90db1b24a1b7fc4959
Bug 1460154 - Part 2: Handle TDZCheckCache in IfEmitter. r=jwalden

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/233ac49a9f9a
https://hg.mozilla.org/mozilla-central/rev/23afb1420cc0