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

[Jon Masamitsu]

On 01/15/13 11:57, Erik Helin wrote:
> Jesper,
>
> 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.
>
> I saw that you updated to your webrev directly after sending my email. 
> I'm think that our changes should be consistent, and therefore I would 
> prefer option 2 (putting the code in CollectorPolicy::initialize_flags).
>
> Jon, what do you think?

Yes to option 2.

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