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

Thu Mar 6 09:59:08 UTC 2014

Thanks Jon and Jesper for reviewing.

Stefan

On 2014-03-06 10:34, Jesper Wilhelmsson wrote:
> 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.
>>>>>>> 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 tI 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.
>>>>> 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 tIt'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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>