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

[Stefan Johansson]

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