RFR (S): 8005989: The default value for MaxNewSize should be less than or equal to MaxHeapSize

[Jesper Wilhelmsson]

Erik,

I would prefer solution number two for the same reason that I moved my 
old gen size fix out of arguments.cpp, the code in Arguments don't know 
about generations and if we can avoid adding that knowledge I think we 
should avoid it. CollectorPolicy already know about generations and I 
think it is the right place for this code.
/Jesper


On 15/1/13 3:45 PM, Erik Helin wrote:
> Jon,
>
> On 01/14/2013 09:41 PM, Jon Masamitsu wrote:
>> Change looks good.
>
> Thanks!
>
> On 01/14/2013 09:41 PM, Jon Masamitsu wrote:
>> Do you think it would
>> fit better in TwoGenCollectorPolicy::initialize_flags()
>> or maybe GenCollectorPolicy::initialize_flags()?  You'd have to
>> add it also to  G1CollectorPolicy::initialize_flags()
>> or do some refactoring of the various initialize_flags().
>
> I don't want to duplicate the code by adding it to both 
> G1CollectorPolicy and GenCollectorPolicy, but I agree that 
> Arguments::set_heap_size might not be the best place for it.
>
> I've tried a few different approaches, and also discussed with Bengt 
> and Jesper, and I think that one of the following approaches is the 
> way to go:
>
> 1. Add a new function, Arguments::set_heap_generation_sizes(), that
>    sets MaxNewSize. Jesper could add his code for OldSize in this
>    function as well.
>
>    Webrev: http://cr.openjdk.java.net/~ehelin/8005989/webrev.01/
>
> 2. Add the code to CollectorPolicy::initialize_flags().
>
>    Webrev: http://cr.openjdk.java.net/~ehelin/8005989/webrev.02/
>
> Both options have drawbacks:
>
> It does not feel right to add a function to arguments.cpp that handles 
> GC specific flags. On the other hand, this has already been done quite 
> extensively, but that is not an excuse for continue doing it.
>
> Adding the code to CollectorPolicy breaks the abstraction that 
> CollectorPolicy is not supposed to know about generational heap 
> collectors (although this has already happened, see 
> CollectorPolicy::initialize_size_info).
>
> Out of these two, I would prefer the first option (adding a new 
> function in arguments.cpp) and I think both options are better than 
> duplicating the code.
>
> Which option do you prefer? Or would you prefer another solution?
>
> Thanks,
> Erik
>
>> Jon
>>
>>
>> On 01/12/13 00:27, Erik Helin wrote:
>>> Hi all,
>>>
>>> this change sets MaxNewSize to always be less than or equal to
>>> MaxHeapSize. The current default value is max_uintx.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~ehelin/8005989/webrev.00/
>>>
>>> RFE:
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005989
>>>
>>> Analysis:
>>> The change should not affect the existing code. The case that has to
>>> be analyzed is when MaxNewSize is not set on the command line, since
>>> the update of MaxNewSize is guarded by "if
>>> (FLAG_IS_DEFAULT(MaxNewSize))".
>>>
>>> MaxNewSize is only used in the following files:
>>>
>>> - g1CollectorPolicy.cpp:
>>>   Guards all the usage of MaxNewSize in if statements:
>>>
>>>      if (FLAG_IS_CMDLINE(MaxNewSize)) {
>>>         ...
>>>      }
>>>
>>>   Since this change uses FLAG_SET_DEFAULT, this change won't affect
>>>   this code path.
>>>
>>> - arguments.cpp:
>>>   Only reads MaxNewSize in FLAG_IS_DEFAULT(MaxNewSize) statements.
>>>   Otherwise, only writes new values to MaxNewSize that are not based on
>>>   the value of MaxNewSize. This code is not affected since the value of
>>>   FLAG_IS_DEFAULT(MaxNewSize) is not affected.
>>>
>>> - collectorPolicy.cpp:
>>>   MaxNewSize is used in a couple of places:
>>>
>>>     if (NewSize > MaxNewSize) {
>>>       MaxNewSize = NewSize;
>>>     }
>>>
>>>   If both NewSize and MaxNewSize have their default values, then
>>>   NewSize is ScaleForWordSize(1*M) which will always be less than
>>>   MaxNewSize which now is MaxHeapSize. The semantics of the if
>>>   statement is only changed if NewSize is set on the command line to a
>>>   value larger than MaxHeapSize, which is not a valid case.
>>>
>>>   if (FLAG_IS_CMDLINE(MaxNewSize) || FLAG_IS_ERGO(MaxNewSize) {
>>>     ...
>>>   } else {
>>>     max_new_size = scale_by_NewRatio_aligned(max_heap_byte_size());
>>>     max_new_size = MIN2(MAX2(max_new_size, NewSize), MaxNewSize);
>>>   }
>>>
>>>   This "if" check will still follow the "else" branch in the default
>>>   case, just as before, since FLAG_SET_DEFAULT is used in this change.
>>>   The semantics of the calculation in the else branch is also 
>>> preserved,
>>>   since MAX2(max_new_size, NewSize) will still always be less than
>>>   MaxNewSize for all valid values of NewSize.
>>>
>>> Testing:
>>> Before the change:
>>>   java -XX:+PrintFlagsFinal -version | grep 'MaxHeapSize \| MaxNewSize'
>>>       uintx MaxHeapSize := 2017460224 {product}
>>>       uintx MaxNewSize   = 18446744073709486080 {product}
>>>
>>> After the change:
>>>   java -XX:+PrintFlagsFinal -version | grep 'MaxHeapSize \| MaxNewSize'
>>>       uintx MaxHeapSize := 2017460224 {product}
>>>       uintx MaxNewSize   = 2015428608 {product}
>>>
>>> JPRT
>>>
>>> Thanks,
>>> Erik
>