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.

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.

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

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.

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

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.

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

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 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 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 on attachment 8335298 [details] [diff] [review] Fix for-let nesting so nonlocal exits are easier. Nice

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

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

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

(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 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 on attachment 8335345 [details] [diff] [review] Remove blockChain from StackFrame Ah, my favorite review.

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

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

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.

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

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.

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

(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 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)?

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.

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

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

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 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 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 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!

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

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.

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

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

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

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

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.

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.

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.

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.

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.

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

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

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.

Update adds static asserts suggested in comment 106.

Green. Let's hit it!

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

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/

\o/