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

[Kim Barrett]

> On May 22, 2020, at 9:59 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> 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:
>>> 
>>> On 22/05/2020 8:31 am, Kim Barrett wrote:
>>>> 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.

Here's an example.

A sufficient smaller change to the MemoryManager constructor would have been

  41 MemoryManager::MemoryManager(const char* name) : _name(name) {
  42   _num_pools = 0;
- 43   (void)const_cast<instanceOop&>(_memory_mgr_obj = instanceOop(NULL));
+ 43   Atomic::store(&_memory_manager, instanceOop(NULL));
  44 }

The old code doesn't compile without the volatile qualified oop::operator=.

A different, but not good (as in invokes UB), way to address the compilation failure 

  const_cast<instanceOop&>(_memory_mgr_obj) = instanceOop(NULL);

Instead, I changed it to

  41 MemoryManager::MemoryManager(const char* name) :
  42   _num_pools(0), _name(name), _memory_mgr_obj() {}

No Atomic store needed here.

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

(See also my reply to Coleen in this thread.)

The convention of volatile qualifying variables that we want to treat
"atomically" only allows naked loads and stores for fundamental types,
not for class types. The volatile qualifier for a member of type oop
uses the compiler to help keep us honest, because naked loads and
stores won't compile. (The same thing was done for markWord; JDK-8229838.)