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

[David Holmes]

Gunter,

On 17/11/2014 4:44 PM, David Holmes wrote:
> 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.

The original code was in fact correct - the double cast binds to the 
summation before the division is applied. Given that and the fact the 
linux code doesn't contain the incorrect comment, I don't see any need 
to modify the linux code. You can simply push the AIX change by itself.

Sorry for messing you around on this.

David

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