CRR (Small-ish): 7088680: G1: Cleanup in the G1CollectorPolicy class

[Y. S. Ramakrishna]

I think this text belongs to your other CRR: "CRR (S/M): 7092309: G1: introduce old region set" :-)

On 10/04/11 15:24, Tony Printezis wrote:
> I also meant to add to the CRR text: While reviewing this change please 
> consider the following. Before a Full GC we tear down most region lists 
> / sets, but leave the humongous set as is (as we currently don't move 
> humongous regions during the Full GC). During the Full GC, any humongous 
> regions we find empty we have to remove from the humongous set. After 
> the Full GC, while we recreate the other region lists / sets, we don't 
> have to recreate the humongous set, as it's already correctly formed. I 
> think the code might be a bit more straightforward if we also tear-down 
> / recreate the humongous region set along with the rest, especially 
> given that we might want to move some humongous regions in the future 
> during a Full GC. Let me know if you think this will be a worthwhile 
> change (we can do this as a separate CR).
> 
> On 10/04/2011 05:01 PM, Tony Printezis wrote:
>> Thanks Mikael,
>>
>> Yes, I accidentally posted the wrong links. The correct ones are:
>>
>> all) http://cr.openjdk.java.net/~tonyp/7088680/webrev.0/webrev.all/
>>
>> 1) 
>> http://cr.openjdk.java.net/~tonyp/7088680/webrev.0/webrev.0.G1G1PCleanupFieldMethodRemoval/ 
>>
>>
>> 2) 
>> http://cr.openjdk.java.net/~tonyp/7088680/webrev.0/webrev.1.G1G1PCleanupMethodRefactoring/ 
>>
>>
>> 3) 
>> http://cr.openjdk.java.net/~tonyp/7088680/webrev.0/webrev.2.G1G1PCleanupDevirtualize/ 
>>
>>
>> Apologies!
>>
>> Tony
>>
>> On 10/4/2011 4:14 PM, Mikael Gerdin wrote:
>>> Maybe you should upload your webrevs to cr.openjdk.java.net :)
>>> /mgerdin
>>>
>>> On Tuesday 04 October 2011 14.57.10 Tony Printezis wrote:
>>>> Hi all,
>>>>
>>>> I'd like to get a couple of code reviews for some cleanup in the
>>>> G1CollectorPolicy class. The changes were motivated by code reviewers'
>>>> comments on a previous CR (thanks John). The webrev for all the changes
>>>> is here:
>>>>
>>>> http://syros.us.oracle.com/local_ws/hs-g1-g1p-cleanup/7088680/webrev.0/webre 
>>>>
>>>> v.all/
>>>>
>>>> I have broken the changes down in three different webrevs in case they
>>>> are a little easier to code review this way:
>>>>
>>>> 1) Remove unnecessary fields and methods:
>>>>
>>>> http://syros.us.oracle.com/local_ws/hs-g1-g1p-cleanup/7088680/webrev.0/webre 
>>>>
>>>> v.0.G1G1PCleanupFieldMethodRemoval/
>>>>
>>>> 2) Remove the G1CollectoryPolicy_BestRegionsFirst class and fold its
>>>> functionality in its superclass, i.e., G1CollectorPolicy. Currently, we
>>>> only have one policy so there's no point in having the separation. And
>>>> the separation was done in a very ad-hoc way anyway. If we want to have
>>>> more than one policy in the future we should really rethink on how 
>>>> it is
>>>> done. Most of the changes were straightforward, the slightly more
>>>> involved one was to combine various
>>>> record_concurrent_mark_cleanup_end*() methods into one and rename some
>>>> of the variables to make sure their names are consistent. I also 
>>>> changed
>>>> protected fields into private, given that protected fields again do not
>>>> make sense any more.
>>>>
>>>> http://syros.us.oracle.com/local_ws/hs-g1-g1p-cleanup/7088680/webrev.0/webre 
>>>>
>>>> v.1.G1G1PCleanupMethodRefactoring/
>>>>
>>>> 3) Make several methods in G1CollectorPolicy non-virtual as it makes no
>>>> sense for them to be virtual:
>>>>
>>>> http://syros.us.oracle.com/local_ws/hs-g1-g1p-cleanup/7088680/webrev.0/webre 
>>>>
>>>> v.2.G1G1PCleanupDevirtualize/
>>>>
>>>> Thanks,
>>>>
>>>> Tony