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

[David Holmes]

Hi Kim,

On 24/05/2020 2:49 am, Kim Barrett wrote:
>> 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.

Okay this is really nothing to do with Atomic::store at all. using the 
initializer list is the obviously correct and simple way to do this.

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

Okay, but I still don't see why we would not remove the volatile 
qualifier for these oop cases.

Thanks,
David

>