While loop using count-- as its condition is not optimized very well

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bz, Assigned: jandem)

Tracking

(Depends on: 1 bug)

unspecified
mozilla29
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Consider this script:

  (function(count) {
    var start = new Date;
    while (count--);
    var stop = new Date;
    print((stop - start) / 10000000 * 1e6);
  })(10000000);

This prints about 2.6ns per iteration for me in the shell.  With --ion-eager it's more like .9ns, which is what I would expect given bug 946422.

Below is what JIT inspector says we're doing.  We seem to be doing some sort of ValueToInt32 conversion (why?) and overflow checks (why?) and then boxing our int and taking a slowish-path boolean test on the Value(!).  Kannan thinks OSR+TI confusion is to blame, and the --ion-eager result above suggests he's right.

Block #3 -> #4 -> #5 :: 9998904 hits
[Label]
[InterruptCheckImplicit]
[OsiPoint]
[ValueToInt32:Normal]
movq       %rax, %r11
shrq       $47, %r11
cmpl       $0x1fff1, %r11d
je         ((690))
cmpl       $0x1fff6, %r11d
jne        ((703))
xorl       %ebx, %ebx
jmp        ((710))
##link     ((690)) jumps to ((710))
movl       %eax, %ebx
##link     ((710)) jumps to ((712))
[OsiPoint]
[SubI:OverflowCheck]
subl       $0x1, %ebx
jo         ((721))
[Box:Int32]
movabsq    $0xfff8800000000000, %rbp
orq        %rbx, %rbp
[TestVAndBranch:MightEmulateUndefined]
movq       %rax, %r11
shrq       $47, %r11
cmpl       $0x1fff2, %r11d
je         ((754))
cmpl       $0x1fff6, %r11d
je         ((767))
cmpl       $0x1fff3, %r11d
jne        ((780))
testl      %eax, %eax
je         ((788))
jmp        ((793))
##link     ((780)) jumps to ((793))
cmpl       $0x1fff1, %r11d
jne        ((806))
testl      %eax, %eax
je         ((814))
jmp        ((819))
##link     ((806)) jumps to ((819))
cmpl       $0x1fff7, %r11d
jne        ((832))
movabsq    $0x7fffffffffff, %rdi
andq       %rax, %rdi
movq       0x8(%rdi), %rsi
movq       0x0(%rsi), %rsi
movabsq    $0x10227a170, %r11
cmpq       %r11, %rsi
je         ((871))
movabsq    $0x102279fa8, %r11
cmpq       %r11, %rsi
je         ((890))
movabsq    $0x10227a348, %r11
cmpq       %r11, %rsi
je         ((909))
testl      $0x40, 0x8(%rsi)
jne        ((922))
jmp        ((927))
##link     ((832)) jumps to ((927))
cmpl       $0x1fff5, %r11d
jne        ((940))
movabsq    $0x7fffffffffff, %r11
andq       %rax, %r11
movq       0x0(%r11), %r11
shrq       $4, %r11
testq      %r11, %r11
je         ((969))
jmp        ((974))
##link     ((940)) jumps to ((974))
movq       %rax, %xmm0
xorpd      %xmm15, %xmm15
ucomisd    %xmm0, %xmm15
je         ((995))
jmp        ((1000))
 
Block #4 -> #3 :: 9998903 hits
[Label]
[MoveGroup]
movq       %rbp, %rax
[Goto]
jmp        ((1022))
##link     ((1022)) jumps to ((1022))
Depends on: 946422
Blocks: 946426
TI is falling over hard in this example.

[ValueToInt32:Normal]
movq       %rax, %r11
shrq       $47, %r11
cmpl       $0x1fff1, %r11d
je         ((690))
cmpl       $0x1fff6, %r11d
jne        ((703))
xorl       %ebx, %ebx
jmp        ((710))
##link     ((690)) jumps to ((710))
movl       %eax, %ebx
##link     ((710)) jumps to ((712))

Ion is trying to fallibly unbox an Int32 or a Null from count, which TI is not reducing down to an integer.  After the unbox, it's incrementing and re-boxing the produced integer, and then producing a massive test for if the newly boxed integer is still "true".

This is almost certainly an issue with TI messing up on OSR compilation.
(Assignee)

Comment 2

4 years ago
This looks like the usual problem where we don't have type information for the arguments, because the function is called in the interpreter. Then when we OSR into Ion, we have a phi(unknown, int32) -> MIRType_Value etc :(
I was thinking we could fix this by updating tyepsets with actual types before starting an OSR compile.  So we'd iterate across the baseline frame, find the argument values and call TypeScript::SetArgument for them, find the stack values and call TypeScript::Monitor for them, and then begin the ion compilation.

Some issues with this:

1. If argument values are updated in the body, then the argument typesets may get filled with bad typeinfo.
2. We'd need a more in-depth analysis to tell us which op pushed a particular value on the stack.
3. Even this analysis may lead to bad type-info being added to the system.  e.g.:

   function foo() { var x = a ? b() : c(); while(x--) { ... } }

Here, |x| may have been pushed by the JSOP_CALL for b() or c().  We can't tell at OSR time which branch was responsible for the value.  We could update both typesets, but that would once again risk introducing bad types.


I think this is another one of those places where segregated loop compilation would help.  We could simply ignore the typing of the value coming in from before the loop, assign types based on actual types existing at OSR time, and wouldn't have to worry about a Phi between a known type and an unknown type destroying our typeinfo.  The OSR preheader would simply have the OSR entry block as the predecessor instead of also the incoming block from the main script body.
That sounds really complicated.  The phi here should really be a phi(empty, int32) where the first term has an empty type set and no type specialization.  Since no actual types are flowing in from that first value the phi should be specialized to int32.
(Assignee)

Comment 5

4 years ago
Created attachment 8345297 [details] [diff] [review]
Patch

I was wondering why GuessPhiType didn't work as described in comment 4. The problem is that GuessPhiType does the right thing in some cases but not when the *first* operand has an empty typeset.

The easy fix hits some edge cases with cyclic dependencies when running jit-tests, so I had to add some code to handle that.

This fixes the problem with the testcase in comment 0.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8345297 - Flags: review?(bhackett1024)
That patch also fixes the problem with the original testcase where I ran into this.  ;)
Attachment #8345297 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9cab0e7adf

Try: https://tbpl.mozilla.org/?tree=Try&rev=8b72479c7120
https://hg.mozilla.org/mozilla-central/rev/4b9cab0e7adf
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Updated

4 years ago
Duplicate of this bug: 946426
You need to log in before you can comment on or make changes to this bug.