Remove blockChain from StackFrame, 2013 edition

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: wingo, Assigned: wingo)

Tracking

unspecified
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 44 obsolete attachments)

8.00 KB, patch
Details | Diff | Splinter Review
9.40 KB, patch
Details | Diff | Splinter Review
12.84 KB, patch
Details | Diff | Splinter Review
16.83 KB, patch
Details | Diff | Splinter Review
5.21 KB, patch
Details | Diff | Splinter Review
25.37 KB, patch
Details | Diff | Splinter Review
1006 bytes, patch
Details | Diff | Splinter Review
11.68 KB, patch
Details | Diff | Splinter Review
29.34 KB, patch
Details | Diff | Splinter Review
16.29 KB, patch
Details | Diff | Splinter Review
55.86 KB, patch
Details | Diff | Splinter Review
72.26 KB, patch
Details | Diff | Splinter Review
Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
Posted patch Remove HAS_BLOCKCHAIN flag (obsolete) — Splinter Review
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.
Assignee

Updated

6 years ago
Attachment #818337 - Attachment description: blockchain → Remove HAS_BLOCKCHAIN flag
Attachment #818337 - Attachment is patch: true
(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.
Assignee

Comment 3

6 years ago
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 :)
Assignee

Comment 5

6 years ago
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.
Assignee

Comment 6

6 years ago
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.
Assignee

Updated

6 years ago
Assignee: nobody → wingo
Assignee

Comment 8

6 years ago
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!)
Attachment #818337 - Attachment is obsolete: true
Assignee

Comment 9

6 years ago
Posted patch Remove HAS_BLOCKCHAIN (obsolete) — Splinter Review
Assignee

Comment 19

6 years ago
Assignee

Updated

6 years ago
Attachment #8335287 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335288 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335292 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335294 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335296 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335298 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335306 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335312 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335314 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335318 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335336 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335345 - Flags: review?(luke)
Assignee

Comment 21

6 years ago
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

Updated

6 years ago
Attachment #8335287 - Flags: review?(luke) → review+

Updated

6 years ago
Attachment #8335288 - Flags: review?(luke) → review+
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).
Attachment #8335294 - Flags: review?(luke) → review+

Updated

6 years ago
Attachment #8335292 - Flags: review?(luke) → review+
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.
Attachment #8335296 - Flags: review?(luke) → review?(jorendorff)
Comment on attachment 8335298 [details] [diff] [review]
Fix for-let nesting so nonlocal exits are easier.

Nice
Attachment #8335298 - Flags: review?(luke) → review+
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?
Attachment #8335306 - Flags: review?(luke) → review+
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()?

Updated

6 years ago
Attachment #8335314 - Flags: review?(luke) → review+
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?
Assignee

Comment 28

6 years ago
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.
Assignee

Comment 29

6 years ago
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.
Assignee

Comment 30

6 years ago
(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
Attachment #8335336 - Flags: review?(luke) → review+
Comment on attachment 8335345 [details] [diff] [review]
Remove blockChain from StackFrame

Ah, my favorite review.
Attachment #8335345 - Flags: review?(luke) → review+
Assignee

Comment 35

6 years ago
(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.
Assignee

Updated

6 years ago
Attachment #8335294 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335287 - Attachment is obsolete: true
Assignee

Comment 38

6 years ago
Posted patch 2/12: Remove HAS_BLOCKCHAIN (obsolete) — Splinter Review
Assignee

Updated

6 years ago
Attachment #8335288 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335292 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8337666 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8337668 - Flags: review+
Assignee

Updated

6 years ago
Attachment #8337669 - Flags: review+
Assignee

Updated

6 years ago
Attachment #8337671 - Flags: review+
Assignee

Comment 41

6 years ago
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.
Attachment #8337672 - Flags: review+
Assignee

Updated

6 years ago
Blocks: 942804
Assignee

Comment 42

6 years ago
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.
Assignee

Comment 44

6 years ago
(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.
Attachment #8335296 - Flags: review?(jorendorff) → review+
Assignee

Comment 46

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #8335296 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335298 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335306 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335312 - Attachment is obsolete: true
Attachment #8335312 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8338364 - Flags: review+
Assignee

Updated

6 years ago
Attachment #8338365 - Flags: review+
Assignee

Updated

6 years ago
Attachment #8338372 - Flags: review+
Assignee

Updated

6 years ago
Attachment #8338374 - Flags: review?(luke)
Assignee

Comment 51

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

Updated

6 years ago
Attachment #8335314 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335318 - Attachment is obsolete: true
Attachment #8335318 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8338411 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8335336 - Attachment is obsolete: true
Assignee

Comment 56

6 years ago
(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.
Assignee

Updated

6 years ago
Attachment #8338410 - Flags: review+
Assignee

Updated

6 years ago
Attachment #8338415 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8338418 - Flags: review?(luke)
Assignee

Updated

6 years ago
Attachment #8335345 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338421 - Flags: review+
Assignee

Updated

6 years ago
Depends on: 943409

Updated

6 years ago
Attachment #8338418 - Flags: review?(luke) → review+
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)?
Attachment #8338374 - Flags: review?(luke) → review+
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.
Assignee

Comment 60

6 years ago
(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.
Assignee

Updated

6 years ago
Attachment #8337672 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338415 - Attachment is obsolete: true
Attachment #8338415 - Flags: review?(luke)
Assignee

Comment 64

6 years ago
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.
Attachment #8341634 - Flags: review?(luke)
Assignee

Comment 65

6 years ago
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.
Attachment #8341635 - Flags: review?(luke)
Assignee

Comment 66

6 years ago
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

Updated

6 years ago
Attachment #8341634 - Flags: review?(luke) → review+
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.
Attachment #8341635 - Flags: review?(luke) → review+
Assignee

Updated

6 years ago
Depends on: 945756
Assignee

Updated

6 years ago
Depends on: 945813
Assignee

Updated

6 years ago
Depends on: 945828
Assignee

Updated

6 years ago
No longer depends on: 943409, 945756
Assignee

Updated

6 years ago
Attachment #8341634 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8341635 - Attachment is obsolete: true
Assignee

Comment 70

6 years ago
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
Attachment #8341855 - Flags: review+
Assignee

Comment 71

6 years ago
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!
Attachment #8341859 - Flags: review+
Assignee

Comment 72

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

Updated

6 years ago
Attachment #8341855 - Attachment is obsolete: true
Assignee

Comment 74

6 years ago
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.
Attachment #8342394 - Flags: review+
Some of these patches are having difficulties applying. Please rebase them :(
Keywords: checkin-needed
Assignee

Updated

6 years ago
Attachment #8337668 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8342394 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338372 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338374 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338410 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8341859 - Attachment is obsolete: true
Assignee

Comment 85

6 years ago
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.
Assignee

Comment 86

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #8343806 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8337669 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8337671 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8343819 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338364 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338365 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8343825 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8343827 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8343829 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8343833 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338418 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8338421 - Attachment is obsolete: true
Assignee

Comment 99

6 years ago
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.
Assignee

Comment 100

6 years ago
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.
Assignee

Comment 101

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #8345357 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8345365 - Attachment is obsolete: true
Assignee

Comment 104

6 years ago
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.
Assignee

Comment 105

6 years ago
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.
Attachment #8346607 - Flags: review?(jcoppeard)
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.
Attachment #8346607 - Flags: review?(jcoppeard) → review+
(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.
Assignee

Updated

6 years ago
Attachment #8346602 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8345362 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #8345366 - Attachment is obsolete: true
Assignee

Comment 111

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #8346607 - Attachment is obsolete: true
Assignee

Comment 113

6 years ago
Update adds static asserts suggested in comment 106.
Assignee

Comment 114

6 years ago
Green.  Let's hit it!
Keywords: checkin-needed
\o/
Depends on: 956173
You need to log in before you can comment on or make changes to this bug.