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

[Bengt Rutisson]

Tony,

On 2011-10-05 16:43, Tony Printezis wrote:
> Bengt,
>
> I have a second cleanup change that's coming up that should remove 
> some of the methods you're referring to. Can I do that first? I can 
> open a new CR for the cleanup you proposed and extend it to also make 
> sure that if-statements always have curly brackets (i.e., a similar 
> change to what I did for the CM files).

You don't have to open a separate CR for the changes I proposed. I just 
didn't understand why you changed some method declarations but not all 
of them. If you don't want to do it I think you should just let it be 
for now.

I'm fine with pushing the change as it is.

Bengt

>
> Tony
>
> Bengt Rutisson wrote:
>>
>> Looks good to me.
>>
>> One question:
>>
>> In several places you have made sure that the method declarations are 
>> on a single line (not having the return type on a separate line 
>> above). I like this, but there are still lots of methods that have 
>> their return type on a separate line. Since this is a cleanup CR I 
>> would suggest changing all method declarations to be consistent.
>>
>> Bengt
>>
>> On 2011-10-04 23:01, 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
>>