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

[Tony Printezis]

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