Hi Coleen,
On 17/07/2020 1:01 am, [email protected] 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
