RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

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

RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

Leo Korinth
Hi,

The bot_updates parameter is removed so that the code is easier to read.
The original idea behind the parameter is that the compiler will be able
to remove a branch instruction (in certain cases) because the value will
be known at compile time. As several classes inherits from the base
G1AllocRegion the member variable _bot_updates is not known to the base
class though it is always set to the same value in each of the separate
sub classes.

I made the constructor G1GCAllocRegion protected (from public) to better
show that that each *AllocRegion class with public constructor will have
a fixed _bot_updates value (per class).

Another solution (not the proposed solution) is to use templates. This
will give the compiler exact knowledge of the _bot_updates value at
compile time. Templates could also be used to give compile time
information about the value of InCSetState _purpose in the classes
SurvivorGCAllocRegion and OldGCAllocRegion.

The template solution is quite verbose as parts of the code is located
in a cpp file. Because of this I think just removing the parameter (the
propose solution) is the way to go. The code becomes easier to read, the
change is smaller, and energy can be put in code paths that will make a
real difference.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8133805

Webrev:
http://cr.openjdk.java.net/~lkorinth/8133805/00/

Testing:
mach5 hs-tier1,hs-tier2

Thanks,
Leo
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

Thomas Schatzl
Hi,

On Mon, 2017-12-11 at 15:32 +0100, Leo Korinth wrote:

> Hi,
>
> The bot_updates parameter is removed so that the code is easier to
> read.
> The original idea behind the parameter is that the compiler will be
> able to remove a branch instruction (in certain cases) because the
> value will be known at compile time. As several classes inherits from
> the base G1AllocRegion the member variable _bot_updates is not known
> to the base class though it is always set to the same value in each
> of the separate sub classes.
>
> I made the constructor G1GCAllocRegion protected (from public) to
> better show that that each *AllocRegion class with public constructor
> will have a fixed _bot_updates value (per class).
>
[...]
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8133805
>
> Webrev:
> http://cr.openjdk.java.net/~lkorinth/8133805/00/
>
> Testing:
> mach5 hs-tier1,hs-tier2

- in g1AllocRegion.hpp, please fix the indentation of the parameters
for G1AllocRegion::allocate() and par_allocate().

looks good otherwise.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

Stefan Johansson


On 2017-12-12 13:07, Thomas Schatzl wrote:

> Hi,
>
> On Mon, 2017-12-11 at 15:32 +0100, Leo Korinth wrote:
>> Hi,
>>
>> The bot_updates parameter is removed so that the code is easier to
>> read.
>> The original idea behind the parameter is that the compiler will be
>> able to remove a branch instruction (in certain cases) because the
>> value will be known at compile time. As several classes inherits from
>> the base G1AllocRegion the member variable _bot_updates is not known
>> to the base class though it is always set to the same value in each
>> of the separate sub classes.
>>
>> I made the constructor G1GCAllocRegion protected (from public) to
>> better show that that each *AllocRegion class with public constructor
>> will have a fixed _bot_updates value (per class).
>>
> [...]
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8133805
>>
>> Webrev:
>> http://cr.openjdk.java.net/~lkorinth/8133805/00/
>>
>> Testing:
>> mach5 hs-tier1,hs-tier2
> - in g1AllocRegion.hpp, please fix the indentation of the parameters
> for G1AllocRegion::allocate() and par_allocate().
>
> looks good otherwise.
I agree, align the parameters and this is good to go.

Thanks,
Stefan
>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

Leo Korinth
Thanks for your reviews Thomas and Stefan!

>> - in g1AllocRegion.hpp, please fix the indentation of the parameters
>> for G1AllocRegion::allocate() and par_allocate().
>>
>> looks good otherwise.
> I agree, align the parameters and this is good to go.
>

Oops, I have now fixed the indentation:

http://cr.openjdk.java.net/~lkorinth/8133805/00-01/
http://cr.openjdk.java.net/~lkorinth/8133805/01/

Thanks,
Leo

> Thanks,
> Stefan
>>
>> Thanks,
>>    Thomas
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

Stefan Johansson


On 2017-12-12 19:27, Leo Korinth wrote:

> Thanks for your reviews Thomas and Stefan!
>
>>> - in g1AllocRegion.hpp, please fix the indentation of the parameters
>>> for G1AllocRegion::allocate() and par_allocate().
>>>
>>> looks good otherwise.
>> I agree, align the parameters and this is good to go.
>>
>
> Oops, I have now fixed the indentation:
>
> http://cr.openjdk.java.net/~lkorinth/8133805/00-01/
> http://cr.openjdk.java.net/~lkorinth/8133805/01/
>
Nice, ship it!

Stefan

> Thanks,
> Leo
>
>> Thanks,
>> Stefan
>>>
>>> Thanks,
>>>    Thomas
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8133805: Remove the bot_updates parameter from G1Allocator's allocation methods

Thomas Schatzl
In reply to this post by Leo Korinth
Hi,

On Tue, 2017-12-12 at 19:27 +0100, Leo Korinth wrote:

> Thanks for your reviews Thomas and Stefan!
>
> > > - in g1AllocRegion.hpp, please fix the indentation of the
> > > parameters
> > > for G1AllocRegion::allocate() and par_allocate().
> > >
> > > looks good otherwise.
> >
> > I agree, align the parameters and this is good to go.
> >
>
> Oops, I have now fixed the indentation:
>
> http://cr.openjdk.java.net/~lkorinth/8133805/00-01/
> http://cr.openjdk.java.net/~lkorinth/8133805/01/
>

 ship it.

Thomas