685150 - Math.min is slower in TI+JM than TM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Shell testcase

See thread at http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/d269deaf42829fef#

On the attached testcase, I get the following numbers on my machine:

-j :
Math.min: 31
Manual: 24
-m :
Math.min: 272
Manual: 126
-m -j -p :
Math.min: 41
Manual: 130
-m -j -p -n :
Math.min: 224
Manual: 52

So the actual Math.min call is just terrible with JM....  TM apparently completely inlines it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I suspect various other things that TM fastpaths have similar issues...  Might be worth going through and making a list so we don't lose all that info again when we do ion.

Comment 2

[Boris Zbarsky [:bzbarsky]]

On the same hardware, v8 is around this:

Math.min: 778
Manual: 614

but that's because its global variable access sucks.  If I wrap the loops in anonymous functions, I get numbers like this:

-j :
Math.min: 30
Manual: 23
-m :
Math.min: 255
Manual: 119
-m -j -p :
Math.min: 37
Manual: 135
-m -j -p -n :
Math.min: 197
Manual: 50
v8:
Math.min: 169
Manual: 42

Note that the "manual" codepath is still faster in TM than either of the others, by the way.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: alternate testcase

This patch accesses an array instead of two loop invariant terms --- using 'a < b' in a condition which always evaluates the same is a little unrealistic I think (and could be hoisted if it came up in practice).  This forces a second trace to be recorded, but doesn't affect the Math.min time (other than an extra array load).

js -j     63 118
js -m -n  290 62
d8        248 62

And, yeah, JM does not inline Math.min.

Comment 4

[Tom S [:evilpie]]

Attachment: v1

Inline Math.max/Math.min. Two different paths for int/double.
There might be a way to use cmov, but this shouldn't be much faster.

Please check the register allocation in the double version, i already made a mistake there, twice while drafting this patch.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 558867 [details] [diff] [review]
v1

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

Thanks!

::: js/src/methodjit/FastBuiltins.cpp
@@ +189,5 @@
> +    JS_ASSERT(!((MaybeJump)notNumber).isSet());
> +    frame.pinReg(fpReg1);
> +    DebugOnly<MaybeJump> notNumber2 = loadDouble(arg2, &fpReg2, &allocate2);
> +    JS_ASSERT(!((MaybeJump)notNumber2).isSet());
> +    frame.unpinReg(fpReg1);

It should be a little faster to do a trick similar to jsop_binary_double, make sure fpReg1 is compiler-owned and use it for the result (removing the need for the pin/unpin as well).  Not a big deal though.

@@ +731,5 @@
>              if (arg2Value.toDouble() == -0.5 || arg2Value.toDouble() == 0.5)
>                  return compileMathPowSimple(arg1, arg2);
>          }
> +        if ((native == js_math_min || native == js_math_max)) {
> +            if (arg1Type == JSVAL_TYPE_INT32 && arg2Type == JSVAL_TYPE_INT32) {

You also need to test the result type here for int32.  The types in the compiler need to reflect inferred types, and if this call has never actually executed then the result type set will be empty and the Math.min native still needs to be called.

@@ +736,5 @@
> +                return compileMathMinMaxInt(arg1, arg2, 
> +                        native == js_math_min ? Assembler::LessThan : Assembler::GreaterThan);
> +            }
> +            if ((arg1Type == JSVAL_TYPE_INT32 || arg1Type == JSVAL_TYPE_DOUBLE) &&
> +                (arg2Type == JSVAL_TYPE_INT32 || arg2Type == JSVAL_TYPE_DOUBLE)) {

Ditto.

Comment 6

[Tom S [:evilpie]]

I still need to change something. (This is why i removed the r?). At the moment the double code does not work correctly with Math.min(0, -0).

But thanks for review.

Comment 7

[Tom S [:evilpie]]

> You also need to test the result type here for int32.  The types in the
> compiler need to reflect inferred types, and if this call has never actually
> executed then the result type set will be empty and the Math.min native
> still needs to be called.

Wouldn't Math.min, with two int arguments always return a int, or is this something we can not trust?

>Ditto.
Okay here it's obvious because in some cases we could return a int and sometimes a double.

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Tom Schuster (evilpie) from comment #7)
> > You also need to test the result type here for int32.  The types in the
> > compiler need to reflect inferred types, and if this call has never actually
> > executed then the result type set will be empty and the Math.min native
> > still needs to be called.
> 
> Wouldn't Math.min, with two int arguments always return a int, or is this
> something we can not trust?

If this callsite has never actually executed, the result type set will be empty.

Comment 9

[Tom S [:evilpie]]

Attachment: v2

I just take a stub call for the hard case.

Comment 10

[Tom S [:evilpie]]

Times in debug build! and -m -j -p -n. (i know mostly useless)

bz
Math.min: 47
Manual: 52
bhackett 
56 60

Comment 11

[Brian Hackett [Laid off!]]

Comment on attachment 559232 [details] [diff] [review]
v2

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

::: js/src/methodjit/FastBuiltins.cpp
@@ +203,5 @@
> +    ifTrue.linkTo(masm.label(), &masm);
> +
> +    /* We would need to handle cases like Math.min(0, -0) correctly */
> +    masm.zeroDouble(fpReg2);
> +    Jump isZero = masm.branchDouble(Assembler::DoubleEqual, fpReg1, fpReg2);

If !allocate2, the FrameState owns fpReg2 and it should not be modified by the compiler.  It should work to use Registers::FPConversionTemp as a scratch register instead.

Comment 12

[(no longer active)]

Backed out from inbound because of make check failures like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=6348206&full=1

Comment 13

[Tom S [:evilpie]]

Attachment: v3

Ugh, how did i miss that, sorry!
NaN is special, because once one of the arguments to min/max is NaN, we are required to return NaN.

Comment 14

[Brian Hackett [Laid off!]]

Comment on attachment 559473 [details] [diff] [review]
v3

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

::: js/src/methodjit/FastBuiltins.cpp
@@ +195,5 @@
> +    DebugOnly<MaybeJump> notNumber2 = loadDouble(arg2, &fpReg2, &allocate);
> +    JS_ASSERT(!((MaybeJump)notNumber2).isSet());
> +
> +
> +    /* We 0 or NaN, because they have special requriments. */

Garbled comment.

Comment 15

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fd47df5b784a

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fd47df5b784a