RFR: 8035057: NewSize ergonomics wrong when setting small or unaligned size on command line

[Jon Masamitsu]

Stefan,

Change looks good.

Could you add a comment explaining why you are doing a direct
assignment to NewSize?  I can easily see someone in the future
looking at that assignment and  thinking it is not right.

Thanks.

Jon

On 3/5/2014 9:34 AM, Stefan Johansson wrote:
> On 2014-03-05 16:59, Jon Masamitsu wrote:
>>
>> On 3/5/2014 7:33 AM, Stefan Johansson wrote:
>>> On 2014-03-05 15:58, Jon Masamitsu wrote:
>>>>
>>>> On 3/4/2014 4:12 PM, Stefan Johansson wrote:
>>>>> Jon,
>>>>>
>>>>> Thanks for looking at this, see inline.
>>>>>
>>>>> On March 4, 2014 8:54:37 PM CET, Jon Masamitsu 
>>>>> <jon.masamitsu at oracle.com> wrote:
>>>>>> Stefan,
>>>>>>
>>>>>> I got side tracked on this review.  I thought that the macro
>>>>>> FLAG_IS_CMDLINE would remember if the flag was sent on
>>>>>> the command line.  Unfortunately, it does not.
>>>>>>
>>>>>> Is the problem that NewSize is set somewhere with
>>>>>> FLAG_SET_ERGO when it should not have been?
>>>>>> That is, if it has been set on the command line,
>>>>>> it should not be reset ergonomically.
>>>>>>
>>>>> The problem here is when the size set on the command line is 
>>>>> unaligned, then we need to change it. We could reset it using 
>>>>> FLAG_SET_CMDLINE, but then we lose the information that we change 
>>>>> it. The best would of course be if the flags could remember all 
>>>>> their states, but that's more like a feature than a simple bug fix.
>>>>
>>>> It doesn't feel right that we're saving that a flag was set on the
>>>> command line in CollectorPolicy.   Also, I usually think of
>>>> FLAG_SET_ERGO as setting a flag because of an ergonomic
>>>> policy.  That a size has to be correctly aligned holds for however
>>>> the size was originally set.  I'm tempted to just do a
>>>> naked assignment for fixing the alignment.  Unless your
>>>> interested in fixing the Flags (globals.hpp) so that it
>>>> remembers that it was set on the command line.
>>> I don't feel that fixing the Flags is something I have time for 
>>> right now, so I think that will have to wait.
>>>
>>> I agree that it feels strange to store that information in the 
>>> CollectorPolicy, but we do the same for the max heap size:
>>> // Needed to keep information if MaxHeapSize was set on the command 
>>> line
>>> // when the flag value is aligned etc by ergonomics.
>>> bool _max_heap_size_cmdline;
>>
>> Yes, but having do it a second time seems to emphasize
>> that Flags is broken.  How many more times will we have
>> to do this?
>>
> Agreed. Hopefully we'll find time in the JDK9 time frame to fix the 
> flags.
>>>
>>> The reason I would like to use FLAG_SET_ERGO is that it is possible 
>>> to look at the flags, their values and how they were set through 
>>> JMC. It might be confusing for a user if they've set NewSize on the 
>>> command line to one value, but JMC reports that it was set to an 
>>> other value on the command line.
>>>
>>> Makes sense?
>>
>> I see the problem but I still don't like this fix.   My preferences
>> would be to either
>>
>> 1) fix Flags
>> 2) set NewSize without changing the Flags and document the possible JMC
>> confusion
> Of your suggested solutions I like this one best, it is what I 
> initially had in mind before starting to think about how the flags are 
> presented to users. Here is the updated webrev:
> http://cr.openjdk.java.net/~sjohanss/8035057/webrev.01
>
> Jesper, are you fine with this change as well?
>
> I will also do some additional testing tomorrow to verify that this 
> doesn't break anything else.
>
>> 3) leave it until Flags can be fixed
>>
>> It's a P5.   We don't have to fix it if we're creating technical debt in
>> the fix.
> It's a P5 but part of the zero-failure project so a fix sooner rather 
> than later would be nice.
>
> Thanks for all you input on this Jon,
> Stefan
>>
>> Jon
>>
>>>
>>> Stefan
>>>>
>>>> Jon
>>>>
>>>>>
>>>>> Stefan
>>>>>
>>>>>> Jon
>>>>>>
>>>>>> On 2/20/14 8:52 AM, Stefan Johansson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Can I please have some reviews for this small fix for:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8035057
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~sjohanss/8035057/
>>>>>>>
>>>>>>> Summary:
>>>>>>> When fixing JDK-8033426, the case when NewSize is set on the 
>>>>>>> command
>>>>>>> line but not aligned was not handled. This small change makes sure
>>>>>>> that if NewSize is given on the command line we will align the 
>>>>>>> value
>>>>>>> and use it as both initial and minimum young generation size.
>>>>>>>
>>>>>>> Testing:
>>>>>>> * Build on JPRT.
>>>>>>> * Manual sanity testing.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Stefan
>>>>
>>>
>>
>