RFR: 8247524: Remove unnecessary volatile qualifiers from member functions

[David Holmes]

On 15/06/2020 6:56 pm, Kim Barrett wrote:
>> On Jun 15, 2020, at 2:59 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> On 15/06/2020 9:15 am, Kim Barrett wrote:
>>> Please review this change to oopDesc and Metadata classes, removing
>>> unnecessary volatile qualifiers from some member functions.
>>> This change also makes virtual overrides of the relevant functions
>>> explicitly virtual.
>>> While there, removed unnecessary workaround in oopDesc::klass_or_null_acquire.
>>> After this change,
>>>    egrep -r -H --exclude-dir=.hg "\)\s*(const |)\s*volatile" src/hotspot
>>> finds nothing; there seem to be no remaining volatile qualified methods.
>>
>> I must admit I'm quite confused as to why anyone would ever have placed a volatile modifier on a method that returns a constant true or false ??? What could it possibly mean?
> 
> The only thing I can think of is that these were called from other member functions that
> were themselves volatile qualified.  But there aren’t any of those anymore...

Okay. Good to see this weirdness gone!

>> On the other hand for a mutable field e.g.:
>>
>> size_t size() const volatile { return _word_size; }
>>
>> I can see an expectation that the volatile would force the compiler to always reload _word_size, even if the field itself was not declared volatile (which it isn't). Isn't this how we are now handling things via Atomic::load - where the field is no longer flagged volatile but the load is so that we still tell the compiler not to make assumptions about potentially caching the field?
> 
> The “preferred" way to do that today would be to make the member volatile and use Atomic
> accesses (possibly relaxed if no ordering is needed).  Or just rely on the volatile qualified
> member (though that’s no longer “preferred”).  (And AtomicValue<> might be used in the
> future, instead of a volatile member.)

So I'm reminded that there is currently disagreement about what the new 
"preferred way" for this, but lets leave that for the AtomicValue 
discussion. :)

> But in this case, none of that matters.  The _word_size is initialized by the constructor and
> is never modified - there are no calls to set_size.  I should have removed set_size but
> forgot; I’ll fix that now.  And it looks like there are a whole bunch of unused or seemingly
> useless functions in Metabase.

I saw set_size, hence my query, but didn't look that it was actually used :)

I'm fine with the current set of changes.

Thanks,
David
-----

>>
>> Thanks,
>> David
>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8247524
>>> Webrev:
>>> https://cr.openjdk.java.net/~kbarrett/8247524/open.00/
>>> Testing:
>>> mach5 tier1-3
> 
>