Request for review: 8007053: Refactor SizePolicy code for consistency across collectors

[Tao Mao]

Bengt, or the ease of reviewing, I took your suggestion to split up the 
CR further into three pieces.

OK. From now on, please ignore this request thread and look at the 
separate review requests. Thanks.

CR 8007763: Rename a bunch of methods in size policy across collecotors
CR 8007763: Refactoring: split up compute_generations_free_space() into 
two functions for class PSAdaptiveSizePolicy
CR 8007764: Wrong initialized value of max_gc_pause_sec for an instance 
of class AdaptiveSizePolicy

On 1/31/2013 6:22 AM, Bengt Rutisson wrote:
>
> Hi Tao,
>
> I have only looked briefly at your webrev, but I have a request before 
> I look more at it.
>
> Could you split it up into a few different changes? I find it 
> difficult get a grasp of the changes when there are so many changed 
> files and they contain changes for different reasons.
>
> You probably know better than me how to split this up. But I think I 
> would like to have at least these three separate changes:
>
> * Renaming of methods
> * Splitting up compute_generations_free_space()
> * Changes to use MaxGCPauseMillis instead of MaxGCMinorPauseMillis
>
> If this division does not make sense to you I have probably drawn the 
> wrong conclusions. But we need to split the review up in some way to 
> make it easier to review. I'm fine with other ways of splitting it up 
> if you find more natural ways of splitting it up.
>
> With splitting up the changes I mean filing separate bugs and 
> submitting separate webrevs, so that they can be pushed as separate 
> changesets.
>
> Also, I don't think you should include the white space changes to 
> these two files:
> psGCAdaptivePolicyCounters.hpp
> adaptiveSizePolicy.hpp
>
> Thanks,
> Bengt
>
>
> On 1/31/13 1:37 AM, Tao Mao wrote:
>> 8007053: Refactor SizePolicy code for consistency across collectors
>> https://jbs.oracle.com/bugs/browse/JDK-8007053
>>
>> webrev:
>> http://cr.openjdk.java.net/~tamao/8007053/webrev.00/
>>
>> changeset:
>> 1. rename a bunch of functions across collectors related to compute 
>> and resize young/tenured gens
>> unified function names:
>> (1) compute_*:
>>       compute_survivor_space_size_and_threshold()
>>       compute_generations_free_space() [compute_eden_space_size() + 
>> compute_tenured_generation_free_space()]
>> (2) resize_*_gen:
>>       resize_young_gen()
>>       resize_tenured_gen()
>>
>> 2. split compute_generations_free_space() into two functions:
>> compute_eden_space_size() + compute_tenured_generation_free_space()
>> each of which (if needed) can be reused without executing an overhead 
>> of the other.
>>
>> *3. in src/share/vm/memory/collectorPolicy.cpp
>> a minor bug in initializing an AdaptiveSizePolicy instance is caught.
>> MaxGCMinorPauseMillis -> MaxGCPauseMillis
>>
>> testing:
>> refworkload test cases: jetstream, scimark, specjbb2000, specjbb2005, 
>> specjvm98
>> GC options: -XX:+UseParallelGC -Xmx512m
>> no regression found from the performance results;
>> PrintGCStats and CompareGCStats show no abnormal variations.
>>
>