Closed
Bug 946425
Opened 11 years ago
Closed 11 years ago
While loop using count-- as its condition is not optimized very well
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: bzbarsky, Assigned: jandem)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
6.09 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
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))
Comment 1•11 years ago
|
||
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•11 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 :(
Comment 3•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
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•11 years ago
|
||
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)
Reporter | ||
Comment 6•11 years ago
|
||
That patch also fixes the problem with the original testcase where I ran into this. ;)
Updated•11 years ago
|
Attachment #8345297 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 7•11 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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•