JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized
Hello, Another relatively small numerics fix: Issue: https://bugs.openjdk.java.net/browse/JDK-4477961 Webrev: http://cr.openjdk.java.net/~bpb/4477961/webrev.00/ Summary: Change constructs like “a * B / C” and “u / V * W” to “x * (Y / Z)” where lower case denotes a variable and upper case a constant. This forces the constant value (Y / Z) to be evaluated only once per VM instance, and replaces division of the variable with multiplication. The resulting performance improvement is more than 300% as measure by JMH on a MacBookPro11,1 dual core i7 running Mac OS 10.9.5. Thanks, Brian
Looks fine. I think it is always important note when a change may result in different results for some inputs. I will reiterate for the record what's mentioned in the bug:
However, one caveat is that this may affect the results of some calculations. For example, some range of numbers that used to overflow to infinity by performing the multiplication by 180, will now not overflow and will return a valid result.
This also applies to very small quantities in toRadians where dividing by 180 may have previously resulted in a zero. Cheers, Mike On Sep 22 2014, at 14:10 , Brian Burkhalter <Brian.Burkhalter@oracle.com> wrote:
Hello,
Another relatively small numerics fix:
Issue: https://bugs.openjdk.java.net/browse/JDK-4477961 Webrev: http://cr.openjdk.java.net/~bpb/4477961/webrev.00/
Summary: Change constructs like “a * B / C” and “u / V * W” to “x * (Y / Z)” where lower case denotes a variable and upper case a constant. This forces the constant value (Y / Z) to be evaluated only once per VM instance, and replaces division of the variable with multiplication. The resulting performance improvement is more than 300% as measure by JMH on a MacBookPro11,1 dual core i7 running Mac OS 10.9.5.
Thanks,
Brian
Indeed these considerations made me a little nervous about the change. For the edge cases which would have previously overflowed or underflowed this does not seem like a problem, i.e., to obtain a valid result where one would not have done before. For the cases in between however I suppose that there will be some numerical inconsistencies between the two versions. Brian On Sep 22, 2014, at 2:24 PM, Mike Duigou <mike.duigou@oracle.com> wrote:
Looks fine.
I think it is always important note when a change may result in different results for some inputs. I will reiterate for the record what's mentioned in the bug:
However, one caveat is that this may affect the results of some calculations. For example, some range of numbers that used to overflow to infinity by performing the multiplication by 180, will now not overflow and will return a valid result.
This also applies to very small quantities in toRadians where dividing by 180 may have previously resulted in a zero.
Hi Brian; I thought it was worth mentioning because I had had to deal with the underflow issue in the mapping software for the Audi/Stanford autonomous. For various reasons that system ends up with many very-tiny-but-non-zero quantities in it's mapping and heading component and I initially had trouble matching results against the Matlab implementation. Since I had to also reduce quantities to -π < x <= π and -180 < x <= 180 I combined the reduce and convert using an approach similar to this revised implementation. Mike On Sep 22 2014, at 14:41 , Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Indeed these considerations made me a little nervous about the change. For the edge cases which would have previously overflowed or underflowed this does not seem like a problem, i.e., to obtain a valid result where one would not have done before. For the cases in between however I suppose that there will be some numerical inconsistencies between the two versions.
Brian
On Sep 22, 2014, at 2:24 PM, Mike Duigou <mike.duigou@oracle.com> wrote:
Looks fine.
I think it is always important note when a change may result in different results for some inputs. I will reiterate for the record what's mentioned in the bug:
However, one caveat is that this may affect the results of some calculations. For example, some range of numbers that used to overflow to infinity by performing the multiplication by 180, will now not overflow and will return a valid result.
This also applies to very small quantities in toRadians where dividing by 180 may have previously resulted in a zero.
Hi Mike, It’s definitely worth mentioning and something which should be taken into consideration when considering the change. Thanks, Brian On Sep 22, 2014, at 2:48 PM, Mike Duigou <mike.duigou@oracle.com> wrote:
I thought it was worth mentioning because I had had to deal with the underflow issue in the mapping software for the Audi/Stanford autonomous. For various reasons that system ends up with many very-tiny-but-non-zero quantities in it's mapping and heading component and I initially had trouble matching results against the Matlab implementation. Since I had to also reduce quantities to -π < x <= π and -180 < x <= 180 I combined the reduce and convert using an approach similar to this revised implementation.
Hi Brian, Looks OK. On 09/23/2014 01:10 AM, Brian Burkhalter wrote:
Summary: Change constructs like “a * B / C” and “u / V * W” to “x * (Y / Z)” where lower case denotes a variable and upper case a constant. This forces the constant value (Y / Z) to be evaluated only once per VM instance, and replaces division of the variable with multiplication. The resulting performance improvement is more than 300% as measure by JMH on a MacBookPro11,1 dual core i7 running Mac OS 10.9.5.
Hm, and this compiler transformation works in strictfp context? I hope compilers do the constant folding in strictfp/fdlibm mode... I would probably just go and declare the private compile-time constants. This is safer, since: a) you are not at the mercy of optimizing compiler anymore (have you tried the benchmark with C1?); b) the initializing expressions are FP-strict, less opportunity for hard to diagnose numeric problem. -Aleksey.
Hi Aleksey, On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev <aleksey.shipilev@oracle.com> wrote:
I would probably just go and declare the private compile-time constants. This is safer, since: a) you are not at the mercy of optimizing compiler anymore (have you tried the benchmark with C1?); b) the initializing expressions are FP-strict, less opportunity for hard to diagnose numeric problem.
I actually tested with compile-time constants and the result was the same as for what I posted. Your suggestion is a good one however so I will update and repost. Thanks, Brian
Hi Aleksey, On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev <aleksey.shipilev@oracle.com> wrote:
Hm, and this compiler transformation works in strictfp context? I hope compilers do the constant folding in strictfp/fdlibm mode…
Yes.
I would probably just go and declare the private compile-time constants. This is safer, since: a) you are not at the mercy of optimizing compiler anymore (have you tried the benchmark with C1?); b) the initializing expressions are FP-strict, less opportunity for hard to diagnose numeric problem.
I created an alternate webrev using compile-time constants per your suggestion: http://cr.openjdk.java.net/~bpb/4477961/webrev.01/ The performance improvement is similar to that cited for webrev.00. If this version is preferable it will need approval from a JDK 9 Reviewer, of course. Thanks, Brian
Looks fine to me! Mike On Sep 22 2014, at 15:34 , Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Hi Aleksey,
On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev <aleksey.shipilev@oracle.com> wrote:
Hm, and this compiler transformation works in strictfp context? I hope compilers do the constant folding in strictfp/fdlibm mode…
Yes.
I would probably just go and declare the private compile-time constants. This is safer, since: a) you are not at the mercy of optimizing compiler anymore (have you tried the benchmark with C1?); b) the initializing expressions are FP-strict, less opportunity for hard to diagnose numeric problem.
I created an alternate webrev using compile-time constants per your suggestion:
http://cr.openjdk.java.net/~bpb/4477961/webrev.01/
The performance improvement is similar to that cited for webrev.00.
If this version is preferable it will need approval from a JDK 9 Reviewer, of course.
Thanks,
Brian
Hi Mike, Thanks for the review. For the sake of completeness I tested converting 89.0 * Double.MIN_VALUE to radians and Double.MAX_VALUE / 89.0 to degrees. The old version gives 0.0 and Double.POSITIVE_INFINITY, respectively, whereas the webrev.01 version gives the respective results 1.0E-323 and 1.1573059492949775E308. Brian On Sep 22, 2014, at 4:24 PM, Mike Duigou <mike.duigou@oracle.com> wrote:
Looks fine to me!
Mike
On Sep 22 2014, at 15:34 , Brian Burkhalter <brian.burkhalter@oracle.com> wrote:
Hi Aleksey,
On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev <aleksey.shipilev@oracle.com> wrote:
Hm, and this compiler transformation works in strictfp context? I hope compilers do the constant folding in strictfp/fdlibm mode…
Yes.
I would probably just go and declare the private compile-time constants. This is safer, since: a) you are not at the mercy of optimizing compiler anymore (have you tried the benchmark with C1?); b) the initializing expressions are FP-strict, less opportunity for hard to diagnose numeric problem.
I created an alternate webrev using compile-time constants per your suggestion:
http://cr.openjdk.java.net/~bpb/4477961/webrev.01/
The performance improvement is similar to that cited for webrev.00.
If this version is preferable it will need approval from a JDK 9 Reviewer, of course.
Thanks,
Brian
Hi Brian, On 09/23/2014 02:34 AM, Brian Burkhalter wrote:
I created an alternate webrev using compile-time constants per your suggestion:
http://cr.openjdk.java.net/~bpb/4477961/webrev.01/ <http://cr.openjdk.java.net/%7Ebpb/4477961/webrev.01/>
Ah, sorry for confusing language about "compile-time constants", I meant "compile-time constant expression", as per JLS 15.28. Constant expressions are FP-strict, so it should be just fine correctness- and performance-wise, and less cryptic: private static final double DEGREES_TO_RADIANS = PI / 180.0; Thanks, -Aleksey.
Hi Aleksey, On Sep 22, 2014, at 11:42 PM, Aleksey Shipilev <aleksey.shipilev@oracle.com> wrote:
On 09/23/2014 02:34 AM, Brian Burkhalter wrote:
I created an alternate webrev using compile-time constants per your suggestion:
http://cr.openjdk.java.net/~bpb/4477961/webrev.01/ <http://cr.openjdk.java.net/%7Ebpb/4477961/webrev.01/>
Ah, sorry for confusing language about "compile-time constants", I meant "compile-time constant expression", as per JLS 15.28. Constant expressions are FP-strict, so it should be just fine correctness- and performance-wise, and less cryptic: private static final double DEGREES_TO_RADIANS = PI / 180.0;
Thanks for the clarification. As the issue is already resolved, however, I am not inclined to change the code unless there is a serious objection in which case I can file another issue. Thanks, Brian
On 09/23/2014 07:12 PM, Brian Burkhalter wrote:
Ah, sorry for confusing language about "compile-time constants", I meant "compile-time constant expression", as per JLS 15.28. Constant expressions are FP-strict, so it should be just fine correctness- and performance-wise, and less cryptic: private static final double DEGREES_TO_RADIANS = PI / 180.0;
Thanks for the clarification. As the issue is already resolved, however, I am not inclined to change the code unless there is a serious objection in which case I can file another issue.
Ok, that's fine, keep the constant. -Aleksey.
Hi Brian Le 23/09/14 00:34, Brian Burkhalter a écrit :
I created an alternate webrev using compile-time constants per your suggestion:
On a very minor formatting detail, the change in StrictMath has a misplaced empty line (before the RADIANS_TO_DEGREES constant, while it should be after). Also, would it be worth to remove the "private" modifier in StrictMath.DEGREES_TO_RADIANS and StrictMath.RADIANS_TO_DEGREES, and have Math.toDegrees and Math.toRadians to use the StrictMath constants instead than duplicating them? It would reduce slightly the size of the Math.class file by avoiding 2 field declarations. Martin
participants (4)
-
Aleksey Shipilev
-
Brian Burkhalter
-
Martin Desruisseaux
-
Mike Duigou