Request for review (S): 7113021 G1: automatically enable young gen size auto-tuning when -Xms==-Xmx

[Bengt Rutisson]

Hi John,

Thanks for the review!

Some comments inline.

On 2011-12-21 19:04, John Cuthbertson wrote:
> Hi Bengt,
>
> Looks good except for the following couple of items:
>
> g1_globals.hpp:
>
>  294   develop(uintx, G1DefaultMinNewGenPercent, 
> 20,                             \
>  295           "Percentage (0-100) of the heap size to use as 
> minunimum "        \
>  296           "young gen 
> size.")                                                \
>
> Should be "minimum".
>
>  298   develop(uintx, G1DefaultMaxNewGenPercent, 
> 50,                             \
>  299           "Percentage (0-100) of the heap size to use as 
> minunimum "        \
>  300           "young gen size.")
>
> Should be "maximum".

Thanks for detecting this! Fixed it.

> g1CollectorPolicy.[ch]pp:
>
> Can you rename calculate_default_min_size() to be something like 
> "calculate_default_min_region_num()"? Similarly for 
> calculate_default_max_size(). They return a number of regions rather 
> than a size in bytes.

Good point. How about calculate_default_min_length() since the variables 
we set are called *_length and I think we often refer to the young gen 
"size" as the length?

Here is an updated webrev:
http://cr.openjdk.java.net/~brutisso/7113021/webrev.04/

Thanks again for the review!
Bengt

>
> Regards,
>
> JohnC
>
>
> On 12/21/2011 4:22 AM, Bengt Rutisson wrote:
>>
>> Tony,
>>
>> Thanks for the review!
>>
>> Good comments. I fixed them. Here is an updated webrev:
>> http://cr.openjdk.java.net/~brutisso/7113021/webrev.03/
>>
>> Bengt
>>
>> On 2011-12-21 10:21, Tony Printezis wrote:
>>> Bengt,
>>>
>>> First, to repeat what I told you privately the other day: I really 
>>> liked the fact that you introduced the enum for the sizing policy. 
>>> It makes the code much easier to understand. I only have a couple of 
>>> minor comments:
>>>
>>> g1CollectorPolicy.cpp:
>>>
>>> Instead of having the asserts:
>>>
>>>  448   assert(G1DefaultMinNewGenPercent<= G1DefaultMaxNewGenPercent, 
>>> "Min larger than max");
>>>  449   assert(G1DefaultMinNewGenPercent>  0&&  
>>> G1DefaultMinNewGenPercent<  100, "Min out of bounds");
>>>
>>>  456   assert(G1DefaultMinNewGenPercent<= G1DefaultMaxNewGenPercent, 
>>> "Min larger than max");
>>>  457   assert(G1DefaultMaxNewGenPercent>  0&&  
>>> G1DefaultMaxNewGenPercent<  100, "Max out of bounds");
>>>
>>> at the start of calculate_default_{min,max}_size(), why don't you 
>>> just do those checks once in the G1YoungGenSizer constructor (i.e., 
>>> treat them like product params)?
>>>
>>>
>>> g1CollectorPolicy.hpp:
>>>
>>>  100 // If only -XX:NewSize is set we should use the specified value 
>>> as the
>>>  101 // minimum size for young gen. Still using 50% of the heap as 
>>> maximum.
>>>  102 //
>>>  103 // If only -XX:MaxNewSize is set we should use the specified 
>>> value as the
>>>  104 // maximum size for young gen. Still using 20% of the heap as 
>>> minimum.
>>>
>>> Instead of saying 20% and 50%, why don't you just refer to the two 
>>> develop parameters, as those might change in the future?
>>>
>>> Apart from that, looks great! Ship it.
>>>
>>> Tony
>>>
>>>
>>> On 12/19/2011 07:48 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi everyone,
>>>>
>>>> Could I have a couple of reviews for this change?
>>>> http://cr.openjdk.java.net/~brutisso/7113021/webrev.02/
>>>>
>>>> The idea is to give G1 some room to allow it to use its heuristics 
>>>> for calculating the young gen size even when the heap size is fixed.
>>>>
>>>> CR:
>>>> 7113021 G1: automatically enable young gen size auto-tuning when 
>>>> -Xms==-Xmx
>>>> http://monaco.us.oracle.com/detail.jsf?cr=7113021
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7113021
>>>>
>>>> I introduce two new develop flags, G1DefaultMinNewGenPercent and 
>>>> G1DefaultMaxNewGenPercent. I will try to get the performance team 
>>>> to evaluate some good default values for these flags. Right now 
>>>> they are 20 and 50 respectively. This might change if we find that 
>>>> some other values are more appropriate.
>>>>
>>>> Background:
>>>>
>>>> There are three command line options related to the young gen size: 
>>>> NewSize, MaxNewSize and NewRatio (There is also -Xmn, but that is 
>>>> just a short form for NewSize==MaxNewSize). G1 will use its 
>>>> internal heuristics to calculate the actual young gen size, so 
>>>> these options basically only limit the range within which G1 can 
>>>> pick a young gen size. Also, these are general options taking byte 
>>>> sizes. G1 will internally work with a number of regions instead. 
>>>> So, some rounding will occur.
>>>>
>>>> The idea is that my fix should work pretty much as follows:
>>>>
>>>> If nothing related to the the young gen size is set on the command 
>>>> line we should allow the young gen to be between 20% and 50% of the 
>>>> heap size. This means that every time the heap size changes the 
>>>> limits for the young gen size will be updated.
>>>>
>>>> If only -XX:NewSize is set we should use the specified value as the 
>>>> minimum size for young gen. Still using 50% of the heap as maximum.
>>>>
>>>> If only -XX:MaxNewSize is set we should use the specified value as 
>>>> the maximum size for young gen. Still using 20% of the heap as 
>>>> minimum.
>>>>
>>>> If -XX:NewSize and -XX:MaxNewSize are both specified we use these 
>>>> values. No updates when the heap size changes. There is a special 
>>>> case when NewSize==MaxNewSize. This is interpreted as "fixed" and 
>>>> will use a different heuristic for calculating the collection set 
>>>> when we do mixed collection.
>>>>
>>>> If only -XX:NewRatio is set we should use the specified ratio of 
>>>> the heap as both min and max. This will be interpreted as "fixed" 
>>>> just like the NewSize==MaxNewSize case above. But we will update 
>>>> the min and max everytime the heap size changes.
>>>>
>>>> NewSize and MaxNewSize override NewRatio. So, NewRatio is ignored 
>>>> if it is combined with either NewSize or MaxNewSize. (A warning 
>>>> message is printed.)
>>>>
>>>> Thanks,
>>>> Bengt
>>
>