RFR 8247878: Move Management strong oops to OopStorage

[David Holmes]

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! :)

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):

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

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.

** Perhaps the OopHandle constructor, and resolve method should take an 
optional memory order Decorator parameter?

---

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

Thanks,
David
-----

> Thanks,
> Coleen
> 
> 
>