Math.min is slower in TI+JM than TM

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {perf, regression, testcase})

unspecified
mozilla9
perf, regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], URL)

Attachments

(3 attachments, 2 obsolete attachments)

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.
Blocks: 619425
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.
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.
(Assignee)

Updated

6 years ago
Assignee: general → evilpies
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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #558867 - Flags: review?(bhackett1024)
(Assignee)

Updated

6 years ago
Attachment #558867 - Flags: review?(bhackett1024)
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.
Attachment #558867 - Flags: review+
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
> 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.
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 559232 [details] [diff] [review]
v2

I just take a stub call for the hard case.
Attachment #558867 - Attachment is obsolete: true
Attachment #559232 - Flags: review?(bhackett1024)
(Assignee)

Comment 10

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

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

Updated

6 years ago
Keywords: perf, regression, testcase
Summary: Math.min performance regression → Math.min is slower in TI+JM than TM
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.
Attachment #559232 - Flags: review?(bhackett1024) → review+
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [inbound]
Backed out from inbound because of make check failures like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=6348206&full=1
Whiteboard: [inbound]
(Assignee)

Comment 13

6 years ago
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.
Attachment #559232 - Attachment is obsolete: true
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.
Attachment #559473 - Flags: review+
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd47df5b784a
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/fd47df5b784a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.