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

[David Holmes]

On 13/11/2014 1:19 AM, Haug, Gunter wrote:
>
> 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?

I'll leave it up to you. If you leave this as AIX only then it tests the 
new process :) There can be a follow up cleanup bug for linux.

Thanks,
David

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