469927 - TM: much slower than interpreter with short loop with |let|

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

I'm teaching the fuzzer in bug 465479 to compare performance as well as output.  It found this testcase that is 6X slower with -j than without -j.

$ cat c.js
for (let i = 0; i < 500000; ++i) { for (let j = 0; j < 4; ++j) { } }

$ time ~/tracemonkey/js/src/debug/js c.js

real	0m0.321s
user	0m0.313s
sys	0m0.008s

$ time ~/tracemonkey/js/src/debug/js -j c.js
recorder: started(14), aborted(13), completed(1), different header(0), trees trashed(0), slot promoted(0), unstable loop variable(0), breaks(0), returns(0), unstableInnerCalls(0)
monitor: triggered(500000), exits(500000), type mismatch(0), global mismatch(1)

real	0m1.969s
user	0m1.949s
sys	0m0.014s

Comment 1

[Brendan Eich [:brendan]]

Aborting on JSOP_ENTERBLOCK, fixable for sure -- but why aren't we blacklisting and not slowing down?

bz, didn't you file a bug just recently on JSOP_ENTERBLOCK aborting?

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: trace {ENTER,LEAVE}BLOCK* ops, they're easy

But the lack of blacklisting needs diagnosis and possibly a separate patch. David, Andreas: can you help?

/be

Comment 3

[Boris Zbarsky [:bzbarsky]]

I hadn't gotten around to it yet.  ENTERBLOCK is mentioned in bug 452499, but
that's it.  This can probably be the bug about it.

Comment 4

[Brendan Eich [:brendan]]

Add-ons and XUL apps including Firefox use let in loops. Downside is bad for short loops. Fix is easy and safe, so I'll look for wanted1.9.1+ and (once it is reviewed) patch approval.

/be

Comment 5

[Jason Orendorff [:jorendorff]]

In record_JSOP_ANONFUNOBJ, we have:

    if (OBJ_GET_PARENT(cx, obj) != cx->fp->scopeChain)
        ABORT_TRACE("can't trace with activation object on scopeChain");

Note that the interpreter doesn't call js_GetScopeChain until after that.  This may have been OK before, if sloppy, as there were never any let blocks to clone. But this patch changes that, right?

Re bug 462027, I think a call to js_GetScopeChain on trace would have to deep-bail.  But it may already be impossible to call it on trace.  If so, I could probably prove this by making it JS_REQUIRES_STACK.

Comment 6

[Jason Orendorff [:jorendorff]]

In addition to fixing record_JSOP_ANONFUNOBJ, I think we have to refuse to execute a tree if (cx->fp->flags & JSFRAME_POP_BLOCKS).  Not sure though.

Comment 7

[Brendan Eich [:brendan]]

Fixed in tm:

http://hg.mozilla.org/tracemonkey/rev/a27bde3b8719

(In reply to comment #5)
> In record_JSOP_ANONFUNOBJ, we have:
> 
>     if (OBJ_GET_PARENT(cx, obj) != cx->fp->scopeChain)
>         ABORT_TRACE("can't trace with activation object on scopeChain");
> 
> Note that the interpreter doesn't call js_GetScopeChain until after that.  This
> may have been OK before, if sloppy, as there were never any let blocks to
> clone. But this patch changes that, right?

Good catch -- that JSOP_ANONFUNOBJ test was a trap for the unwary, to wit: me. I will patch again in this bug.

> Re bug 462027, I think a call to js_GetScopeChain on trace would have to
> deep-bail.  But it may already be impossible to call it on trace.  If so, I
> could probably prove this by making it JS_REQUIRES_STACK.

Is the problem unreserved block objects? That seems fixable.

(In reply to comment #6)
> In addition to fixing record_JSOP_ANONFUNOBJ, I think we have to refuse to
> execute a tree if (cx->fp->flags & JSFRAME_POP_BLOCKS).  Not sure though.

We could jit more code from record_JSOP_LEAVEBLOCK* to handle that case.

/be

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to comment #7)
> Is the problem unreserved block objects? That seems fixable.

Oh - I didn't think about that.  For now, we shouldn't have to worry.  When we leave a tree, we don't have to clone block objects.  They'll be cloned later, by the interpreter or whatever, if needed.  If we do start supporting anonfunobj or eval in blocks, it will have to be fixed.

I just meant that if js_GetScopeChain is called, we can't continue on trace, because (a) it sets JSFRAME_POP_BLOCKS, which the patch currently assumes isn't set; (b) the new block objects wouldn't behave correctly.  (For example, JITted JSOP_SETLOCAL instructions don't actually update the stack frame's fp->slots.)

Comment 9

[Brendan Eich [:brendan]]

I think we're ok. JSOP_ANONFUNOBJ is recorded only if in a loop, so it ran already and called js_GetScopeChain. So cx->fp->scopeChain will included cloned block objects, and cx->fp->blockChain will be null.

What's more: the compiler-created function object, obj in TraceRecorder::record_JSOP_ANONFUNOBJ, will not have a parent equal to the current scope chain.

In global code that's compile-and-go (no separate JS_Compile* API usage that might be followed by several JS_Execute* calls passing different scope objects), the compiler will make the anonymous function object's parent be the global. But since we trace at HOTLOOP=2 we must have called js_GetScopeChain from the interpreter for this frame already, so cx->fp->scopeChain will have a cloned block object at its head and that object reference will not equal the global object reference in obj->fslots[JSSLOT_PARENT].

If we inlined from a hot loop into a function containing a let-block scope around a lambda, then we indeed may have no cloned block for the let scope at cx->fp->scopeChain. But under this assumption, the compiler can't preset the function object's parent to any known scope (it can't compute the call objects at runtime for each call to the outer function -- the function we inlined our way into from the hot loop).

So here too, the

7657	    if (OBJ_GET_PARENT(cx, obj) != cx->fp->scopeChain)

condition guarding an ABORT_TRACE will save us.

Jason, please double-check this argument. It reduces to basis case analysis. No induction required.

/be

Comment 10

[Brendan Eich [:brendan]]

I did test in gdb, both lambda-in-function and lambda-in-global-code.

Does bug 462027 cover js_GetScopeChain bailing if called on-trace? I should go review that patch!

/be

Comment 11

[Brendan Eich [:brendan]]

Fixed in m-c:

http://hg.mozilla.org/mozilla-central/rev/c0d189525474

/be

Comment 12

[Ed Lee :Mardak]

Could this have caused these crashes (~100 reports ~4 hours after nightly) that started with today:

http://crash-stats.mozilla.com/report/list?version=Firefox%3A3.2a1pre&signature=js_NewObjectWithGivenProto

Location bar uses plenty of |let|s.

Setting jit.chrome to false avoids the crashes.

Comment 13

[Brendan Eich [:brendan]]

Mardak, you on IRC? I pinged you.

This bug is potentially involved. What else in js/src was touched yesterday on mozilla-central? But it's hard to see. The stacks all end in array_slice calling js_NewArrayObject calling js_NewObjectWithGivenProto, which dies on a mostly nearly null address (but sometimes not nearly null at all) around where clasp is dereferenced. Yikes. Hope we don't have a store through a wild pointer.

We should be able to extract a testcase based on the stacks. Can you try? I do not know the autocomplete code as well as you, but the only slice call is in _getBoundaryIndices, which might be called indirectly from the popupOpen setter as indicated in

http://crash-stats.mozilla.com/report/index/529a9ca5-df13-452f-8f96-203d32081218

among others.

Is there a new bug on file for those crash reports? Let's move discussion there until we pin blame better here (in which case I'll back out and reopen -- I may do that anyway to test -- by tonight if there's no better plan).

/be

Comment 14

[Benjamin Smedberg]

bug 470223 is the crasher bug, and has reduced testcases

Comment 15

[Ed Lee :Mardak]

Backing out changeset c0d189525474 from mozilla-central locally doesn't lead to asserts when running the script in bug 470223 comment 4.

However, even with the changeset applied, if I run this in js shell, nothing asserts.

function F(A) {
  for (R = [], s = 0; (m = A.indexOf("m", s++)) >= 0; )
    R.push([m]);
  for (i = R.length; i--; ) {
    r = R[i]; // |let r = R[i];| asserts
    if (r[0]);
  }
}
F("m"); F("mm");

So it seems less likely that this patch uncovered a latent bug because it only fails when |let| is there.

Comment 16

[Brendan Eich [:brendan]]

This bug's patch was incomplete. Fixing in bug 470300.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

Do we have a followup bug on the blacklisting not working?  That seems like it could help various other TM performance regression bugs too...

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2d1147e15b12

Comment 19

[Mike Shaver (:shaver emeritus)]

(In reply to comment #9)
> I think we're ok. JSOP_ANONFUNOBJ is recorded only if in a loop, so it ran
> already and called js_GetScopeChain.

I've reasoned like this many times myself, but I now think it's incorrect:

for (var i = 0; i < 100; i++) {
  if (i > HOTLOOP)
    doSomething();
}

doSomething will be traced, but has never been called by the interpreter, right?

Comment 20

[Andreas Gal :gal]

That would be executed by the interpreter at least once (when tracing it).

Comment 21

[Mike Shaver (:shaver emeritus)]

Ahem, quite.

Comment 22

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0febaef8af06

Comment 23

[Bob Clary [:bc] (inactive)]

Checking in js1_8_1/trace/regress-469927.js
http://hg.mozilla.org/mozilla-central/rev/e281e5f0266e

Comment 24

[Bob Clary [:bc] (inactive)]

modify test to prevent random failures on x64
http://hg.mozilla.org/mozilla-central/rev/4a0117ec34af

Comment 25

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2 on 32bit.