JDK-8140384: TicksToTimeHelper::milliseconds() should return a double

[Bengt Rutisson]

Hi David,

Thanks for looking at this!

On 2015-10-23 12:53, David Holmes wrote:
> On 23/10/2015 6:41 PM, Markus Gronlund wrote:
>> Hi Bengt,
>>
>> Looks good - thanks for fixing.
>
> Arguably if you asked for milliseconds you really didn't care about 
> the fractional part. If you did care then you could just multiply 
> seconds by 1000. But on that basis we could just drop the milliseconds 
> altogether.

I see your point. However, I need the conversion for logging and I would 
like to log with a few decimals. For example "11.34ms". And I think it 
just looks silly in the code to have "* 1000" everywhere. It looks nicer 
to convert to millis directly.

>
>> One small thing:
>>
>> I think we can drop the return (double) cast on line 55, no need for 
>> updated webrevs.
>
> We can drop a most of the double casts This:
>
> static double time_conversion(const Tickspan& span,
>                               TicksToTimeHelper::Unit unit) {
>    double frequency_per_unit = (double)os::elapsed_frequency() / 
> (double)unit;
>
>    return (double) ((double)span.value() / frequency_per_unit);
> }
>
> can become just:
>
>  static double time_conversion(const Tickspan& span,
>                                TicksToTimeHelper::Unit unit) {
>    double frequency_per_unit = os::elapsed_frequency() / (double)unit;
>
>    return span.value() / frequency_per_unit;
> }
>
> promotion rules will ensure each expression is of type double.

Agreed. I will fix this before I push.

Thanks for the review!

Bengt

>
> Cheers,
> David
>
>> Thanks
>> Markus
>>
>> -----Original Message-----
>> From: Bengt Rutisson
>> Sent: den 23 oktober 2015 10:27
>> To: serviceability-dev at openjdk.java.net 
>> serviceability-dev at openjdk.java.net
>> Subject: JDK-8140384: TicksToTimeHelper::milliseconds() should return 
>> a double
>>
>>
>> Hi all,
>>
>> Could I have  a couple of reviews for this small change?
>>
>> http://cr.openjdk.java.net/~brutisso/8140384/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8140384
>>
>> The TicksToTimeHelper class use different return values depending on 
>> if you want to convert to seconds or milliseconds.
>>
>> class TicksToTimeHelper : public AllStatic {
>>    public:
>>     enum Unit {
>>       SECONDS = 1,
>>       MILLISECONDS = 1000
>>     };
>>     static double seconds(const Tickspan& span);
>>     static jlong milliseconds(const Tickspan& span); };
>>
>> No one is using the conversion to milliseconds() but because it 
>> returns a jlong it loses precision.
>>
>> Making milliseconds() return a double allows for coming uses to get 
>> millisecond values with higher precision.
>>
>> Thanks,
>> Bengt
>>