Last Comment Bug 685150 - Math.min is slower in TI+JM than TM
: Math.min is slower in TI+JM than TM
Status: RESOLVED FIXED
[inbound]
: perf, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
http://jsperf.com/math-min-vs-primiti...
Depends on:
Blocks: infer-perf-regress
  Show dependency treegraph
 
Reported: 2011-09-07 07:38 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2011-09-23 20:52 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Shell testcase (317 bytes, text/plain)
2011-09-07 07:38 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
alternate testcase (473 bytes, application/x-javascript)
2011-09-07 09:22 PDT, Brian Hackett (:bhackett)
no flags Details
v1 (7.63 KB, patch)
2011-09-07 10:22 PDT, Tom Schuster [:evilpie]
bhackett1024: review+
Details | Diff | Review
v2 (8.41 KB, patch)
2011-09-08 11:49 PDT, Tom Schuster [:evilpie]
bhackett1024: review+
Details | Diff | Review
v3 (8.82 KB, patch)
2011-09-09 08:24 PDT, Tom Schuster [:evilpie]
bhackett1024: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-07 07:38:58 PDT
Created attachment 558799 [details]
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 [:bz] (Out June 25-July 6) 2011-09-07 07:49:57 PDT
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 [:bz] (Out June 25-July 6) 2011-09-07 07:59:06 PDT
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 (:bhackett) 2011-09-07 09:22:33 PDT
Created attachment 558841 [details]
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 Schuster [:evilpie] 2011-09-07 10:22:12 PDT
Created attachment 558867 [details] [diff] [review]
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 (:bhackett) 2011-09-07 13:56:40 PDT
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 Schuster [:evilpie] 2011-09-07 13:59:17 PDT
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 Schuster [:evilpie] 2011-09-07 14:03:48 PDT
> 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 (:bhackett) 2011-09-07 14:05:58 PDT
(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 Schuster [:evilpie] 2011-09-08 11:49:32 PDT
Created attachment 559232 [details] [diff] [review]
v2

I just take a stub call for the hard case.
Comment 10 Tom Schuster [:evilpie] 2011-09-08 11:56:07 PDT
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 (:bhackett) 2011-09-08 14:28:19 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-09 07:06:47 PDT
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 Schuster [:evilpie] 2011-09-09 08:24:13 PDT
Created attachment 559473 [details] [diff] [review]
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 (:bhackett) 2011-09-23 07:33:27 PDT
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 16 Ed Morley [:emorley] 2011-09-23 20:52:18 PDT
https://hg.mozilla.org/mozilla-central/rev/fd47df5b784a

Note You need to log in before you can comment on or make changes to this bug.