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

[Stefan Johansson]

Thanks Jon,

On 2014-03-05 18:45, Jon Masamitsu wrote:
> 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.
>
I took inspiration from metaspace.cpp where we also skip to use 
FLAG_SET_ERGO:
// Do not use FLAG_SET_ERGO to update NewSize here, since this will override
// if NewSize was set on the command line or not. This information is needed
// later when setting the initial and minimum young generation size.

I'll add this right before the assignment, should I prepare another webrev?

Cheers,
Stefan
> 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
>>>>>
>>>>
>>>
>>
>