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

[Jesper Wilhelmsson]

I'm fine with this change for now. Ship it!

I do think we need to fix the flags asap, in 9 the latest. It would be fairly 
easy to change the code to remember all states for a flag, so that if a flag is 
set on the command line and later changed by the ergonomics, it would reply true 
to both FLAG_IS_CMDLINE and FLAG_IS_ERGO. There are a couple of cases that would 
be greatly simplified if we make that change.

Thanks,
/Jesper


Jon Masamitsu skrev 5/3/14 21:57:
>
> On 3/5/2014 10:01 AM, Stefan Johansson wrote:
>> 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?
>
> I don't need another webrev.
>
> Jon
>>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>