RFR 8247878: Move Management strong oops to OopStorage
David Holmes
david.holmes at oracle.com
Mon Jul 20 04:47:25 UTC 2020
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
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>
>
More information about the serviceability-dev
mailing list