659577 - Don't alias stack variables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Call objects are allocated with a slots vector, and closed variables initially live on the interpreter stack. When an activation is finished, those closed variables are copied into the call object's slots.

We should consider removing these variables off the stack and making callObj->dslots their canonical location. They wouldn't contribute to |script->nfixed| and we'd have new opcodes like GETCALLSLOT etc. For closed arguments, the act of creating a call object would copy closed arguments.

Pros:
 * Greatly simplify JIT work required to support closed variables.
 * Eliminate callObj->fp() testing.
 * Eliminate PutCallObject from the critical path - it shows up in profiles,
   like in earley-boyer.

Cons:
 * Accessing closed variables within their defining scope gets a little more
   expensive.

Comment 1

[Luke Wagner [:luke]]

Sounds great.  And hey, the obvious implementation may even end up fixing bug 577572.

Comment 4

[Luke Wagner [:luke]]

Attachment: combined patch (WIP)

WIP note: the combined set of patches (including dependent bugs) is passing shell and browser debugger tests with --disable-methodjit.  So, pretty close.

Comment 5

[Luke Wagner [:luke]]

Attachment: combined patch (WIP 2)

Last patch (finally) basically done and passing tests (still need to try-server and fill in TODO comments etc).

On early-boyer running in the shell, my machine is really noisy, but I think I see a ~10% score improvement.  Cachegrind shows a 14% icount reduction and 12% dref reduction.

Comment 6

[Luke Wagner [:luke]]

Attachment: don't use ScopeObject::maybeStackFrame part 1

This patch fixes up all the DebugScopeObject code added by bug 690135 to not depend on ScopeObject::maybeStackFrame (since it is removed by this bug).

The strategy is pretty simple: maintain a map ScopeObject* --> StackFrame* that simulates maybeStackFrame (with a memoization strategy to avoid O(n^2) stack walking).

Comment 7

[Luke Wagner [:luke]]

Attachment: don't use ScopeObject::maybeStackFrame part 2

This patch removes the indirect dependency on maybeStackFrame: since ScopeObjects will no longer alias the stack frame, we need to stop proxying all reads/writes.  This requires hoisting this logic into a new DebugScopeProxy::unaliasedAccess.  (witness the power of DebugScopeProxy!)

Another trick is that, since js_PutCallObject isn't doing it for us, we have to copy unaliased locals/args into Call/Block objects before they are popped.

Comment 8

[Luke Wagner [:luke]]

Attachment: emit ScopeCoordinate::hops

This adds the final bit to the front-end to actually emit ScopeCoordinate::hops.  This patch just asserts we got it right and still aliases the stack frame as always.

Comment 9

[Luke Wagner [:luke]]

Attachment: put the blockChain into ScopeCoordinate

This patch stores the block index in the opcode instead of the atom index.  The atom index is derivable from the block index (see ScopeCoordinateAtom), but the additional block index information allows us to avoid the hard question "what is the block index at some random pc" (normally we rely on fp->maybeBlockChain, but that only works if you are asking about fp's current pc).

Comment 10

[Luke Wagner [:luke]]

Attachment: stop aliasing StackFrame, completely breaking the mjit

This patch finally does the deed.

Comment 11

[Luke Wagner [:luke]]

Attachment: fix the mjit

This patch fixes the mjit by emitting code to access ArgumentsObject and ScopeObject.  The thought-provoking part was the change (made in the previous patch) of having a single StackFrame::prologue that is always executed by the callee (instead of the Invoke).  I think this makes things a good bit simpler and, later, dvander and I would like to see the prologue split out (again) into a JSOP_BEGIN.

Comment 12

[Luke Wagner [:luke]]

The whole stack is green on try:
https://tbpl.mozilla.org/?tree=Try&rev=5c4ac9959dfc

Comment 13

[Brian Hackett [Laid off!]]

Comment on attachment 624584 [details] [diff] [review]
stop aliasing StackFrame, completely breaking the mjit

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

Yay!

::: js/src/vm/ScopeObject.cpp
@@ +626,5 @@
>  
> +    /*
> +     * Copy in the closed-over locals. Closed-over locals don't need
> +     * any fixup since the initial value is 'undefined'.
> +     */

I don't understand what this comment means.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 624581 [details] [diff] [review]
emit ScopeCoordinate::hops

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

I really haven't been following this work well enough to review this well -- I don't recognize ScopeCoordinate or any of the aliased-var code or terminology, except for a gist I've gleaned from the occasional discussion -- but whatever.

::: js/src/jsinterp.cpp
@@ +1191,5 @@
>  # define END_CASE_LEN8      len = 8; goto advance_pc;
>  # define END_CASE_LEN9      len = 9; goto advance_pc;
>  # define END_CASE_LEN10     len = 10; goto advance_pc;
> +# define END_CASE_LEN11     len = 11; goto advance_pc;
> +# define END_CASE_LEN12     len = 12; goto advance_pc;

Maybe we should round up to page size for better cache locality?

::: js/src/vm/ScopeObject-inl.h
@@ +47,5 @@
>  
>  inline
>  ScopeCoordinate::ScopeCoordinate(jsbytecode *pc)
> +  : hops(GET_UINT16(pc)), binding(GET_UINT16(pc + 2)),
> +    frameBinding(GET_UINT16(pc + 8))

pc + 2 + 2 + 4 perhaps.

@@ +130,5 @@
> +        } else {
> +            unsigned var = bindings.bindingToLocal(sc.binding);
> +            JS_ASSERT(fp->script()->varIsAliased(var));
> +            fp->localSlot(var) = v;
> +        }

Parity with aliasedVar would seem to suggest an early return here, rather than the else structure.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 624583 [details] [diff] [review]
put the blockChain into ScopeCoordinate

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

Also kinda graspy.

::: js/src/vm/ScopeObject.h
@@ +76,5 @@
> +extern StaticBlockObject *
> +ScopeCoordinateBlockChain(JSScript *script, jsbytecode *pc);
> +
> +/* Return the name being accessed by the given ALIASEDVAR op. */
> +extern JSAtom *

PropertyName

Comment 16

[Jim Blandy :jimb]

Comment on attachment 624577 [details] [diff] [review]
don't use ScopeObject::maybeStackFrame part 1

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

::: js/src/jit-test/tests/basic/eif-generator.js
@@ +1,2 @@
>  // |jit-test| debug
> +// |jit-test| debug

This looks like a cut-and-paste error?

::: js/src/vm/ScopeObject.cpp
@@ +1672,5 @@
> +DebugScopes::updateLiveScopes(JSContext *cx)
> +{
> +    JS_CHECK_RECURSION(cx, return false);
> +
> +    for (AllFramesIter i(cx->runtime->stackSpace); !i.done(); ++i) {

Would this text be correct as a comment? It took me a bit to figure this out:

Note that we must always update the top frame's scope objects' entries in liveScopes, because we can't be sure code hasn't run in that frame to change the scope chain since we were last called. This means that each call to GetDebugScopeForFrame or GetDebugScopeForFunction will run the inner loop below, over this frame's virtual scope objects.

The fp->prevUpToDate() flag indicates whether the frames older than fp are already accurately described by liveScopes. It might seem simpler to have fp instead carry a flag indicating whether fp itself is accurately described, but then we would need to clear that flag whenever fp ran code. By storing the 'up to date' bit for fp->prev() in fp, simply popping fp effectively clears the flag for us, at exactly the time when execution resumes in the older frame.

@@ +1682,5 @@
> +            if (si.hasScopeObject() && !liveScopes.put(&si.scope(), fp))
> +                return false;
> +        }
> +
> +        /* liveScopes will accurately reflect fp->prev() until fp is popped. */

I was confused by this comment. I think it should say:

/* liveScopes will accurately reflect all frames older than fp until fp is popped. */

::: js/src/vm/ScopeObject.h
@@ +465,5 @@
> +     * two lazy updates, liveScopes becomes incomplete (but not invalid, onPop*
> +     * removes scopes as they are popped). Thus, two consecutive debugger lazy
> +     * updates of liveScopes need only fill in the new scopes.
> +     */
> +    typedef HashMap<ScopeObject *, StackFrame *, DefaultHasher<ScopeObject *>, RuntimeAllocPolicy> LiveScopeMap;

nit: Source code should fit in 100 columns.

::: js/src/vm/Stack.cpp
@@ +292,5 @@
>  void
>  StackFrame::popWith(JSContext *cx)
>  {
> +    if (cx->compartment->debugMode())
> +        cx->runtime->debugScopes->onPopWith(this);

It's possible to leave debug mode with frames on the stack; if you add an assertion to JSCompartment::updateForDebugMode, then you can see that jit-test/test/debug/Debugger-debuggees-{15,16}.js cause this to happen.

If that occurs, then it seems like this code will leave referencse to dead frames in liveScopes.

The following extant code has the same issue (all r=me, sigh):
- StackFrame::functionEpilogue calling onPopCall
- StackFrame::popBlock calling onPopBlock
- js::ExecuteKernel calling onPopStrictEvalScope (in this patch)
- StackFrame::stealFrameAndSlots calling onGeneratorFrameChange(maybe not?)

Perhaps it would be sufficient to, when the compartment leaves debug mode, pre-emptively remove all liveScopes entries for objects in that compartment.

Comment 17

[Jim Blandy :jimb]

Is this an accurate diagram of what we're aiming for (once aliased variables no longer ever live on the stack)?

https://github.com/jimblandy/DebuggerDocs/blob/master/debugger-scopes.png

Comment 18

[Jim Blandy :jimb]

Better link:
https://github.com/jimblandy/DebuggerDocs/raw/master/debugger-scopes.png

Comment 19

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #16)
> Perhaps it would be sufficient to, when the compartment leaves debug mode,
> pre-emptively remove all liveScopes entries for objects in that compartment.

This is what onCompartmentLeaveDebugMode already does in the patch :)  While I get to the nits, perhaps you'd like to reconsider the r-?

(In reply to Jim Blandy :jimb from comment #17)
> Is this an accurate diagram of what we're aiming for (once aliased variables
> no longer ever live on the stack)?

Looks good to me.  The only interesting addition is that, when the stack frame is popped, 'scope' holds *all* the slots, closed or not.  (I think that's in part 2, though.)

Comment 20

[Jim Blandy :jimb]

Comment on attachment 624577 [details] [diff] [review]
don't use ScopeObject::maybeStackFrame part 1

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

D'oh! :( I see it now.

I'm always willing to reconsider an r-. :)

::: js/src/vm/ScopeObject.cpp
@@ +1497,5 @@
>          if (IsAboutToBeFinalized(e.front().value))
>              e.removeFront();
>      }
> +
> +    /* Scopes can be finalized when a suspended generator becomes garbage. */

I know I'm supposed to know how generators work, but it would still be nice to have a little more of a comment here.

In the usual case, we remove scope objects from liveScopes as soon as we leave their dynamic scope; and scopeChain_ itself holds them live until that time. But in the generator case, we do not pop BlockObjects and WithObjects when we yield from within them --- so they may get collected before they're popped. Right?

@@ +1573,5 @@
>  {
>      if (fp->isYielding())
>          return;
>  
> +    if (fp->fun()->isHeavyweight()) {

So there's a symmetry here: reified environment ribs need to have entries removed from liveScopes; whereas non-reified environment ribs may need to have entries for fake scope chain objects we created for the debugger. Both BlockObjects and WithObjects get the same logic.

Comment 21

[Jim Blandy :jimb]

Comment on attachment 624580 [details] [diff] [review]
don't use ScopeObject::maybeStackFrame part 2

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

Wow, looks great.

::: js/src/jit-test/tests/debug/Frame-eval-13.js
@@ +7,5 @@
> +g.eval("var surprise = null");
> +
> +var dbg = new Debugger(g);
> +dbg.onDebuggerStatement = function(hFrame) {
> +    hit = true;

This variable isn't used.

::: js/src/vm/ScopeObject.cpp
@@ +1312,5 @@
> +     *
> +     * unaliasedAccess returns 'true' if the access was unaliased and completed
> +     * by unaliasedAccess.
> +     */
> +    bool unaliasedAccess(JSContext *cx, ScopeObject &scope, jsid id, Action action, Value *vp)

I like this a lot. This is sort of the kind of thing I've been expecting DebugObjectScope to do from the start.

nit: I'd prefer the name be verby, like handleUnaliasedAccess, or accessUnaliased. At first I misread its uses below as a call to a predicate, not the thing which *did* the access. But only fix if you agree.

@@ +1324,5 @@
> +        if (scope.isCall() && !scope.asCall().isForEval()) {
> +            CallObject &callobj = scope.asCall();
> +            JSScript *script = callobj.getCalleeFunction()->script();
> +            if (!script->ensureHasTypes(cx))
> +                return false;

This is an error return, right? Is it a problem that this function uses 'false' to mean both 'this references is not my business' and 'an error occurred'? (If it's guaranteed that ensureHasTypes only throws OOM errors, then this could be okay, but it's a little strange.)

@@ +1328,5 @@
> +                return false;
> +
> +            if (shape->setterOp() == CallObject::setVarOp) {
> +                unsigned i = shape->shortid();
> +                if (!script->varIsAliased(i)) {

nit: it might be clearer to just say:

if (script->valIsAliased(i))
  return false;

and leave the rest unindented. Similarly for the arg and block cases.

@@ +1344,5 @@
> +                    if (action == SET)
> +                        TypeScript::SetLocal(cx, script, i, *vp);
> +                    return true;
> +                }
> +            } else if (shape->setterOp() == CallObject::setArgOp) {

nit: Brendan liked to discourage 'return after else' in SM. I think he'd either put the 'return false' in the final 'else', and make the final return in the function return true, or just have each clause return and remove the 'else's.

@@ +1799,5 @@
>                  liveScopes.add(livePtr, &toIter.scope(), to);
>          } else {
>              if (MissingScopeMap::Ptr p = missingScopes.lookup(ScopeIter(toIter, from))) {
>                  DebugScopeObject &debugScope = *p->value;
> +                liveScopes.lookup(&debugScope.scope())->value = to;

The reason we no longer need to change the frames here is that the ScopeObjects synthesized for the debugger no longer use their stack frame pointers; and in fact, they are set to NULL in GetDebugScopeForMissing. Is that right?

Comment 22

[Paul Wright]

Is this going to land for fx15?

Comment 23

[Luke Wagner [:luke]]

(In reply to Jim Blandy :jimb from comment #16)
Thanks for all the great comments!  Sorry for taking so long to respond after the review; I've been unexpectedly swamped.

> Would this text be correct as a comment? It took me a bit to figure this out:

Exactly.

(In reply to Jim Blandy :jimb from comment #20)
> But in the generator case, we do not pop BlockObjects and WithObjects
> when we yield from within them --- so they may get collected before they're
> popped. Right?

Right

(In reply to Jim Blandy :jimb from comment #21)
> I like this a lot. This is sort of the kind of thing I've been expecting
> DebugObjectScope to do from the start.

It was the plan from the start, just had to build up to it :)

> (If it's guaranteed that ensureHasTypes only throws OOM errors,
> then this could be okay, but it's a little strange.)

That's the case.  It is weird, but it seemed preferable to adding a whole new case (which grosses up callers) for this extreme corner case.

> The reason we no longer need to change the frames here is that the
> ScopeObjects synthesized for the debugger no longer use their stack frame
> pointers; and in fact, they are set to NULL in GetDebugScopeForMissing. Is
> that right?

Correct

Comment 24

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7636c7036e2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d64eea8385
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee37d2b88eeb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc63b39ab57
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee940e4debd0

Comment 25

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/727f3e801afb

https://tbpl.mozilla.org/php/getParsedLog.php?id=12262384&tree=Mozilla-Inbound (and the rest of the debugs)
TEST-UNEXPECTED-FAIL | jit_test.py -a -m -d | /builds/slave/m-in-lnx64-dbg/build/js/src/jit-test/tests/jaeger/testBug550743.js: Assertion failure: gen->state == JSGEN_OPEN, at ../../../js/src/vm/ScopeObject.cpp:1476

Comment 26

[Luke Wagner [:luke]]

That's what I get for tweaking after try'ing.

Comment 27

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies to cset 123be6501757)

On second thought, I would really appreciate a dose of fuzzing.

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/7636c7036e2e
https://hg.mozilla.org/mozilla-central/rev/35d64eea8385
https://hg.mozilla.org/mozilla-central/rev/ee37d2b88eeb
https://hg.mozilla.org/mozilla-central/rev/ddc63b39ab57
https://hg.mozilla.org/mozilla-central/rev/ee940e4debd0

Comment 29

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/727f3e801afb

Comment 30

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Luke Wagner [:luke] from comment #27)
> Created attachment 629401 [details] [diff] [review]
> combined patch for fuzzing (applies to cset 123be6501757)
> 
> On second thought, I would really appreciate a dose of fuzzing.

Patched on top of m-c revision 68abc86fde1c:

(function({
    l
}) {
    eval()
})()

Assertion failure: JSID_IS_STRING(iden),

===

(with -d:)

try {
    function f() {}
    (1 for (x in []))
} catch (e) {}
gc()

Assertion failure: gen->state == JSGEN_OPEN,

Comment 31

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies to cset 123be6501757) (v.2)

Ah, 1 overly-strict assert (need to add || state == JSGEN_NEWBORN) and 1 decompile-value-generator + destructuring formals.  Let's try one more time.

Comment 32

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 629550 [details] [diff] [review]
combined patch for fuzzing (applies to cset 123be6501757) (v.2)

Almost there, not there yet:

Function("for(;(function(){([x],0)});x){}var x")

Assertion failure: bce->sc->funIsHeavyweight(),

(tested on 64-bit debug js shell with patch on top of m-c changeset d0ebcaa7efb5)

Comment 33

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 629550 [details] [diff] [review]
combined patch for fuzzing (applies to cset 123be6501757) (v.2)

To be fair, other than the:

Assertion failure: bce->sc->funIsHeavyweight(),

in comment 32, at this point in time I don't think jsfunfuzz has found anything more, so turning to feedback+ pending this assertion fix.

Comment 34

[Christian Holler (:decoder)]

Comment on attachment 629550 [details] [diff] [review]
combined patch for fuzzing (applies to cset 123be6501757) (v.2)

I got three issues here that only occur with this patch:


First (64 bit dbg, options -m -n -a): Assertion failure: false, at methodjit/InvokeHelpers.cpp:915

gczeal(2);
function C(i) { 
  this.m *=  function () { return "'no'\u001D+' error'" <<  this & dbg[i]; }
}
var a = [];
for (var i = 0; i < 5; ((function  () { return 42; } )))
    a[a.length] = new C(i);



Second (64 bit dbg, options -m -n -a): Assertion failure: [barrier verifier] Unmarked edge: <unknown>, at jsgc.cpp:4443

gczeal(4);
evaluate("\
Date.formatFunctions = {count:0};\
Date.prototype.dateFormat = function(format) {\
    var funcName = 'format' + Date.formatFunctions.count++;\
    var code = 'Date.prototype.' + funcName + ' = function(){return ';\
    var ch = '';\
    for (var i = 0; i < format.length; ++i) {\
        ch = format.charAt(i);\
        eval(code.substring(0, code.length - 3) + ';}');\
    }\
};\
var date = new Date('1/1/2007 1:11:11');\
    var shortFormat = date.dateFormat('Y-m-d');\
");



Third (64 bit dbg, options -m -n -a): Assertion failure: (ptrBits & 0x7) == 0, at ../../jsval.h:702

function TestCase(n, d, e, a) {}
function enterFunc (funcName)
function test() {}
try {
test('c{3,4}');
} catch(exc1) {}
function test() {
  enterFunc ('test');
  try  { 
    eval('for each (this in []) { }');
  } catch ( [ [r, g, b]  , l   , v  , e       ]   ) { 
  }  
}
new TestCase( "15.5.2", "String()", "", String() );
test();

Comment 35

[Luke Wagner [:luke]]

Thanks guys!  2 overzealous asserts, 1 thinko in the mjit throw path, and missing write barriers for the SETALIASEDVAR jit paths (oh hai incremental gc).

Comment 36

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d657e8cb0201
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff46668b156
https://hg.mozilla.org/integration/mozilla-inbound/rev/4832054e4e42
https://hg.mozilla.org/integration/mozilla-inbound/rev/75e9c37626e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b863ef9946b8

Comment 37

[Luke Wagner [:luke]]

Talking to Bill just now, we realized that the weak pointers in DebugScopes need read barriers for incremental GC:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0b1390710f

Comment 38

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/mozilla-central/rev/d657e8cb0201
https://hg.mozilla.org/mozilla-central/rev/2ff46668b156
https://hg.mozilla.org/mozilla-central/rev/4832054e4e42
https://hg.mozilla.org/mozilla-central/rev/75e9c37626e8
https://hg.mozilla.org/mozilla-central/rev/b863ef9946b8
https://hg.mozilla.org/mozilla-central/rev/fe0b1390710f

Comment 39

[David Anderson [:dvander] - inactive, e-mail if emergency]

Yay!!!!