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

[Erik Helin]

John,

On 01/16/2013 01:07 AM, Jon Masamitsu wrote:
> Would you consider fixing G1CollectorPolicy
> to derive from GenCollectorPolicy so that you could
> add the code to GenCollectorPolicy::initialize_flags()?

I agree that the CollectorPolicy inheritance hierarchy needs to be 
fixed, but I don't think that it should be part of this change.

I can file an RFE and see if I can get some time work on it.

Thanks,
Erik

>
> Thanks.
>
> Jon
>
>
> On 01/15/13 06:45, 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
>>