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

[Volker Simonis]

OK, thanks.

Just pushed it to hotspot-rt and it worked!


Regards,
Volker


On Tue, Nov 18, 2014 at 5:04 AM, David Holmes <david.holmes at oracle.com> wrote:
> 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
>>>>>>>
>>>>>
>>>
>