RFR 8247878: Move Management strong oops to OopStorage

[Kim Barrett]

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

------------------------------------------------------------------------------
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());

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

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?

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.

------------------------------------------------------------------------------