Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Martin Desruisseaux]

Hi Brian

Le 23/09/14 00:34, Brian Burkhalter a écrit :
> I created an alternate webrev using compile-time constants per your 
> suggestion:
>
> http://cr.openjdk.java.net/~bpb/4477961/webrev.01/

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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Aleksey Shipilev]

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.

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi Aleksey,

On Sep 22, 2014, at 11:42 PM, Aleksey Shipilev  
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/
>> 
> 
> 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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Aleksey Shipilev]

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

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.

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

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  wrote:

> Looks fine to me!
> 
> Mike
> 
> On Sep 22 2014, at 15:34 , Brian Burkhalter  
> wrote:
> 
>> Hi Aleksey,
>> 
>> On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
>> 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
> 

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Mike Duigou]

Looks fine to me!

Mike

On Sep 22 2014, at 15:34 , Brian Burkhalter  wrote:

> Hi Aleksey,
> 
> On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
> 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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi Aleksey,

On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

Hi Aleksey,

On Sep 22, 2014, at 2:43 PM, Aleksey Shipilev  
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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Mike Duigou]

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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Aleksey Shipilev]

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.

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

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

Re: JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Mike Duigou]

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

JDK 9 RFR of 4477961: java.lang.Math.toDegrees(double) could be optimized

[Brian Burkhalter]

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