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

[David Holmes]

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?

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

David