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

[Erik Helin]

John and Jesper,

On 01/16/2013 01:11 AM, Jon Masamitsu wrote:
> On 01/15/13 11:57, Erik Helin wrote:
>> On 2013-01-15 19:25, Jesper Wilhelmsson wrote:
>>> 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.
>>
>> Jon, what do you think?
>
> Yes to option 2.

Ok, then I will commit option 2. Thanks for reviewing this code!

Erik

> Jon
>>
>>> /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
>>>>
>>>