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

[Jon Masamitsu]

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?

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

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