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.

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.

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.

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 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.

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.

> 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.

I just take a stub call for the hard case.

Times in debug build! and -m -j -p -n. (i know mostly useless) bz Math.min: 47 Manual: 52 bhackett 56 60

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.

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

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 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.