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

[Jon Masamitsu]

Erik,

I'd prefer option 2) to get it closer to the code that
knows about generations.  I agree that
CollectorPolicy::initialize_size_info() is broken
and duplicating the code would be the worst.

I'm guessing that the current G1CollectorPolicy
doesn't derive from GenCollectorPolicy because
G1 was not exclusively generational until recently.
Would you consider fixing G1CollectorPolicy
to derive from GenCollectorPolicy so that you could
add the code to GenCollectorPolicy::initialize_flags()?
If you're touching code and think it is ugly, I think
you have license to fix it.

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
>