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

[David Holmes]

On 14/11/2014 2:39 AM, Haug, Gunter wrote:
>
> 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.

I guess that will have to be me. :) I will try to look at this again 
tomorrow.

David

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