RFR (M) JDK-8151178: Move the collection set out of the G1 collector policy

[Tom Benson]

Hi Mikael,
Looks good to me.   I like the idea of changing _g1p to _policy.
Tom

On 3/7/2016 9:32 AM, Mikael Gerdin wrote:
> Hi Thomas,
>
> On 2016-03-07 15:25, Thomas Schatzl wrote:
>> Hi Mikael,
>>
>> On Mon, 2016-03-07 at 10:46 +0100, Mikael Gerdin wrote:
>>> Hi all,
>>>
>>> Here's a new version of the webrev based on feedback from Jesper and
>>> Tom.
>>>
>>> http://cr.openjdk.java.net/~mgerdin/8151178/version2/
>>>
>>> The incremental layout is similar to version1, with the addition of
>>> webrev_4 which contains the review changes.
>>>
>>> In this update I also took the liberty to change G1CollectionSet to
>>> become a value member in G1CollectedHeap instead of a separately C
>>> -heap
>>> allocated object. I see no value in it being allocated separately.
>>
>>    some minor issues:
>>
>> - maybe the include changes in g1CollectorState.hpp should be a
>> separate CR, but then again, it's very small.
>
> I had to change g1CollectorState.hpp, I could not include it otherwise 
> (unless including allocation.hpp before). I took the liberty to sort 
> it while I was editing its include list.
>
>>
>> - instead of G1CollectionSet::set_g1p() I would have chosen
>>   G1CollectionSet::set_policy() for a name.
>
> Hm, yeah I was torn on naming this. On one hand we usually name 
> accessors after the member variable name. On the other hand I agree 
> that set_g1p is not a nice name. I also thought about "initialize" or 
> something, but that makes it sound like it does a lot more.
>
> Maybe I should just rename it _policy an have a set_policy?
>
>>
>> - as a future cleanup suggestion, maybe the CollectionSetChooser could
>> be one of the first targets, and probably hidden from direct access by
>> the G1CollectorPolicy. Not sure.
>
> It might be possible, but it's not on my refactor list at this time, 
> unfortunately.
>
>>
>> - g1CollectorPolicy.cpp:977 - superfluous newline ;)
>
> Will fix.
>
> Thanks for the review.
>
> /Mikael
>
>>
>> Feel free to implement any of these suggestions, no need for re-review.
>>
>> Thanks,
>>    Thomas
>>