1090491 - Don't allocate stack slots for aliased locals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Body-level var/let

Right now we allocate slots for both unaliased and aliased locals. Aliased variables don't use their slot though, as they are stored in the call object. For a number of reasons it'd be nice to only use slots for unaliased locals:

(*) Generators store all locals in their call object. Having script->nfixed() == 0 means we don't need to push any extra (unused) values when we resume a generator and this also makes it easier to resume generators from JIT code.

(*) The interpreter and Baseline push UndefinedValue() for each local when initializing frames, we'd no longer have to do that for aliased locals. The frames also become a bit smaller.

(*) Ion snapshots and resume points depend on the number of locals and will also become smaller.

The attached patch does this for body-level locals (vars/let). I've another patch for block slots but it's a different approach and I'd like to get this in first.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8512964 [details] [diff] [review]
Part 1 - Body-level var/let

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

Looks good to me.  I was actually surprised this wasn't a bigger patch.  Probably should double-check with shu since there is some newer stuff with "lexicals" that I'm not familiar with (what is a lexical?).

::: js/src/frontend/BytecodeEmitter.cpp
@@ +161,5 @@
> +    // Assign stack slots to unaliased locals (aliased locals are stored in the
> +    // call object and don't need their own stack slots). We do this by filling
> +    // a Vector that can be used to map a local to its stack slot.
> +
> +    if (varsToFrameSlots_.length() >= script->bindings.numLocals())

How do we hit this case?

@@ +178,5 @@
> +        if (bi->aliased()) {
> +            varsToFrameSlots_.infallibleAppend(UINT32_MAX);
> +        } else {
> +            varsToFrameSlots_.infallibleAppend(slot);
> +            slot++;

Personally, I'd put the slot++ in infallibleAppend and drop all the curlies (here and below).

Comment 2

[Andy Wingo [:wingo]]

Drive-by LGTM as well, with the same question Luke had.  Jan you are a superhero.

Comment 3

[Shu-yu Guo [:shu]]

Comment on attachment 8512964 [details] [diff] [review]
Part 1 - Body-level var/let

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

Looks good!

Just recording my thought process here for posterity for why this patch preserves existing invariants about the TDZ implementation:

For the unaliased case, lexical bindings are still appended after var bindings, and updateVarsToFrameSlots is stable in the slot ordering.

For the aliased case, nothing needs to change as we already only add aliased bindings to the CallObject's slotspan.

::: js/src/frontend/BytecodeEmitter.h
@@ +120,5 @@
> +     * Only unaliased locals have stack slots assigned to them. This vector is
> +     * used to map a var index (which includes unaliased and aliased locals)
> +     * to its stack slot index.
> +     */
> +    Vector<uint32_t, 16> varsToFrameSlots_;

Nit: I'd name this and related things localsToFrameSlots_ as it covers lets too.

::: js/src/jit/BaselineFrame.cpp
@@ +92,5 @@
>          MarkLocals(this, trc, nfixed, numValueSlots());
>  
>          // Clear dead block-scoped locals.
>          while (nfixed > nlivefixed)
> +            unaliasedLocal(--nfixed).setMagic(JS_UNINITIALIZED_LEXICAL);

Getting rid of that DONT_CHECK_ALIASING argument here is so nice.

::: js/src/jsscript.h
@@ +1736,3 @@
>          return i_ < bindings_->numArgs() ? i_ : i_ - bindings_->numArgs();
>      }
> +    uint32_t varIndex() const {

Ditto on naming this 'local' here.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75de7e0fe086

Addressed all nits. Thanks for the quick reviews/feedback!

(In reply to Luke Wagner [:luke] from comment #1)
> > +    if (varsToFrameSlots_.length() >= script->bindings.numLocals())
> 
> How do we hit this case?

There's an updateNumBlockScoped call in CompileScript that can add more block slots. I added a comment.

Comment 5

[Jan de Mooij [:jandem]]

Realized I forgot to bump XDR_BYTECODE_VERSION:

https://hg.mozilla.org/integration/mozilla-inbound/rev/42da94fcdfd9

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - rm loop in ClonedBlockObject::create

As discussed on IRC, this loop no longer seems necessary. Passes all jit-tests and jstests.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75de7e0fe086
https://hg.mozilla.org/mozilla-central/rev/42da94fcdfd9

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 3 - rm unaliasedVar, fix comment

Just realized unaliasedVar() on Interpreter/BaselineFrame/AbstractFramePtr is unused. Also fixes a comment.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8514432 [details] [diff] [review]
Part 3 - rm unaliasedVar, fix comment

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

Yay removing code

Comment 10

[Jan de Mooij [:jandem]]

Parts 2 and 3:

https://hg.mozilla.org/integration/mozilla-inbound/rev/301694366e20
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4da2811e10e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/301694366e20
https://hg.mozilla.org/mozilla-central/rev/b4da2811e10e

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Refactor InterpreterFrame::markValues a bit

This patch restructures InterpreterFrame::markValues a bit to be more like the similar code in BaselineFrame::trace.

This is necessary for the next patch, but it's also nice for consistency.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Don't allocate block slots if all locals are aliased

This patch sets numBlockScoped to 0 if allLocalsAliased(). This ensures generators always have nfixed == 0 (I asserted this in GeneratorObject::create).

A more precise fix (only allocate block slots for unaliased lets, like part 1 did for body-level locals) is a lot more complicated and probably not worth it.

Comment 14

[Andy Wingo [:wingo]]

Comment on attachment 8516170 [details] [diff] [review]
Part 5 - Don't allocate block slots if all locals are aliased

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3084,5 @@
> +    // can set numBlockScoped = 0. This is nice for generators as it ensures
> +    // nfixed == 0, so we don't have to initialize any local slots when resuming
> +    // a generator.
> +    if (bce->sc->allLocalsAliased())
> +        bce->script->bindings.setNoUnaliasedBlockScoped();

The double-negative in the name is a little confusing.  Consider "setAllLocalsAliased()" or something?

::: js/src/vm/GeneratorObject.cpp
@@ +21,5 @@
>      MOZ_ASSERT(regs.stackDepth() == 0);
>      InterpreterFrame *fp = regs.fp();
>  
>      MOZ_ASSERT(fp->script()->isGenerator());
> +    MOZ_ASSERT(fp->script()->nfixed() == 0);

Nice :)

Comment 15

[Andy Wingo [:wingo]]

(In reply to Jan de Mooij [:jandem] from comment #13)
> A more precise fix (only allocate block slots for unaliased lets, like part
> 1 did for body-level locals) is a lot more complicated and probably not
> worth it.

Yeah, I just checked and while it seems there should be some suitably small fix like the one you did before, I don't see it either.

Comment 16

[Jan de Mooij [:jandem]]

Parts 4 + 5:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a2c467103b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c13267c53ab

(In reply to Andy Wingo [:wingo] from comment #14)
> The double-negative in the name is a little confusing.  Consider
> "setAllLocalsAliased()" or something?

Done.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/e3a2c467103b
https://hg.mozilla.org/mozilla-central/rev/0c13267c53ab