1131537 - Use Baseline feedback for MMul ops added for JSOP_POS

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

We're 10x slower than V8 on a underscore.js benchmark (see bug 1131099 comment 2), because we spend most of our time in a slow MulValues path for a JSOP_POS emitted for the "i++" below.

Assigning isSorted to i seems to confuse things, because isSorted can be |undefined|. If I add "i = i|0;" before the loop we're faster than V8, though V8 also gets a bit faster so it's possible they have a similar problem.

Usually we use Baseline feedback for binary instructions to handle the is-never-undefined-at-that-point case but here the Mul instruction is for a JSOP_POS instead of JSOP_MUL and that's probably not handled right now.

Also note that Ion is able to completely eliminate the "if (typeof isSorted == 'number')" branch, but that happens in a later pass. There are a few more things we should fix here, to be more robust, will file separate bugs for those.

    if (isSorted) {
      if (typeof isSorted == 'number') {
        i = isSorted < 0 ? Math.max(0, length + isSorted) : isSorted;
      } else {
        ...
      }
    }
    for (; i < length; i++) {
      ...
    }

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> There are a few more
> things we should fix here, to be more robust, will file separate bugs for
> those.

Making the improveTypesAtTest infrastructure handle "if (typeof isSorted == 'number')" is bug 994016.

That also avoids the perf issue here because we won't end up with |undefined| for isSorted and |i|...

Comment 2

[Tom S]

Would it make sense to add a new instruction for this to IonMonkey and get the type feedback from the IC stub?

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Jan de Mooij [:jandem] from comment #0)
> > There are a few more
> > things we should fix here, to be more robust, will file separate bugs for
> > those.
> 
> Making the improveTypesAtTest infrastructure handle "if (typeof isSorted ==
> 'number')" is bug 994016.
> 
> That also avoids the perf issue here because we won't end up with
> |undefined| for isSorted and |i|...

Indeed ;)
http://arewefastyet.com/#machine=28&view=single&suite=misc&subtest=bugs-1131099-underscore&start=1425023453&end=1425162164

Comment 4

[Jan de Mooij [:jandem]]

Awesome :)

Comment 5

[Till Schneidereit [:till]]

The test case from bug 1131099 is much faster in our shell than the d8 (or jsc) shell now:

SM  180/187/3195ms
d8  360/365/4770ms
jsc 383/318/22907ms