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

[Kim Barrett]

> On May 23, 2020, at 9:30 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 23/05/2020 5:04 am, Kim Barrett wrote:
>>> 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.
> 
> This is missing the point. If Atomic::store is presumed necessary for some kind of atomicity guarantee, then how does use of an initializer list achieve the same requirement?

No atomicity guarantees are needed until the object is exposed to
other threads than the one constructing the object. (That exposure
needs to deal with atomicity and ordering requirements, of course. We
wouldn't want to expose a partially constructed MemoryManager object
to another thread, for example.) But other than at construction time
for the member subobject, we don't have a way to say that (other than
by casting away the volatile qualification or adding a volatile
qualified assignment operator, neither of which we want). So we would
end up needing to use Atomic::store for an "initialization" outside
the initializer list.

>>>>> 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.)
> 
> Hmmm. Granted "volatile" on a Class type has no meaning. But I always think of oop as really being a pointer type so volatile actually makes some sense there.
> 
> By the same token Atomic::load/store have no meaning on class types either.
> 
> So now I'm very unsure what we are actually doing here ??

Atomic operations have to be "taught" about classes that are thin
wrappers over primitive types, like oop and markWord.  That's done via
PrimitiveConversions::Translate<> specializations.

Atomic::load(const volatile oop* p) is translated into

oop(Atomic::PlatformLoad<sizeof(oop)>()(reinterpret_cast<oopDesc* const volatile*>(p)))

Of course, in the case of oop that's only relevant in the
CHECK_UNHANDLED_OOPS case, where oop is a class wrapper around an
oopDesc*.  In the !CHECK_UNHANDLED_OOPS case, that translation
machinary isn't needed and doesn't apply, because Translate<oop>
is only defined when oop is defined as a class.