RFR 8247878: Move Management strong oops to OopStorage

[coleen.phillimore at oracle.com]

David, thank you for the review and help.
Coleen

On 7/20/20 12:47 AM, David Holmes wrote:
> Hi Coleen,
>
> On 18/07/2020 7:29 am, coleen.phillimore at oracle.com wrote:
>>
>> I've corrected this change with Kim's and David's feedback, and 
>> retested with tier1-3.  This is much better.
>
> Yes this is better. :) Thanks to Kim for clarifying the 
> acquire/release issue.
>
> LGTM.
>
> Thanks,
> David
> -----
>
>> incremental webrev at 
>> http://cr.openjdk.java.net/~coleenp/2020/8247878.02.incr/webrev
>> full webrev at 
>> http://cr.openjdk.java.net/~coleenp/2020/8247878.02/webrev
>>
>> Thanks,
>> Coleen
>>
>> On 7/17/20 10:50 AM, coleen.phillimore at oracle.com wrote:
>>>
>>> Hi Kim, Thank you for reviewing this.
>>>
>>> On 7/17/20 5:02 AM, Kim Barrett wrote:
>>>>> On Jul 16, 2020, at 11: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
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>> ------------------------------------------------------------------------------ 
>>>>
>>>> src/hotspot/share/oops/oopHandle.inline.hpp
>>>>    50   if (peek() != NULL) {
>>>>
>>>> Adding that seems like an unrelated change, and it's not clear to me
>>>> why this is being done.
>>>
>>> This was to save null checks in all the callers, particularly here:
>>>
>>>  ThreadSnapshot::~ThreadSnapshot() {
>>> + _blocker_object.release(OopStorageSet::vm_global());
>>> + _blocker_object_owner.release(OopStorageSet::vm_global());
>>> + _threadObj.release(OopStorageSet::vm_global());
>>> +
>>>
>>>>
>>>> ------------------------------------------------------------------------------ 
>>>>
>>>> src/hotspot/share/services/lowMemoryDetector.cpp
>>>>   299   if (_sensor_obj.peek() != NULL) {
>>>>   300     InstanceKlass* sensorKlass = 
>>>> Management::sun_management_Sensor_klass(CHECK);
>>>>   301     Handle sensor_h(THREAD, _sensor_obj.resolve());
>>>>
>>>> I see no reason for a peek operation here, and think it just makes the
>>>> code harder to understand.  Just unconditionally resolve the sensor.
>>>>
>>>> Similarly here:
>>>>   364   if (_sensor_obj.peek() != NULL) {
>>>>   365     InstanceKlass* sensorKlass = 
>>>> Management::sun_management_Sensor_klass(CHECK);
>>>>   366     Handle sensor(THREAD, _sensor_obj.resolve());
>>>
>>> I can move the NULL check down after the Handle.  I was mostly 
>>> keeping the existing pattern.
>>>>
>>>> ------------------------------------------------------------------------------ 
>>>>
>>>> src/hotspot/share/services/memoryManager.cpp
>>>>   136       _memory_mgr_obj = 
>>>> AtomicOopHandle(OopStorageSet::vm_global(), mgr_obj);
>>>>
>>>> There is a race here. The handle constructor allocates the oopstorage
>>>> entry and then does a release_store of the value into it. After (in
>>>> source order) the handle is constructed, it is copied into
>>>> _memory_mgr_obj, which is just a copy-assign of the oop* from
>>>> oopstorage. There's nothing to prevent that copy-assign from being
>>>> reordered before the store of the value into the oopstorage, either by
>>>> the compiler or the hardware.
>>>
>>> Ok, I see that.
>>>>
>>>> The _obj in _memory_mgr_obj is being read without locking and may be
>>>> written by another thread, so should itself be appropriate atomic.
>>>> It's not clear what the semantics of this new kind of handle are
>>>> supposed to be, but I think as used for _memory_mgr_obj there are
>>>> problems.
>>>>
>>>> The same is true for _memory_pool_obj.
>>>>
>>>> I think the atomicity here is in the wrong place. The pointee of the
>>>> oop* doesn't need atomic access, the oop* itself does.  At least I
>>>> think so; both _memory_mgr_obj and _memory_pool_obj are lazily
>>>> assigned on first use and never change after that, right?
>>>
>>> Yes.  volatile is in the wrong place.
>>>>
>>>> One way to do that would be that the type of _memory_mgr_obj is `oop*
>>>> volatile`, and is initialized as
>>>>
>>>>    oop* oop_ptr = ... allocate oopstorage entry ...
>>>>    NativeAccess<>::oop_store(oop_ptr, value);
>>>>    Atomic::release_store(&_memory_mgr_obj, oop_ptr);
>>>>
>>>> Alternatively, use
>>>>
>>>>    volatile OopHandle _memory_mgr_obj;
>>>>
>>>>    Atomic::release_store(&_memory_mgr_obj, OopHandle(...));
>>>>
>>>> and teach Atomic how to deal with OopHandle by defining a
>>>> PrimitiveConversions::Translator for it.
>>>>
>>>> The declaration would be
>>>>
>>>>    volatile OopHandle _memory_mgr_obj;
>>>>
>>>> and accesses would be
>>>>
>>>>    Atomic::load_acquire(&_memory_mgr_obj).resolve();
>>>>
>>>> And AtomicOopHandle isn't useful and should be discarded.
>>>
>>> Ok, yes, this is exactly what I want.  And David will be happy 
>>> because he didn't like AtomicOopHandle.
>>>
>>> Thanks for catching this and your help.
>>> Coleen
>>>
>>>>
>>>> ------------------------------------------------------------------------------ 
>>>>
>>>>
>>>
>>