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 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).

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

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.

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.

Realized I forgot to bump XDR_BYTECODE_VERSION: https://hg.mozilla.org/integration/mozilla-inbound/rev/42da94fcdfd9

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

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

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

Comment on attachment 8514432 [details] [diff] [review] Part 3 - rm unaliasedVar, fix comment Review of attachment 8514432 [details] [diff] [review]: ----------------------------------------------------------------- Yay removing code

Parts 2 and 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/301694366e20 https://hg.mozilla.org/integration/mozilla-inbound/rev/b4da2811e10e

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

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.

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 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 :)

(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.

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.

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