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

Jon Masamitsu jon.masamitsu at oracle.com
Wed Mar 5 20:57:05 UTC 2014


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




More information about the hotspot-gc-dev mailing list