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

[Erik Helin]

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