RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

Thomas Schatzl
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/
Testing:
jprt

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

sangheon.kim@oracle.com
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

Thanks,
Sangheon


> Testing:
> jprt
>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

Thomas Schatzl
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)

Testing:
local build

Thomas
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

Stefan Johansson
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)
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.

Thanks,
Stefan

> Testing:
> local build
>
> Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

Thomas Schatzl
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)
>
> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8189666: Replace various inlined percentage calculations with global percent_of()

sangheon.kim@oracle.com
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