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

[Jesper Wilhelmsson]

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