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

[David Holmes]

On 22/05/2020 6:23 pm, Kim Barrett wrote:
>> 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.

I still don't see any distinction there with regards to why we use 
volatile or equivalently 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.

The current convention allows for naked loads and stores on volatile 
variables. When we only have Atomic load/store plus other atomic ops 
then the "convention" no longer requires the variable to be volatile. 
Which in turn would I assume undo the initializer-list argument.

David
-----

> 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.
> 
>