927782 - Remove blockChain from StackFrame, 2013 edition

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

StackFrame has space for a "blockChain" member.  It is set if HAVE_BLOCKCHAIN is set in the frame's flags.

However this member is unnecessary.  If the block contains aliased locals, it will also be present on the scope chain.  This is the case for closures, direct eval, and with.  AFAICT the block chain is only used for debugging.  However its presence has a cost: for example, there are instructions that push and pop the stack, when that has no semantic value that I can see.

This discussion has been had before.  In bug 535912, blockChain was ripped out, but that was back in pre-baseline times.  That refactor was reverted in bug 692274, due to correctness issues -- but also there were lots of interactions with other parts of the system back then (tracemonkey, the decompiler, etc).

I don't have time to do this, but I would have liked to -- the block chain is a logical part of a frame from a debugging standpoint but it doesn't need to be stored in the frame; it can be computed from a side table.  As it is, it looks like generators will have to save the block chain, which is pointless.

Comment 1

[Andy Wingo [:wingo]]

Attachment: Remove HAS_BLOCKCHAIN flag

This patch removes the HAS_BLOCKCHAIN flag in frames.  Instead the blockchain is always initialized to NULL, and its presence is indicated by whether it is NULL or not.

This is just something I had sitting around before realizing the scope of the problem.

In Bug 692274 comment 38, Luke indicates that this flag is a win.  I am surprised -- the store is dead, and to me the code is more complex with the flag than without it.  Also the store only happens for baseline code.  Anyway the right way to remove the store is to remove the "need" for the store, and this patch might be a useful step along the way for someone looking at this issue.

Comment 2

[Luke Wagner [:luke]]

(In reply to Andy Wingo [:wingo] from comment #0)
+1.  You're right, all the hot paths shouldn't need to use fp->blockChain.  Also, the static analysis of which you speak is already present: GetBlockChainAtPC.  I suspect it is missing a few corner cases though (I thought there was one with try/finally...), covered up by the fact that it is only used for error reporting.

(In reply to Andy Wingo [:wingo] from comment #1)
StackFrame::HAS_BLOCKCHAIN was only a speedup in the methodjit, which created js::StackFrames from jit code.  The baseline jit doesn't even use js::StackFrame, so any operations on StackFrame are relatively cold.  We've been un-doing optimizations and simplifying things since baseline landed, so this is a great step.

Comment 3

[Andy Wingo [:wingo]]

Hi,

(In reply to Luke Wagner [:luke] from comment #2)
> (In reply to Andy Wingo [:wingo] from comment #0)
> +1.  You're right, all the hot paths shouldn't need to use fp->blockChain. 
> Also, the static analysis of which you speak is already present:
> GetBlockChainAtPC.  I suspect it is missing a few corner cases though (I
> thought there was one with try/finally...), covered up by the fact that it
> is only used for error reporting.

So, this analysis is not really what I was thinking of.  JSOP_ENTERLET0 and JS_LEAVEBLOCK literally do nothing and should be removed -- and that leaves you without the source material for GetBlockChainAtPC, as currently implemented.  Instead at byte-compile-time one should record the PC ranges of blocks in a separate table, and not rely on the decompiler to parse the generated code, because a lexical scope without aliased locals should not generate any bytecode at all.

> (In reply to Andy Wingo [:wingo] from comment #1)
> StackFrame::HAS_BLOCKCHAIN was only a speedup in the methodjit, which
> created js::StackFrames from jit code.  The baseline jit doesn't even use
> js::StackFrame, so any operations on StackFrame are relatively cold.  We've
> been un-doing optimizations and simplifying things since baseline landed, so
> this is a great step.

FWIW that patch also removed the flag from BaselineFrame.  Dunno if that changes your impression as to its perf impact.

Comment 4

[Luke Wagner [:luke]]

(In reply to Andy Wingo [:wingo] from comment #3)
> So, this analysis is not really what I was thinking of.  JSOP_ENTERLET0 and
> JS_LEAVEBLOCK literally do nothing and should be removed -- and that leaves
> you without the source material for GetBlockChainAtPC, as currently
> implemented.  Instead at byte-compile-time one should record the PC ranges
> of blocks in a separate table, and not rely on the decompiler to parse the
> generated code, because a lexical scope without aliased locals should not
> generate any bytecode at all.

Oh yes, by all means, if you are up for the more significant refactoring :)  The ideal has long been to have non-aliased let variables use JSOP_(GET|SET)LOCAL ops instead of the crazy dynamic stack pushing/popping.  This would also have the free benefit of making non-aliased let access compile in Ion.  +2!

> FWIW that patch also removed the flag from BaselineFrame.  Dunno if that
> changes your impression as to its perf impact.

It makes sense that a single extra store on baseline's call path wouldn't matter since it's not the high-end jit; I assume this was measured though?  Anyhow, I'm not the perf sheriff; I was just commenting for historical interest :)

Comment 5

[Andy Wingo [:wingo]]

After bug 932312 and bug 932276, I thought we might be close to a solution.  However, debugging ruins the party.

Let's imagine that we have a loop like this on the console:

  function () { for (let x = 0; x < 10; x++) print(x) }

Right now the disassembly is:

    00000:  zero
    00001:  enterlet0 depth 0 {x: 0}
    00006:  nop
    00007:  goto 40 (+33)
    00012:  loophead
    00013:  callgname "print"
    00018:  undefined
    00019:  notearg
    00020:  getlocal 0
    00023:  notearg
    00024:  call 1
    00027:  pop
    00028:  getlocal 0
    00031:  pos
    00032:  dup
    00033:  one
    00034:  add
    00035:  setlocal 0
    00038:  pop
    00039:  pop
    00040:  loopentry 1
    00042:  getlocal 0
    00045:  int8 10
    00047:  lt
    00048:  ifne 12 (-36)
    00053:  leaveblock 1
    00056:  stop

Gaah, that's trash -- bug 854037.  Let's munge it to put the let inside the for:

    00000:  zero
    // 00001:  enterlet0 depth 0 {x: 0}
    00006:  nop
    00007:  goto 40 (+33)
    00012:  loophead
    00013:  callgname "print"
    00018:  undefined
    00019:  notearg
    00020:  getlocal 0
    00023:  notearg
    00024:  call 1
    00027:  pop
    00028:  getlocal 0
    00031:  pos
    00032:  dup
    00033:  one
    00034:  add
    00035:  setlocal 0
    00038:  pop
    00039:  pop
        A:  leaveforletin   // Doesn't pop block locals.
    00040:  loopentry 1
        B:  enterlet0 depth 0 {x:0} // Bind top-of-stack as "x"; initial value
                            // from initializer or from previous iteration.
    00042:  getlocal 0
    00045:  int8 10
    00047:  lt
    00048:  ifne 12 (-36)
    00053:  leaveblock 1
    00056:  stop

If a breakpoint is set at the print, the debugger will reify a full scope chain, including blocks, activations, "with" statements, and the global.  Accesses to block-scoped locals go through this reified chain, but there is a proxy in place to redirect access to unaliased locals to the stack, if the scope is still active.  There are hooks in the "leaveforletin" instruction to mark a DebugScope as being inactive.  And there's the rub!

If you think about removing the block chain, and assume you replace it with a "pushblockscope" instruction only if there are aliased locals, then in this loop literally the enterlet0 does nothing.  And so it should be removed -- having a VM op for the benefit of the debugger is an antipattern.
If we record the static block information (bug 932376) we know that in the ranges [12, A) and [B,48) there is a block scope.  However there would be nowhere to hang a hook indicating that we are leaving a scope -- no LEAVEBLOCK.

It is possible to re-build such a callback using per-instruction hooks and nonlocal exit hooks -- http://www.gnu.org/software/guile/manual/html_node/Traps.html#Traps for an example -- but that's a bit of work.

Anyway, what I'm saying is that everything is in place to optimize "let", I think (will post some more patches), and remove blockChain from StackFrame, except for the debugger.  Bummer.

Comment 6

[Andy Wingo [:wingo]]

Tentative plan is to add a DEBUGLEAVEBLOCK op that does the DebugScopes::onPopBlock call, and punt that problem down the road.  Then I would change ENTERBLOCK/ENTERLET0/... to be PUSHBLOCKSCOPE, but only if there are aliased locals, and likewise with POPBLOCKSCOPE.  I have most of a patch ready I think.  Without aliased locals it should even allow Ion compilation, as Luke noted.

Comment 7

[Andy Wingo [:wingo]]

Attachment: Expose JSScript::getBlockScope(jsbytecode *)

Comment 8

[Andy Wingo [:wingo]]

Comment on attachment 818337 [details] [diff] [review]
Remove HAS_BLOCKCHAIN flag

Marking the remove has-blockchain patch as obsolete, as I push on my new 12-part patch series (gulp!)

Comment 9

[Andy Wingo [:wingo]]

Attachment: Remove HAS_BLOCKCHAIN

Comment 10

[Andy Wingo [:wingo]]

Attachment: Add DEBUGLEAVEBLOCK opcode to invalidate live DebugScopes

Comment 11

[Andy Wingo [:wingo]]

Attachment: Generators allocate all locals on the scope chain

Comment 12

[Andy Wingo [:wingo]]

Attachment: Clean up bytecode generation for catch clauses

Comment 13

[Andy Wingo [:wingo]]

Attachment: Fix for-let nesting so nonlocal exits are easier.

Comment 14

[Andy Wingo [:wingo]]

Attachment: Refactor entering and leaving block scopes in BytecodeEmitter

Comment 15

[Andy Wingo [:wingo]]

Attachment: Record block scope ranges more precisely

Comment 16

[Andy Wingo [:wingo]]

Attachment: Fix js_DumpScriptDepth to not abort on unreachable bytecodes

Comment 17

[Andy Wingo [:wingo]]

Attachment: Iterate block chain from compile-time block scope maps, not runtime blockChain

Comment 18

[Andy Wingo [:wingo]]

Attachment: Optimize block scopes without aliased locals

Comment 19

[Andy Wingo [:wingo]]

Attachment: Remove blockChain from StackFrame

Comment 20

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=d455b374edc3

Comment 21

[Andy Wingo [:wingo]]

Whew.  That was gnarly.  Attachment 8335318 [details] [diff] is the gnarliest, as it is the one with the responsibility of ensuring that the compile-time-computed scope map really matches what we see at runtime with the blockChain.  Attachment 8335336 [details] [diff] is the raddest, because it lets let-bound variables be Ion-compiled.

Other than that, it's just a small set of refactors.  See https://bugzilla.mozilla.org/attachment.cgi?id=8335336&action=diff#a/js/src/frontend/BytecodeEmitter.cpp_sec5 for the design.  It could merge to trunk at any point, but some of the earlier patches pessimize things, so ideally the whole set could go in over a short-ish period of time.

TBPL here: https://tbpl.mozilla.org/?tree=Try&rev=d455b374edc3

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 8335294 [details] [diff] [review]
Generators allocate all locals on the scope chain

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

Good riddance.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +678,5 @@
>  }
>  
> +#ifdef DEBUG
> +static bool
> +AllLocalsAliased(StaticBlockObject &obj)

Perhaps you could move this close to its use below?

::: js/src/vm/ScopeObject.cpp
@@ +1629,5 @@
>      if (!CanUseDebugScopeMaps(cx))
>          return true;
>  
> +    if (si.frame().isFunctionFrame() && si.frame().callee()->isGenerator())
> +        return true;

IIUC, there is only one caller, GetDebugScopeForMissing, and you've ensured/asserted that generator scopes are never missing.  If that's right, I think you could replace this with an assert (up next to the others above).

Comment 23

[Luke Wagner [:luke]]

Comment on attachment 8335296 [details] [diff] [review]
Clean up bytecode generation for catch clauses

Having little experience with how we emit try, I'm going to punt this one to Jason.

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 8335298 [details] [diff] [review]
Fix for-let nesting so nonlocal exits are easier.

Nice

Comment 25

[Luke Wagner [:luke]]

Comment on attachment 8335306 [details] [diff] [review]
Refactor entering and leaving block scopes in BytecodeEmitter

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +143,5 @@
>  
>  static StaticBlockObject &
> +BlockAtPC(BytecodeEmitter *bce, jsbytecode *pc)
> +{
> +    uint32_t index = GET_UINT32_INDEX(pc);

I think you'll need a DebugOnly<uint32_t> to avoid an opt 'defined but used' warning.

@@ +145,5 @@
> +BlockAtPC(BytecodeEmitter *bce, jsbytecode *pc)
> +{
> +    uint32_t index = GET_UINT32_INDEX(pc);
> +    JS_ASSERT(index < bce->objectList.length);
> +    JS_ASSERT(index == bce->objectList.length - 1);

With this precondition, it seems like we should make the name sound less general.  Perhaps LastBlockAdded(bce, pc)?

@@ +2318,5 @@
>      int32_t low, high;
>      int noteIndex;
>      size_t switchSize;
>      jsbytecode *pc;
> +    StaticBlockObject *blockObj = nullptr;

Could you move this decl down right before its first use?

Comment 26

[Luke Wagner [:luke]]

Comment on attachment 8335312 [details] [diff] [review]
Record block scope ranges more precisely

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

One non-nit question below:

::: js/src/frontend/BytecodeEmitter.cpp
@@ +523,5 @@
>      return true;
>  }
>  
> +namespace {
> +    

Trailing whitespace

@@ +528,5 @@
> +class NonLocalExitScope {
> +    ExclusiveContext *cx;
> +    BytecodeEmitter *bce;
> +    uint32_t savedScopeIndex;
> +    int savedDepth;

Can you make these both 'const'?

@@ +532,5 @@
> +    int savedDepth;
> +
> +    NonLocalExitScope(const NonLocalExitScope &) MOZ_DELETE;
> +
> +public:

Two spaces before 'public:'

@@ +538,5 @@
> +      : cx(cx_)
> +      , bce(bce_)
> +      , savedScopeIndex(bce->blockScopeList.length())
> +      , savedDepth(bce->stackDepth)
> +    {}

SM style is the commas go at the end of the line (https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#Initializer_lists).

::: js/src/jsscript.cpp
@@ +2851,3 @@
>              blockChain = getObject(note->index);
> +            if (note->flags & BlockScopeNote::PopForNonLocalExit)
> +                blockChain = blockChain->as<StaticBlockObject>().enclosingBlock();

Instead of having a PopForNonLocalExit flag, could you instead having note->index just be the index for blockChain->enclosingBlock()?

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8335318 [details] [diff] [review]
Iterate block chain from compile-time block scope maps, not runtime blockChain

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

I have two algorithmic concerns with this patch:

1. With this patch we'll be calling getBlockChain in many common situations, and this function is O(n) with the number of blocks which means we'll open ourselves up to unexpected slowdowns in large scripts with many lets.  While this sounds pathological, this stuff always tends to show up.  One thought is that getBlockChain could use a binary search.  Unfortunately, it's a tree, so we'd have to tweak the block notes to store a linear list of un-nested segments.  That is, for:
  let (x) {
    let (y) {}
  }
we'd break the notes [[y], [x]] into [[x-before], [y], [x-after]].

2. The patch makes JSAbstractIterator::{scopeChain,callObj} take O(n), where n is the distance of the frame from the young end of the stack.  This is pretty scary because, last I recall, Firebug uses this API to iterate over each frame of the stack, calling scopeChain on each one, so we can easily get O(n^2) behavior where n can be quite large (40k, if you hit a stack overflow).  For this, I get the feeling that maybe we can do one of (1) can remove JSAbstractFrameIter and use JSBrokenFrameIterator in all those places or (2) make JSAbstractFrameIter also store 'pc'.  On (2): the interface seems to indicate that JSAbstractFrameIter has to fit in a word, but I can't see anyone really using this.

::: js/src/jit/BaselineFrame.h
@@ +169,5 @@
>  
>      Value &unaliasedLocal(unsigned i, MaybeCheckAliasing checkAliasing = CHECK_ALIASING) const {
>  #ifdef DEBUG
> +        // FIXME?
> +        CheckLocalUnaliased(checkAliasing, script(), nullptr, i);

Can you pass GetBlockChainAtPC?

::: js/src/vm/Interpreter.h
@@ +316,5 @@
>  extern bool
>  HasInstance(JSContext *cx, HandleObject obj, HandleValue v, bool *bp);
>  
>  /* Unwind block and scope chains to match the given depth. */
> +class ScopeIter;

Can you put this near the top so the comment can still precede the function?

::: js/src/vm/ScopeObject.cpp
@@ +1110,2 @@
>                      if (action == GET)
> +                        vp.set(maybeLiveScope->frame().unaliasedVar(i));

For regularity with the following two cases, can you have
  AbstractFramePtr frame = maybeLiveScope->frame();
here too?

Comment 28

[Andy Wingo [:wingo]]

Comment on attachment 8335312 [details] [diff] [review]
Record block scope ranges more precisely

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

Thanks for looking at this.

::: js/src/jsscript.cpp
@@ +2851,3 @@
>              blockChain = getObject(note->index);
> +            if (note->flags & BlockScopeNote::PopForNonLocalExit)
> +                blockChain = blockChain->as<StaticBlockObject>().enclosingBlock();

I wanted to do that originally, but you need to be able to encode "no block scope" as well.  This scheme allows you to encode that by saying "the enclosing block scope of object N", which may be null.  We could reserve a special value instead, if we want to use the fourth blockscopenote word as some kind of cursor.

Comment 29

[Andy Wingo [:wingo]]

Comment on attachment 8335318 [details] [diff] [review]
Iterate block chain from compile-time block scope maps, not runtime blockChain


(In reply to Luke Wagner [:luke] from comment #27)
> I have two algorithmic concerns with this patch:
> 
> 1. With this patch we'll be calling getBlockChain in many common situations,

We should enumerate the situations.

Outside of debug mode, we only call getBlockScope when unwinding a scope due to a nonlocal exit (via "throw").

In debug mode, you call getBlockScope when building a DebugScope proxy scope chain (once per frame), and when leaving a block scope (DEBUGLEAVEBLOCK).

That's it.  There are some additional cases in this patch that are removed in attachment 8335336 [details] [diff] [review].  

I agree it can be improved (and probably should be).  It doesn't seem to me to be a showstopper; though you know "what occurs in the wild" much better than I do :)

>   let (x) {
>     let (y) {}
>   }
> we'd break the notes [[y], [x]] into [[x-before], [y], [x-after]].

Good idea.  Followup material I guess.

> 2. The patch makes JSAbstractIterator::{scopeChain,callObj} take O(n), where
> n is the distance of the frame from the young end of the stack.  This is
> pretty scary because, last I recall, Firebug uses this API to iterate over
> each frame of the stack, calling scopeChain on each one, so we can easily
> get O(n^2) behavior where n can be quite large (40k, if you hit a stack
> overflow).  For this, I get the feeling that maybe we can do one of (1) can
> remove JSAbstractFrameIter and use JSBrokenFrameIterator in all those places
> or (2) make JSAbstractFrameIter also store 'pc'.  On (2): the interface
> seems to indicate that JSAbstractFrameIter has to fit in a word, but I can't
> see anyone really using this.

How does this change JSAbstractIterator::{scopeChain,callObj} ?  I am missing that connection.  The scope chain is still accessible directly from the stack frame.  Again probably I am overlooking something.

Comment 30

[Andy Wingo [:wingo]]

(In reply to Andy Wingo [:wingo] from comment #29)
> Outside of debug mode, we only call getBlockScope when unwinding a scope due
> to a nonlocal exit (via "throw").

Now that I think about it, I wonder why we do this in non-debug mode.  The thing about the block chain is that it has no semantic value, so we shouldn't have to do anything in that case.

Comment 31

[Luke Wagner [:luke]]

(In reply to Andy Wingo [:wingo] from comment #28)
> I wanted to do that originally, but you need to be able to encode "no block
> scope" as well.

Could you use a sentinel block index like UINT32_MAX (given a proper symbolic name, of course)?

(In reply to Andy Wingo [:wingo] from comment #29)
> > 1. With this patch we'll be calling getBlockChain in many common situations,
> 
> We should enumerate the situations.
[...]

Ok, that's better than I thought; no need to fix in this bug.  In the worst case, somebody reports some slowdowns when throwing/debugging in some crazy long let-filled function and we just fix it then.  Probably that day is far off, though.

> How does this change JSAbstractIterator::{scopeChain,callObj} ?  I am
> missing that connection.  The scope chain is still accessible directly from
> the stack frame.  Again probably I am overlooking something.

Oops, I mistyped: I meant JSAbstractFramePtr::{scopeChain,callObj}, which now call FindPCForLiveFrame.

Comment 32

[Luke Wagner [:luke]]

(In reply to Andy Wingo [:wingo] from comment #30)
> Now that I think about it, I wonder why we do this in non-debug mode.  The
> thing about the block chain is that it has no semantic value, so we
> shouldn't have to do anything in that case.

On first glance: UnwindScope uses ScopeIter and ScopeIter internally uses 'block'.  IIRC, the tricky bit is this: just because you see a Block on the scope chain in UnwindScope, you can't just compare it's stackDepth() because it might be a Block from your *enclosing* scope, so you need to know your static scope via the blockChain.

Comment 33

[Luke Wagner [:luke]]

Comment on attachment 8335336 [details] [diff] [review]
Optimize block scopes without aliased locals

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +581,5 @@
>              if (!bce->blockScopeList.appendPopForNonLocalExit(scopeObjectIndex, bce->offset()))
>                  return false;
> +            if (blockObj.needsClone())
> +                if (Emit1(cx, bce, JSOP_POPBLOCKSCOPE) < 0)
> +                    return false;

need { } around multi-line then branch.

@@ +797,5 @@
>          return false;
>  
> +    if (blockObj.needsClone())
> +        if (!EmitInternedObjectOp(cx, scopeObjectIndex, JSOP_PUSHBLOCKSCOPE, bce))
> +            return false;

{ }

@@ +853,5 @@
>      bce->blockScopeList.recordEnd(blockScopeIndex, bce->offset());
>  
> +    if (blockOnChain)
> +        if (Emit1(cx, bce, JSOP_POPBLOCKSCOPE) < 0)
> +            return false;

{ }

::: js/src/vm/Interpreter.cpp
@@ +3372,5 @@
> +#ifdef DEBUG
> +#if 0
> +    // Pop block from scope chain.
> +    // FIXME: Assertions currently worthless due to nonlocal exits like "return"
> +    // unwinding the stack.

In this case (and since this isn't fixed in a later patch), I'd just leave this out.  We generally try not to have #if 0'd hunks lying around since they usually rot pretty quickly and clutter.

::: js/src/vm/Stack.cpp
@@ +327,5 @@
>  bool
>  StackFrame::pushBlock(JSContext *cx, StaticBlockObject &block)
>  {
> +    JS_ASSERT (block.needsClone());
> +    

Trailing whitespace

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 8335345 [details] [diff] [review]
Remove blockChain from StackFrame

Ah, my favorite review.

Comment 35

[Andy Wingo [:wingo]]

(In reply to Luke Wagner [:luke] from comment #22)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +678,5 @@
> >  }
> >  
> > +#ifdef DEBUG
> > +static bool
> > +AllLocalsAliased(StaticBlockObject &obj)
> 
> Perhaps you could move this close to its use below?

Done

> ::: js/src/vm/ScopeObject.cpp
> @@ +1629,5 @@
> >      if (!CanUseDebugScopeMaps(cx))
> >          return true;
> >  
> > +    if (si.frame().isFunctionFrame() && si.frame().callee()->isGenerator())
> > +        return true;
> 
> IIUC, there is only one caller, GetDebugScopeForMissing, and you've
> ensured/asserted that generator scopes are never missing.  If that's right,
> I think you could replace this with an assert (up next to the others above).

Done.

Comment 36

[Andy Wingo [:wingo]]

Attachment: Generators allocate all locals on the scope chain

Comment 37

[Andy Wingo [:wingo]]

Attachment: 1/12: Expose JSScript::getBlockScope(jsbytecode *)

Comment 38

[Andy Wingo [:wingo]]

Attachment: 2/12: Remove HAS_BLOCKCHAIN

Comment 39

[Andy Wingo [:wingo]]

Attachment: 3/12: Add DEBUGLEAVEBLOCK opcode to invalidate live DebugScopes

Comment 40

[Andy Wingo [:wingo]]

Attachment: 4/12: Generators allocate all locals on the scope chain

Comment 41

[Andy Wingo [:wingo]]

Comment on attachment 8337672 [details] [diff] [review]
4/12: Generators allocate all locals on the scope chain

Marked patches 1 through 4 inclusive r=luke, and updated patch 4 according to comments.

Comment 42

[Andy Wingo [:wingo]]

Full TBPL with patch-set from today: https://tbpl.mozilla.org/?tree=Try&rev=c804cf3a7be0

Comment 43

[Jan de Mooij [:jandem]]

Comment on attachment 8337671 [details] [diff] [review]
3/12: Add DEBUGLEAVEBLOCK opcode to invalidate live DebugScopes

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

::: js/src/jit/BaselineCompiler.cpp
@@ +2605,5 @@
> +bool
> +BaselineCompiler::emit_JSOP_DEBUGLEAVEBLOCK()
> +{
> +    // Call a stub to pop the block from the block chain.
> +    prepareVMCall();

Drive-by comment: Baseline JIT code is invalidated when we enter debug mode, so you could wrap this all in an "if (debugMode_)", or add:

if (!debugMode_)
    return true;

At the start of this method.

Comment 44

[Andy Wingo [:wingo]]

(In reply to Jan de Mooij [:jandem] from comment #43)
> Comment on attachment 8337671 [details] [diff] [review]
> 3/12: Add DEBUGLEAVEBLOCK opcode to invalidate live DebugScopes
> 
> Review of attachment 8337671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/BaselineCompiler.cpp
> @@ +2605,5 @@
> > +bool
> > +BaselineCompiler::emit_JSOP_DEBUGLEAVEBLOCK()
> > +{
> > +    // Call a stub to pop the block from the block chain.
> > +    prepareVMCall();
> 
> Drive-by comment: Baseline JIT code is invalidated when we enter debug mode,
> so you could wrap this all in an "if (debugMode_)", or add:
> 
> if (!debugMode_)
>     return true;
> 
> At the start of this method.

True!  And indeed somehow this changed ended up in attachment 8335318 [details] [diff] [review], later in the series.

Comment 45

[Jason Orendorff [:jorendorff]]

Comment on attachment 8335296 [details] [diff] [review]
Clean up bytecode generation for catch clauses

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

Totally cool.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +3853,5 @@
> +        uint32_t blockObjIndex = bce->blockScopeList.list[blockScopeIndex].index;
> +        ObjectBox *blockObjBox = bce->objectList.find(blockObjIndex);
> +        StaticBlockObject *blockObj = &blockObjBox->object->as<StaticBlockObject>();
> +        JS_ASSERT(blockObj == bce->blockChain);
> +#endif

This is an invariant, right? We could assert this pretty much anywhere in the emitter, as long as topStmt is a block scope? Should we make it a method, and check in the places where these values are modified instead?

I just wonder if I'm missing something. Why assert this *here*? Are we doing anything in this function that especially relies on the asserted property?

Without a comment, future readers may wonder the same thing.

@@ +3947,5 @@
> +        // The emitted code for a catch block looks like:
> +        //
> +        // enterblock
> +        // exception
> +        // if there is a catchguard:

The real bytecode has code after `exception` to store the exception in the new slot(s) pushed by `enterblock`:

    enterblock {V}
    exception
    <V = exception>
    pop

Could you change the comment?

Also, could you please take a quick look at the bytecode and see if it emits a `pop; dup` pair, for each catch guard, that can be easily eliminated? That is, something like this:

        enterblock
        exception
        <V = exception>
-       pop
        if there is a catchguard:
-           dup
            < catchguard code >
            ifne POST
            throwing
            debugleaveblock
            leaveblock
            goto <next catch block>
           POST:
-           pop
+       pop
        < catch block statements >
        ...

This is not a big deal at all. A separate followup bug would be fine.

Comment 46

[Andy Wingo [:wingo]]

Thank you for the review, Jason!

(In reply to Jason Orendorff [:jorendorff] from comment #45)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +3853,5 @@
> > +        uint32_t blockObjIndex = bce->blockScopeList.list[blockScopeIndex].index;
> > +        ObjectBox *blockObjBox = bce->objectList.find(blockObjIndex);
> > +        StaticBlockObject *blockObj = &blockObjBox->object->as<StaticBlockObject>();
> > +        JS_ASSERT(blockObj == bce->blockChain);
> > +#endif
> 
> This is an invariant, right? We could assert this pretty much anywhere in
> the emitter, as long as topStmt is a block scope? Should we make it a
> method, and check in the places where these values are modified instead?

Right.  However the only place we know that topStmt will be a block scope is when exiting a scope.  This is one of the only places we do so nonlocally, which is why this check is here.  It goes away later when we add the NonLocalExitScope: https://bugzilla.mozilla.org/attachment.cgi?id=8335312&action=diff#a/js/src/frontend/BytecodeEmitter.cpp_sec5

I think this check's unfactored presence here is just an artifact of splitting the patch.  Given that this code gets better in a later patch I will just leave it as it is, if that is OK.

> @@ +3947,5 @@
> > +        // The emitted code for a catch block looks like:
> > +        //
> > +        // enterblock
> > +        // exception
> > +        // if there is a catchguard:
> 
> The real bytecode has code after `exception` to store the exception in the
> new slot(s) pushed by `enterblock`:
> 
>     enterblock {V}
>     exception
>     <V = exception>
>     pop
> 
> Could you change the comment?

Sure.  The diff looks like this:

         // The emitted code for a catch block looks like:
         //
         // enterblock
         // exception
+        // if there is a catchguard:
         //   dup
+        // setlocal 0; pop              assign or possibly destructure exception
         // if there is a catchguard:
         //   < catchguard code >
         //   ifne POST
         //   throwing                   pop exception to cx->exception
         //   debugleaveblock
         //   leaveblock
         //   goto <next catch block>
         //   POST: pop
         // < catch block contents >

> Also, could you please take a quick look at the bytecode and see if it emits
> a `pop; dup` pair, for each catch guard, that can be easily eliminated? That
> is, something like this:
> 
>         enterblock
>         exception
>         <V = exception>
> -       pop
>         if there is a catchguard:
> -           dup
>             < catchguard code >
>             ifne POST
>             throwing
>             debugleaveblock
>             leaveblock
>             goto <next catch block>
>            POST:
> -           pop
> +       pop
>         < catch block statements >
>         ...
> 
> This is not a big deal at all. A separate followup bug would be fine.

It's not clear to me that you can avoid the dup in the general case.  Here's a bare try/catch with this patch:

js> print(disassemble(function() { try { 1 } catch (e) { e } }))
flags: LAMBDA
loc     op
-----   --
main:
00000:  try 6 (+6)
00001:  goto 26 (+25)
00006:  enterblock depth 0 {e: 0}
00011:  exception
00012:  setlocal 0
00015:  pop
00016:  debugleaveblock
00017:  leaveblock 1
00020:  goto 26 (+6)
00025:  nop
00026:  retrval

At the end of this patch series it is the same except with no ENTERBLOCK or LEAVEBLOCK instructions.  Anyway.  We see no dup.  If I do this:

js> print(disassemble(function() { try { 1 } catch (e if e instanceof foo) { e } catch (e) { e } }))
flags: LAMBDA
loc     op
-----   --
main:
00000:  try 6 (+6)
00001:  goto 71 (+70)
00006:  enterblock depth 0 {e: 0}
00011:  exception
00012:  dup
00013:  setlocal 0
00016:  pop
00017:  getlocal 0
00020:  name "foo"
00025:  instanceof
00026:  ifne 41 (+15)
00031:  throwing
00032:  debugleaveblock
00033:  leaveblock 1
00036:  goto 51 (+15)
00041:  pop
00042:  debugleaveblock
00043:  leaveblock 1
00046:  goto 71 (+25)
00051:  enterblock depth 0 {e: 0}
00056:  exception
00057:  setlocal 0
00060:  pop
00061:  debugleaveblock
00062:  leaveblock 1
00065:  goto 71 (+6)
00070:  nop
00071:  retrval

How to avoid the dup at 00012?  I don't know tbh.  The guard expression can be (e if (e = true)), or it can destructure, so we can't re-use the lexical slot.

Comment 47

[Andy Wingo [:wingo]]

Attachment: 5/12: Clean up bytecode generation for catch clauses

Comment 48

[Andy Wingo [:wingo]]

Attachment: 6/12: Fix for-let nesting so nonlocal exits are easier.

Comment 49

[Andy Wingo [:wingo]]

Attachment: 7/12: Refactor entering and leaving block scopes in BytecodeEmitter

Comment 50

[Andy Wingo [:wingo]]

Attachment: 8/12: Record block scope ranges more precisely UNREVIEWED

Comment 51

[Andy Wingo [:wingo]]

Updated attachment 8338394 [details] [diff] [review] in response to Luke's comment 26.  PTAL :)

Comment 52

[Andy Wingo [:wingo]]

Attachment: 9/12: Fix js_DumpScriptDepth to not abort on unreachable bytecodes

Comment 53

[Andy Wingo [:wingo]]

Attachment: 10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain UNREVIEWED

Comment 54

[Andy Wingo [:wingo]]

Attachment: 10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain UNREVIEWED

Comment 55

[Andy Wingo [:wingo]]

Attachment: 11/12: Optimize block scopes without aliased locals

Comment 56

[Andy Wingo [:wingo]]

(In reply to Luke Wagner [:luke] from comment #33)
> Comment on attachment 8335336 [details] [diff] [review]
> Optimize block scopes without aliased locals

Fixed nits.

> ::: js/src/vm/Interpreter.cpp
> @@ +3372,5 @@
> > +#ifdef DEBUG
> > +#if 0
> > +    // Pop block from scope chain.
> > +    // FIXME: Assertions currently worthless due to nonlocal exits like "return"
> > +    // unwinding the stack.
> 
> In this case (and since this isn't fixed in a later patch), I'd just leave
> this out.  We generally try not to have #if 0'd hunks lying around since
> they usually rot pretty quickly and clutter.

Good catch -- a relic of a thing that didn't work before but now does.  Actually the assertion is fine, I have enabled it now.

(In reply to Luke Wagner [:luke] from comment #27)
> Comment on attachment 8335318 [details] [diff] [review]
> Iterate block chain from compile-time block scope maps, not runtime
> blockChain
> ::: js/src/jit/BaselineFrame.h
> @@ +169,5 @@
> >  
> >      Value &unaliasedLocal(unsigned i, MaybeCheckAliasing checkAliasing = CHECK_ALIASING) const {
> >  #ifdef DEBUG
> > +        // FIXME?
> > +        CheckLocalUnaliased(checkAliasing, script(), nullptr, i);
> 
> Can you pass GetBlockChainAtPC?

I changed to leave this as it was in patch 10/12, but remove the block argument in patch 11/12.  It's really inconvenient to plumb through the PC here, which we would need to check the block scope.  With my change it checks for unaliased var locals but not blocks :(  A regression in that regard, but I couldn't help it.  Setting r? on patch 11 to check that this is OK.

Comment 57

[Andy Wingo [:wingo]]

Attachment: 12/12: Remove blockChain from StackFrame

Comment 58

[Luke Wagner [:luke]]

Comment on attachment 8338374 [details] [diff] [review]
8/12: Record block scope ranges more precisely  UNREVIEWED

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6910,5 @@
> +    while (index--) {
> +        JS_ASSERT(list[index].start <= pos);
> +        if (list[index].length == 0) {
> +            // We are looking for the nearest enclosing live scope.  If the
> +            // scope contains POS, it should still be open, so its length should

Perhaps s/POS/'pos'/ (here and below)?

Comment 59

[Luke Wagner [:luke]]

I'm sorry if I missed this earlier in the comments (per-bug threaded comments would be nice) but for patch 10/12 I think my initial concern still remains to be addressed (the O(n) stack search in FindPCForLiveFrame called by JSAbstractFramePtr::scopeChain/callObject).  IIUC (but please do lmk if I'm wrong), these functions are called by Firebug for all frames in the callstack which makes it pretty easy to get into O(n^2) performance for really deep stacks.  We've run into this problem several times before (in jsd and xpc) which is why I'm worried.

Comment 60

[Andy Wingo [:wingo]]

(In reply to Luke Wagner [:luke] from comment #59)
> I'm sorry if I missed this earlier in the comments (per-bug threaded
> comments would be nice) but for patch 10/12 I think my initial concern still
> remains to be addressed (the O(n) stack search in FindPCForLiveFrame called
> by JSAbstractFramePtr::scopeChain/callObject).  IIUC (but please do lmk if
> I'm wrong), these functions are called by Firebug for all frames in the
> callstack which makes it pretty easy to get into O(n^2) performance for
> really deep stacks.  We've run into this problem several times before (in
> jsd and xpc) which is why I'm worried.

Will take a look tomorrow; thanks for the pointer.

Comment 61

[Jason Orendorff [:jorendorff]]

(In reply to Andy Wingo [:wingo] from comment #46)
> I think this check's unfactored presence here is just an artifact of
> splitting the patch.  Given that this code gets better in a later patch I
> will just leave it as it is, if that is OK.

Sure!

> How to avoid the dup at 00012?  I don't know tbh.

Hmm. Still looks possible to me, but I'll file the follow-up bug if I feel like pursuing it. This patch is fine as-is.

Comment 62

[Andy Wingo [:wingo]]

Attachment: 4/12: Generators allocate all locals on the scope chain

Comment 63

[Andy Wingo [:wingo]]

Attachment: 10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain UNREVIEWED

Comment 64

[Andy Wingo [:wingo]]

Comment on attachment 8341634 [details] [diff] [review]
4/12: Generators allocate all locals on the scope chain

Hi Luke, setting r? again on this one in light of bug 943409.  This patch now includes a small change to the indexedDB tests that removes the

  (function () { objectStore })();

that was previously used to force a particular local to be on the scope chain, and thus participate in a cycle; as now with this patch it will be on the scope chain anyway.

Comment 65

[Andy Wingo [:wingo]]

Comment on attachment 8341635 [details] [diff] [review]
10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain  UNREVIEWED

I fixed the FindPCForLiveFrame calls in this patch by making JSAbstractFramePtr a two-word object.  It's still passed by value in some places but in my understanding of things this shouldn't be a problem.  In order to provide the pc to all of its constructors, I had to add pc to a couple of baseline/interpreter debugging calls.

Comment 66

[Andy Wingo [:wingo]]

Happily, only the patches that needed review didn't apply cleanly to current m-c; the rest are good as-is.  In-progress TBPL here: https://tbpl.mozilla.org/?tree=Try&rev=b1a2ecc529f7

Comment 67

[Luke Wagner [:luke]]

Comment on attachment 8341635 [details] [diff] [review]
10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain  UNREVIEWED

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

Nice fix.  Overall, fantastic job!  I appreciate the patch splitting.  Sorry you had to deal with the CC.

::: js/src/vm/Interpreter.cpp
@@ +886,4 @@
>  void
>  js::UnwindForUncatchableException(JSContext *cx, const FrameRegs &regs)
>  {
>      /* c.f. the regular (catchable) TryNoteIter loop in Interpret. */

Can you update this comment to refer to HandleError instead?

@@ +963,5 @@
> +    FinallyContinuation
> +};
> +
> +static HandleErrorContinuation
> +HandleError(JSContext *cx, FrameRegs &regs)

I love what you did with this whole hoisting and refactoring.  Error-handling in the interpreter might just be approaching sanity...

@@ +1761,5 @@
>       * false after the inline_return label.
>       */
>      CHECK_BRANCH();
>  
> +  successfulReturnContinuation:

The label naming style in Interpret seems to be underbars_and_lowercase.

Comment 68

[Andy Wingo [:wingo]]

Attachment: 4/12: Generators allocate all locals on the scope chain

Comment 69

[Andy Wingo [:wingo]]

Attachment: 10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain

Comment 70

[Andy Wingo [:wingo]]

Comment on attachment 8341855 [details] [diff] [review]
4/12: Generators allocate all locals on the scope chain

Uploaded new version with the bug-inducing part from the additional two tests of 945813.  r=luke

Comment 71

[Andy Wingo [:wingo]]

Comment on attachment 8341859 [details] [diff] [review]
10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain

Addressed nits; r=luke.  Thanks luke for the detailed reviews!

Comment 72

[Andy Wingo [:wingo]]

Currently waiting on https://tbpl.mozilla.org/?tree=Try&rev=490d53f8a704 to go green.  Quasiquoth Nina Simone, o/~ let it be greeeeeeen o/~

Comment 73

[Andy Wingo [:wingo]]

Attachment: 4/12: Generators allocate all locals on the scope chain

Comment 74

[Andy Wingo [:wingo]]

Comment on attachment 8342394 [details] [diff] [review]
4/12: Generators allocate all locals on the scope chain

New patch adapts to apply cleanly over changes in bug 945813.

Comment 75

[Andy Wingo [:wingo]]

Looking green! https://tbpl.mozilla.org/?tree=Try&rev=b2adb9d79006

Comment 76

[Ryan VanderMeulen [:RyanVM]]

Some of these patches are having difficulties applying. Please rebase them :(

Comment 77

[Andy Wingo [:wingo]]

Attachment: 1/12: Expose JSScript::getBlockScope(jsbytecode *)

Comment 78

[Andy Wingo [:wingo]]

Attachment: 4/12: Generators allocate all locals on the scope chain

Comment 79

[Andy Wingo [:wingo]]

Attachment: 7/12: Refactor entering and leaving block scopes in BytecodeEmitter

Comment 80

[Andy Wingo [:wingo]]

Attachment: 8/12: Record block scope ranges more precisely

Comment 81

[Andy Wingo [:wingo]]

Attachment: 9/12: Fix js_DumpScriptDepth to not abort on unreachable bytecodes

Comment 82

[Andy Wingo [:wingo]]

Attachment: 10/12: Iterate block chain from compile-time block scope maps, not runtime blockChain

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d99e9ca7b32
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8304ccf88e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7a979712fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbdd50c96b85
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6946bd743b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1237f11edcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c74b1f68590
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f086f95b305
https://hg.mozilla.org/integration/mozilla-inbound/rev/b971de7edfff
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed9795fa80e
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d6617835d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f86d2d4cfadf

Comment 84

[Ryan VanderMeulen [:RyanVM]]

Backed out for SM rootanalysis orange :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/94cdaced90bf

https://tbpl.mozilla.org/php/getParsedLog.php?id=31574619&tree=Mozilla-Inbound

Comment 85

[Andy Wingo [:wingo]]

Bug 944930 entirely invalidated this patchset.  I have been rebasing most of yesterday, have had significant conflicts, and am now stuck in some baseline compiler runtime error.  Not fun.  I really would have appreciated better communication.

Comment 86

[Andy Wingo [:wingo]]

This is a classic case where we could have had both patches in yesterday, but instead we have only the small one in, and no ETA.

Comment 87

[Andy Wingo [:wingo]]

Attachment: Part 1: Expose JSScript::getBlockScope(jsbytecode *).

Comment 88

[Andy Wingo [:wingo]]

Attachment: Part 2: Remove HAS_BLOCKCHAIN.

Comment 89

[Andy Wingo [:wingo]]

Attachment: Part 3: Add DEBUGLEAVEBLOCK opcode to invalidate live DebugScopes.

Comment 90

[Andy Wingo [:wingo]]

Attachment: Part 4: Generators allocate all locals on the scope chain.

Comment 91

[Andy Wingo [:wingo]]

Attachment: Part 5: Clean up bytecode generation for catch clauses.

Comment 92

[Andy Wingo [:wingo]]

Attachment: Part 6: Fix for-let nesting so nonlocal exits are easier.

Comment 93

[Andy Wingo [:wingo]]

Attachment: Part 7: Refactor entering and leaving block scopes in BytecodeEmitter.

Comment 94

[Andy Wingo [:wingo]]

Attachment: Part 8: Record block scope ranges more precisely.

Comment 95

[Andy Wingo [:wingo]]

Attachment: Part 9: Fix js_DumpScriptDepth to not abort on unreachable bytecodes.

Comment 96

[Andy Wingo [:wingo]]

Attachment: Part 10: Iterate block chain from compile-time block scope maps, not runtime blockChain.

Comment 97

[Andy Wingo [:wingo]]

Attachment: Part 11: Optimize block scopes without aliased locals.

Comment 98

[Andy Wingo [:wingo]]

Attachment: Part 12: Remove blockChain from StackFrame.

Comment 99

[Andy Wingo [:wingo]]

Updated patches to adapt to changes introduced in bug 944930.  Reworded commit logs while I was at it so that RyanVM wouldn't have to do it next time.

I have tried to produce the rooting analysis orange but could not do so locally.  Am trying again at https://tbpl.mozilla.org/?tree=Try&rev=122fcfb69333.  Other tests at https://tbpl.mozilla.org/?tree=Try&rev=1619a6dfc148.

Comment 100

[Andy Wingo [:wingo]]

Seems it wasn't the br-haz build that I needed: https://tbpl.mozilla.org/?tree=Try&rev=7e5383573ae0
but just a linux64 debug build (?) https://tbpl.mozilla.org/?tree=Try&rev=53d52a691298
We will see I guess.

Comment 101

[Andy Wingo [:wingo]]

Turns out if you "try" multiple configurations separately, it might not realize that you touched js/src, and therefore doesn't run the rooting foo.  https://tbpl.mozilla.org/?tree=Try&rev=dc7c6f4347dd should run all the right things.

Comment 102

[Andy Wingo [:wingo]]

Attachment: Part 4: Generators allocate all locals on the scope chain.

Comment 103

[Andy Wingo [:wingo]]

Attachment: Part 10: Iterate block chain from compile-time block scope maps, not runtime blockChain.

Comment 104

[Andy Wingo [:wingo]]

New try build here, with fix: https://tbpl.mozilla.org/?tree=Try&rev=1315afb0a9c8
Update in Part 4 was mechanical.  Setting r? on part 10 to jonco, for the GGC-related fix.

Comment 105

[Andy Wingo [:wingo]]

Comment on attachment 8346607 [details] [diff] [review]
Part 10: Iterate block chain from compile-time block scope maps, not runtime blockChain.

Unhappily, interdiff doesn't seem to work.  The relevant bit is in vm/ScopeObject.{h,cpp}, specifically the part that deals in ScopeIterVal.

I tried to unify the scope iters -- there are three of them now (ugh).  There is ScopeIter itself, which has Rooted<> pointers for stack use; ScopeIterKey which has raw pointers, for use in hash keys, and how ScopeIterVal with RelocatablePtr<>.  Surely there is a way to unify them with templates.  I tried for a couple hours (!) and didn't get anywhere.  I'm happy to land a cleanup later though.

Comment 106

[Jon Coppeard (:jonco)]

Comment on attachment 8346607 [details] [diff] [review]
Part 10: Iterate block chain from compile-time block scope maps, not runtime blockChain.

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

r=me for the GGC postbarrier changes.

::: js/src/vm/ScopeObject.h
@@ +601,5 @@
>      static HashNumber hash(ScopeIterKey si);
>      static bool match(ScopeIterKey si1, ScopeIterKey si2);
>  };
>  
> +class ScopeIterVal

It's worth adding a comment that the content ScopeIterKey and ScopeIterVal should match apart from the barriers and also adding a static assert that they're the same size.

Comment 107

[Terrence Cole [:terrence]]

(In reply to Andy Wingo [:wingo] from comment #105)
> Comment on attachment 8346607 [details] [diff] [review]
> Part 10: Iterate block chain from compile-time block scope maps, not runtime
> blockChain.
> 
> Unhappily, interdiff doesn't seem to work.  The relevant bit is in
> vm/ScopeObject.{h,cpp}, specifically the part that deals in ScopeIterVal.
> 
> I tried to unify the scope iters -- there are three of them now (ugh). 
> There is ScopeIter itself, which has Rooted<> pointers for stack use;
> ScopeIterKey which has raw pointers, for use in hash keys, and how
> ScopeIterVal with RelocatablePtr<>.  Surely there is a way to unify them
> with templates.  I tried for a couple hours (!) and didn't get anywhere. 
> I'm happy to land a cleanup later though.

I've burned whole weeks on this, so don't feel too bad. :-)

The thing that's really needed for a reasonable template approach here is the ability to automatically specialize on the storage class: stack, gc heap, malloc heap, reallocable malloc heap, etc. Of course, C/C++ doesn't make this distinction -- either statically or even at runtime -- so this isn't really possible. We basically made the explicit decision to give up this fight. Fortunately it only bites us in a handful of places; I think it's limited to {ReadOnly}CompileOptions, StackIter{Key,Val}, {Stack}BaseShape at the moment.

"Giving up" isn't really the most elegant of solutions, particularly when it should be straightforward c.f. Haskell or Rust; but on the scale of C++ atrocities it's really not even worth worrying about.

Comment 108

[Andy Wingo [:wingo]]

Attachment: Part 4: Generators allocate all locals on the scope chain.

Comment 109

[Andy Wingo [:wingo]]

Attachment: Part 8: Record block scope ranges more precisely.

Comment 110

[Andy Wingo [:wingo]]

Attachment: Part 11: Optimize block scopes without aliased locals.

Comment 111

[Andy Wingo [:wingo]]

Praise be: a green SM(r), SM(ggc) build: https://tbpl.mozilla.org/?tree=Try&rev=709ec175a663

The recently uploaded patch changes are just to build, except for attachment 8347250 [details] [diff] [review], which moves the recorded PC starting a block scope note after the PUSHBLOCKSCOPE, if PUSHBLOCKSCOPE is emitted -- fixing an orange found by the previous try build when the block scope computed from the PC and block note list was not consistent with the scope chain.

Waiting on the rest of the try build to go green before setting checkin-needed.

Comment 112

[Andy Wingo [:wingo]]

Attachment: Part 10: Iterate block chain from compile-time block scope maps, not runtime blockChain.

Comment 113

[Andy Wingo [:wingo]]

Update adds static asserts suggested in comment 106.

Comment 114

[Andy Wingo [:wingo]]

Green.  Let's hit it!

Comment 115

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a3fec21994
https://hg.mozilla.org/integration/mozilla-inbound/rev/963a687c1cca
https://hg.mozilla.org/integration/mozilla-inbound/rev/98190772bfeb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c007db70cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1571ba544f39
https://hg.mozilla.org/integration/mozilla-inbound/rev/d79e6f67ac75
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1681c11fc2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/58590571c4e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/66b62aae3815
https://hg.mozilla.org/integration/mozilla-inbound/rev/2446b9abbe6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b854b106d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e707fd8e62d

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1a3fec21994
https://hg.mozilla.org/mozilla-central/rev/963a687c1cca
https://hg.mozilla.org/mozilla-central/rev/98190772bfeb
https://hg.mozilla.org/mozilla-central/rev/e2c007db70cf
https://hg.mozilla.org/mozilla-central/rev/1571ba544f39
https://hg.mozilla.org/mozilla-central/rev/d79e6f67ac75
https://hg.mozilla.org/mozilla-central/rev/e1681c11fc2c
https://hg.mozilla.org/mozilla-central/rev/58590571c4e0
https://hg.mozilla.org/mozilla-central/rev/66b62aae3815
https://hg.mozilla.org/mozilla-central/rev/2446b9abbe6c
https://hg.mozilla.org/mozilla-central/rev/d0b854b106d4
https://hg.mozilla.org/mozilla-central/rev/9e707fd8e62d

\m/

Comment 117

[Luke Wagner [:luke]]

\o/