936234 - Inline n-Ary Math.min and Math.max

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Michael Bebenita [:mbx]]

Attachment: min-example.js

It's often necessary to take the min or max of 4 values when computing (Axially Aligned Bounding Boxes) and it looks like Math.min/max(a, b, c, d) doesn't get inlined. The performance difference between a hand rolled min/max and the Math.min/max can be up to one order of magnitude. We should probably special case the n-ary case, at least up to 4 arguments.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: Chain MMinMax to inline n-Aray Math.min and Math.max

Test case in Comment 0 before/after:

> Measure: Math.min() Count: 1024 Elapsed: 853.0000 (0.8330)
> Measure: min() Count: 1024 Elapsed: 96.0000 (0.0938)

> Measure: Math.min() Count: 1024 Elapsed: 43.0000 (0.0420)
> Measure: min() Count: 1024 Elapsed: 96.0000 (0.0938)

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 828946 [details] [diff] [review]
Chain MMinMax to inline n-Aray Math.min and Math.max

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

Stealing review. Looks good.

::: js/src/jit/MCallOptimize.cpp
@@ +874,5 @@
>  
>      callInfo.unwrapArgs();
>  
> +    // Chain N-1 MMinMax instructions to compute the MinMax.
> +    MMinMax *last = MMinMax::New(callInfo.getArg(0), callInfo.getArg(1), returnType, max);

Not something this patch should do, but some thoughts below.

Is it worth it to save a few conversions here (and below) by using the argument type of the two operands, if they are the same? So that if we are inlining something like Math.min(3,4,1.2), we wouldn't need to convert all the int32 operands to the final return type of double. We can save more conversions if we also reorder the operands and batch them by type.

That said, a pattern like Math.min(3,4,1.2) is probably pretty rare.

Comment 3

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a39a26962730

That would be reasonable followup, were we inclined to further optimize Math.max/min.

Comment 4

[Carsten Book [:Tomcat]]

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