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

[Jesper Wilhelmsson]

Thanks Jon!
/Jesper

Den 12/7/16 kl. 20:46, skrev 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
>>>>>>
>>>>>
>>>
>