RFR(XS): 8064471: Port 8013895: G1: G1SummarizeRSetStats output on Linux needs improvement to AIX

[Haug, Gunter]

On 12.11.2014 09:04, David Holmes wrote:
> Hi Gunter,
>
> On 11/11/2014 11:23 PM, Haug, Gunter wrote:
>> Hi All,
>>
>> The change '8013895:  (G1: G1SummarizeRSetStats output on Linux needs 
>> improvement)' makes use of getrusage() to retrieve accurate 
>> per-thread data on resource usage. We can use exactly the same code 
>> on AIX to achieve this.
>>
>> Please review the following change:
>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8064471-aixTime/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8064471
>
> I have a couple of comments on this code which presumably also apply 
> to the orginal :(
Yes, they apply to the original as well, see below.
>
> First this comment is no longer applicable (actually it was never 
> applicable to AIX!):
>
>   // For now, we say that linux does not support vtime. I have no idea
>   // whether it can actually be made to (DLD, 9/13/05).
>
You're right. I will remove it.
> Second this calculation seems wrong:
>
> return (double) (usage.ru_utime.tv_sec + usage.ru_stime.tv_sec) + 
> (double) (usage.ru_utime.tv_usec + usage.ru_stime.tv_usec) / (1000 * 
> 1000);
>
> To me this performs integer division (ie truncation_) then converts 
> the resulting integer to a double. I would expect to see additional 
> parentheses (even if not needed, for clarity):
>
> return (double) (usage.ru_utime.tv_sec + usage.ru_stime.tv_sec) + 
> ((double) (usage.ru_utime.tv_usec + usage.ru_stime.tv_usec)) / (1000 * 
> 1000);
>
> or more simply divide by a floating-point value:
>
> return (double) (usage.ru_utime.tv_sec + usage.ru_stime.tv_sec) + 
> (usage.ru_utime.tv_usec + usage.ru_stime.tv_usec) / (1000.0 * 1000);
>
> and you don't need two double casts regardless as the expression will 
> be of type double as soon as there is one operand of type double. So 
> that should reduce to:
>
> return usage.ru_utime.tv_sec + usage.ru_stime.tv_sec + 
> (usage.ru_utime.tv_usec + usage.ru_stime.tv_usec) / (1000.0 * 1000);
>
OK. Do you want that we also change the Linux version like you proposed?

Thanks,
Gunter

> Cheers,
> David
>
>> Thanks,
>> Gunter
>>