[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()
From:       "sangheon.kim" <sangheon.kim () oracle ! com>
Date:       2017-10-20 17:50:00
Message-ID: 80453515-a89d-cad7-56d9-f01420ba2acb () oracle ! com
[Download RAW message or body]

Hi Thomas,

On 10/20/2017 02:59 AM, Thomas Schatzl wrote:
> Hi Stefan,
>
> On Fri, 2017-10-20 at 11:24 +0200, Stefan Johansson wrote:
>> Hi Thomas,
>>
>> On 2017-10-20 10:32, Thomas Schatzl wrote:
>>> Hi Sangheon,
>>>
>>> On Thu, 2017-10-19 at 12:02 -0700, sangheon.kim wrote:
>>>> Hi Thomas,
>>>>
>>>> On 10/19/2017 10:13 AM, Thomas Schatzl wrote:
>>>>> Hi all,
>>>>>
>>>>>          could I have reviews for this change that replaces some ad-
>>>>> hoc
>>>>> percentage calculations by the global percent_of() method in gc
>>>>> code?
>>>>>
>>>>> CR:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8189666
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~tschatzl/8189666/webrev/
>>>> Looks good as is.
>>>>
>>>> These are minor nits so you can ignore.
>>>> 1. Below lines also can be updated to use percent_of().
>>>>             1) threadLocalAllocBuffer.cpp:278
>>>>         double waste_percent = alloc == 0 ? 0.0 :
>>>>                                                              100.0 * waste / alloc;
>>>>
>>>>             2) threadLocalAllocBuffer.cpp:419
>>>>         double waste_percent = _total_allocation == 0 ? 0.0 :
>>>>                                                       100.0 * waste / _total_allocation;
>>>>
>>>> 2. Copyright needs to be updated for below files:
>>>>             - g1IHOPControl.cpp
>>>>             - g1StringDedupStat.cpp
>>>>             - g1StringDedupTable.cpp
>>>>
>>>        all fixed. Also found some more percent calculations while
>>> widening
>>> my search in metaspace code.
>>>
>>> webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8189666/webrev.1 (full)
>>> http://cr.openjdk.java.net/~tschatzl/8189666/webrev.0_to_1 (diff)
webrev.1 looks good.

Thanks,
Sangheon


>> Thanks for cleaning this up Thomas, one minor thing that you can skip
>> if
>> you don't agree.
>> src/hotspot/share/gc/g1/g1DefaultPolicy.hpp:
>> - Rename reclaimable_bytes_perc to reclaimable_bytes_percent.
>    I fixed this in an extra CR, also out for review.
>
> Thanks for your review.
>
> Thomas

[prev in list] [next in list] [prev in thread] [next in thread]