RFR: 8245594: Remove volatile-qualified member functions and parameters from oop class

[Kim Barrett]

> On May 21, 2020, at 10:39 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> Not following the details 100% but the general gist seems okay, except for a couple of queries below.
> 
> On 22/05/2020 8:31 am, Kim Barrett wrote:
>> Please review this change to eliminate volatile qualification in the
>> member functions and parameters of the oop class and its subclasses.
>> Removed volatile qualified member functions and parameters from class
>> oop and its subclasses (instanceOop, arrayOop, objArrayOop, typeArrayOop).
>> Changed the few remaining volatile oop accesses to instead use relaxed
>> atomics.
>> Changed constructors for MemoryManager and MemoryPool to use
>> initializer list, so we don't need to use Atomic::store to initialize
>> an oop-derived member.
> 
> What is the connection between using an initializer-list and not needing the Atomic::Store?

The volatile oop-derived member (instanceOop in both cases) needs to
be initialized. If done in an initializer list, it's at construction
time for the member object, where volatile isn't relevant. If done
later (for example, in the containing object's constructor body, the
member is now volatile and so involved a volatile assignment, which
would now need to be an Atomic::store.

>> Eliminated ShenandoahVerifierTask's volatile-qualified assignment
>> operator, which isn't used.
>> Some tidying up oop class. Cleaned up constructors, eliminating public
>> set_obj(). Use const-ref rather than by-value parameters for comparisons,
>> to avoid unnecessary copies that go through register/deregister.
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8245594
>> Webrev:
>> https://cr.openjdk.java.net/~kbarrett/8245594/open.00/
> 
> In the following the referenced oops still retain their volatile qualifier - should it not now be removed?
> 
> - src/hotspot/share/runtime/thread.hpp
>  _exception_oop is still marked volatile
> - src/hotspot/share/services/memoryManager.hpp
>  _memory_mgr_obj is still marked volatile
> - src/hotspot/share/services/memoryPool.hpp
>  _memory_pool_obj is still marked volatile

Access to these objects is via Atomic operations, so they should still
be volatile qualified per current convention.

It might be worth examining their use of Atomic.  The MemoryManager
case seems a bit odd, but I haven't studied it carefully.  I haven't
looked at the others yet.  I think doing so is outside the scope of
this change.

> 
> Thanks,
> David
> 
>> Testing:
>> mach5 tier1-5
>> Local (linux-x64) hotspot:tier1 with Shenandoah enabled.