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

[Bengt Rutisson]

Hi all,
I'll withdraw this review request.

It turns out that I don't need the TicksToTimeHelper::milliseconds() 
conversion and with my changes I may remove the only usage of the 
TicksToTimeHelper::seconds() method too. If things turn out that way we 
could just remove the TicksToTimeHelper class.

I'll leave it untouched for now.

Thanks Markus and David for looking at this anyway!
Bengt

On 2015-10-23 13:13, Bengt Rutisson wrote:

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@openjdk.java.net 
serviceability-dev@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