RFR 8247878: Move Management strong oops to OopStorage

[coleen.phillimore at oracle.com]

Hi David, thank you for reviewing this.

On 7/17/20 12:13 AM, David Holmes wrote:
> Hi Coleen,
>
> On 17/07/2020 1:01 am, coleen.phillimore at oracle.com wrote:
>> Summary: Use OopStorage for strong oops stored with memory and thread 
>> sampling and dumping, and remove oops_do and GC calls.
>>
>> These use OopStorageSet::vm_global()  OopStorage for now.  I'll 
>> change the thread sampling oops to use a new OopStorage once the GC 
>> code is changed to nicely allow additional oop storages.  The memory 
>> pool oops are never deleted once created, so they should stay in 
>> vm_global() oop storage.
>>
>> Tested with tiers 1-3 (tiers 4-6 with other changes) and 
>> javax/management tests.  I timed the tests to see if there was any 
>> noticeable performance difference, and there was not.
>>
>> open webrev at 
>> http://cr.openjdk.java.net/~coleenp/2020/8247878.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8247878
>
> Overall the bulk of the changes look good - lots of red! :)
Yup!
>
> I'm unsure about defining AtomicOopHandle. I think the memory ordering 
> operations may be better kept in the code paths that use the 
> OopHandle. The name doesn't convey what this actually does in terms of 
> the release-set and resolve-acquire, and I can't think of a good name 
> that would do so**. If you do want to keep it then I think there 
> should be a non-acquire version of resolve as not all accesses require 
> acquire semantics. Looking at memoryManager.cpp (but the same applies 
> to memoryPool.cpp):

I do want to keep the separate type because it's sort of inconvient to 
declare the underlying oop in the OopHandle as volatile or whatever 
we're going to do to replace volatile.  Plus a separate type allows the 
compiler to give an error if the access rules are broken.  Scattering 
these varying accesses around the vm with reasoning makes the code too 
complicated.  Kim and I came up with the name AtomicOopHandle since it 
uses the same mechanism as the underlying MO_ACQUIRE/MO_RELEASE semantics.
>
>   56 bool MemoryManager::is_manager(instanceHandle mh) const {
>   57   return mh() == _memory_mgr_obj.resolve_acquire();
>   58 }
>
> This doesn't have acquire semantics currently and doesn't need it.
>
>  122       // The lock has done an acquire, so the load can't float 
> above it, but
>  123       // we need to do a load_acquire as above.
>  124       mgr_obj = _memory_mgr_obj.resolve_acquire();

Yes, this used to just have Atomic::load() and not acquire.  I didn't 
really see any reason to optimize and break the consistency in order to 
do a plain resolve().  I could add it, it just seems not useful.
>
> The original code and comment is wrong - this doesn't need acquire as 
> the lock already handles that. If another thread has set the mgr 
> object then it did so holding the lock that the current thread now 
> owns, which means the other thread had to have released the lock 
> first, hence when the current thread acquired the lock, all stores in 
> relation to the mgr object are already guaranteed to be visible.

Yes, true.  I can fix the comment or remove it since using OopHandle, 
there will be a load that cannot float.
>
> ** Perhaps the OopHandle constructor, and resolve method should take 
> an optional memory order Decorator parameter?

We still need to redeclare the underlying oop* though.
>
> ---
>
> Should we introduce a naming convention to help distinguish an oop 
> variable from an oopHandle? You seem to have adopted the convention 
> that _foo is the oopHandle and foo is the oop. But should we use foo_h 
> as we do (sometimes at least) for other handles? (I'm not generally 
> fond of encoding types in variable names but when handles and their 
> contents can be accessed in the same code, it does make things clearer.)

I want to vote no on that.  _foo is the OopHandle because that's how the 
field is declared.  The only place I did this was here:

http://cr.openjdk.java.net/~coleenp/2020/8247878.01/webrev/src/hotspot/share/services/threadService.cpp.udiff.html

I don't think it'll help to have _blocker_object_h distinguishible from 
blocker_object.  I think the code is somewhat clear that the field is 
assigned at the end.

Thanks,
Coleen
>
> Thanks,
> David
> -----
>
>> Thanks,
>> Coleen
>>
>>
>>