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

[Ramki Ramakrishna]

Looks good...

On 10/5/2011 7:43 AM, Tony Printezis wrote:
> Ramki, thanks for the code review! Yes, good observation re: not 
> needing sort_end_sec. Changed it to this:
>
>  ...
>  _collectionSetChooser->sortMarkedHeapRegions();
>  double end_sec = os::elapsedTime();
>  if (G1PrintParCleanupStats) {
>    gclog_or_tty->print_cr("  sorting: %8.3f ms.",
>                           (end_sec - known_garbage_end_sec) * 1000.0);
>  }
>
>  double elapsed_time_ms = (end_sec - _mark_cleanup_start_sec) * 1000.0;
>  ...
>
> Y. S. Ramakrishna wrote:
>> g1CollectorPolicy.cpp, Lines 2644, 2646 and 2651: you don't need both 
>> sort_end_sec and end_sec.
>> Only one of them will do. Rest looks fine.
>>
>> -- ramki
>>
>> On 10/04/11 14: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