Closed Bug 1090491 Opened 5 years ago Closed 5 years ago

Don't allocate stack slots for aliased locals

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
Attachment #8512964 - Flags: review?(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).
Attachment #8512964 - Flags: review?(luke)
Attachment #8512964 - Flags: review+
Attachment #8512964 - Flags: feedback?(shu)
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.
Attachment #8512964 - Flags: feedback?(shu) → feedback+
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.
Keywords: leave-open
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.
Attachment #8513795 - Flags: review?(shu)
Attachment #8513795 - Flags: review?(shu) → review+
Depends on: 1091757
Just realized unaliasedVar() on Interpreter/BaselineFrame/AbstractFramePtr is unused. Also fixes a comment.
Attachment #8514432 - Flags: review?(shu)
Comment on attachment 8514432 [details] [diff] [review]
Part 3 - rm unaliasedVar, fix comment

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

Yay removing code
Attachment #8514432 - Flags: review?(shu) → review+
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.
Attachment #8516152 - Flags: review?(wingo)
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.
Attachment #8516170 - Flags: review?(wingo)
Attachment #8516152 - Flags: review?(wingo) → review+
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 :)
Attachment #8516170 - Flags: review?(wingo) → review+
(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.
Blocks: 1093573
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e3a2c467103b
https://hg.mozilla.org/mozilla-central/rev/0c13267c53ab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1100623
Depends on: 1101561
No longer depends on: 1101561
Depends on: 1118559
You need to log in before you can comment on or make changes to this bug.