Request for review (S): 7102191 G1: assert(_min_desired_young_length <= initial_region_num) failed: Initial young gen size too small

[Bengt Rutisson]

Hi John,

Thanks for the review!

On 2011-10-26 01:23, John Cuthbertson wrote:
> Hi Bengt,
>
> This looks good to me. One suggestion - replace the body of 
> G1YoungGenSize::initial_young_region_num() with "ShouldNotCallThis();" 
> and add a comment summarizing your explanation below.

I agree that it was unnecessary to remove the 
G1YoungGenSize::initial_young_region_num() method. But rather than 
replacing it with ShouldNotCallThis() I left it as it was before my 
change. This seems good for completeness. We have getters for min and 
max so leaving the inital seems natural to me. I don't think it is wrong 
for anyway to call it if they think they need it, so I'll skip adding 
the ShouldNotCallThis().

Thanks again for the review!

Bengt

>
> Thanks,
>
> JohnC
>
> On 10/24/2011 1:54 PM, Bengt Rutisson wrote:
>>
>> Hi all,
>>
>> Could I have a couple of reviews for this small change?
>> http://cr.openjdk.java.net/~brutisso/7102191/webrev.01/
>>
>> Background
>> When we use the NewRatio flag we set the min and max values for the 
>> young gen to the same value. We do this just after we have let the GC 
>> policy calculate min, initial and max values. However, if NewRatio is 
>> being used we only care about the min and max values. Thus, the 
>> update_young_list_size_using_newratio() method does not update any 
>> initial value.
>>
>> This is what triggers the assert. In G1CollectorPolicy::init() first 
>> calculates the "default" min, initial and max values, then we detect 
>> that NewRatio is being used and we update min and max, but forget 
>> about initial.
>>
>> Looking a bit closer at the calculations it turns out that the 
>> initial value is not needed. We only use it when we have a fixed 
>> young gen. In this case min == initial == max per definition. So, 
>> either min or max can be used instead of initial.
>>
>> The fix is to delete the initial value calculation and the local 
>> variable.
>>
>> CR:
>> 7102191 G1: assert(_min_desired_young_length <= initial_region_num) 
>> failed: Initial young gen size too small
>> http://monaco.sfbay.sun.com/detail.jsf?cr=7102191
>>
>> Thanks,
>> Bengt