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

[Tony Printezis]

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