RFR: 8024137 - Flags should be set using the proper macro

[Jon Masamitsu]

On 7/12/2016 3:21 AM, Jesper Wilhelmsson wrote:
> Den 12/7/16 kl. 05:54, skrev Jon Masamitsu:
>>
>>
>> On 7/11/2016 3:14 PM, Jesper Wilhelmsson wrote:
>>> Hi Jon,
>>>
>>> Den 11/7/16 kl. 20:47, skrev Jon Masamitsu:
>>>> Jesper,
>>>>
>>>> http://cr.openjdk.java.net/~jwilhelm/8024137/webrev.00/src/share/vm/gc/shared/collectorPolicy.cpp.frames.html 
>>>>
>>>>
>>>>
>>>>
>>>>  361   if (OldSize < old_gen_size_lower_bound()) {
>>>>  362     FLAG_SET_ERGO(size_t, OldSize, old_gen_size_lower_bound());
>>>>  363   }
>>>>  364   if (!is_size_aligned(OldSize, _gen_alignment)) {
>>>>  365     FLAG_SET_ERGO(size_t, OldSize, align_size_down(OldSize,
>>>> _gen_alignment));
>>>>
>>>>
>>>> If OldSize has to be adjusted for both lower bound and alignment,
>>>> what gets saved as the previous value?
>>>
>>> There is no difference in what is saved as the previous value due to 
>>> this
>>> change as far as I can see.
>>
>> I understand that to mean that the previous value saved by the first 
>> use of
>> FLAG_SET_ERGO
>> is not changed when FLAG_SET_ERGO is used a second time.   In this 
>> case at 362
>> the value set
>> on the command line is saved and the minimum adjusted value is set in 
>> OldSize.
>> Then at
>> 365 the previous value is still the value set on the command line and 
>> OldSize is
>> set to an aligned value.
>>
>> If that is so, then the change looks good.
>
> I assume you are referring to the tracing framework's tracking of flag 
> changes? This tracks all changes, not just the last one. But since the 
> old code did not use FLAG_SET_ERGO no change was tracked at all in 
> this case, no old value was saved. Now we track both changes and store 
> both old values in an event each.
>
> In the cases where the old code would change OldSize later using 
> FLAG_SET_ERGO (for instance in line 395) the old value stored could be 
> the result of alignment etc and would appear to come out of thin air.
>
> So my statement that there is no difference was not telling the whole 
> truth, but this is a change to the better :)
>
>
> I found another case where we could clean up the code. The variable 
> _max_heap_size_cmdline in CollectorPolicy can be removed since we 
> already know if the flag was set on the command line or not.
>
>
> There is a new webrev with this change included here:
> http://cr.openjdk.java.net/~jwilhelm/8024137/webrev.02/

Thanks for the explanation.

All changes look good.

Jon

>
> Thanks,
> /Jesper
>
>>
>> Reviewed.
>>
>> Jon
>>
>>> /Jesper
>>>
>>>>
>>>> Jon
>>>>
>>>> On 06/21/2016 01:01 PM, Jesper Wilhelmsson wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this change to make the GC ergonomics use 
>>>>> FLAG_SET_ERGO()
>>>>> instead of setting values directly in flags. This change builds on 
>>>>> the fix for
>>>>> JDK-8048093 and assumes that we will remember that the flags was 
>>>>> set on the
>>>>> command line.
>>>>>
>>>>> If you know of any other places where we assign a flag directly 
>>>>> without using
>>>>> FLAG_SET_ERGO() please let me know since it would make sense to 
>>>>> change all
>>>>> these places in this change.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8024137
>>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8024137/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> /Jesper
>>>>>
>>>>
>>