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

[Haug, Gunter]

On 13.11.2014 09:50, David Holmes wrote:
> 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.

Hi David,

I think it's not worth the effort to make two separate changes on linux 
and aix, so I fixed linux as well. Please find the new webrev below. 
There will probably be more opportunities to test the new process in the 
future.

http://cr.openjdk.java.net/~simonis/webrevs/8064471.v2/ 
<http://cr.openjdk.java.net/%7Esimonis/webrevs/8064471.v2/>


Now we need a sponsor, as it is not aix only anymore.

Thanks,
Gunter


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