RFR: 8247281: migrate ObjectMonitor::_object to OopStorage

[Kim Barrett]

> On Sep 11, 2020, at 2:55 PM, Daniel D.Daugherty <dcubed at openjdk.java.net> wrote:
> 
> This RFE is to migrate the following field to OopStorage:
> 
> class ObjectMonitor {
> <snip>
>  void* volatile _object; // backward object pointer - strong root
> 
> Unlike the previous patches in this series, there are a lot of collateral
> changes so this is not a trivial review. Sorry for the tedious parts of
> the review. Since Erik and I are both contributors to this patch, we
> would like at least 1 GC team reviewer and 1 Runtime team reviewer.
> 
> This changeset was tested with Mach5 Tier[1-3],4,5,6,7,8 testing
> along with JDK-8252980 and JDK-8252981. I also ran it through my
> inflation stress kit for 48 hours on my Linux-X64 machine.
> 
> -------------
> 
> Commit messages:
> - 8247281: migrate ObjectMonitor::_object to OopStorage
> 
> Changes: https://git.openjdk.java.net/jdk/pull/135/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=135&range=00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8247281
>  Stats: 445 lines in 36 files changed: 108 ins; 234 del; 103 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/135.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/135/head:pull/135
> 
> PR: https://git.openjdk.java.net/jdk/pull/135


------------------------------------------------------------------------------
src/hotspot/share/oops/weakHandle.cpp
36 WeakHandle::WeakHandle(OopStorage* storage, oop obj) :
37   _obj(storage->allocate()) {
38   assert(obj != NULL, "no need to create weak null oop");

Please format this differently so the ctor-init-list is more easily
distinguished from the body.  I don't care that much which of the several
alternatives is used.

------------------------------------------------------------------------------  
src/hotspot/share/runtime/objectMonitor.cpp 
 244 // Check that object() and set_object() are called from the right context:
 245 static void check_object_context() {

This seems like checking we would normally only do in a debug build. Is this
really supposed to be done in product builds too? (It's written to support
that, just wondering if that's really what we want.) Maybe these aren't
called very often so it doesn't matter? I also see that guarantee (rather
than assert) is used a fair amount in this and related code.

------------------------------------------------------------------------------  
src/hotspot/share/runtime/objectMonitor.cpp 
 251     JavaThread* jt = (JavaThread*)self;

Use self->as_Java_thread().

Later: Coleen already commented on this and it's been fixed.

------------------------------------------------------------------------------ 
src/hotspot/share/runtime/objectMonitor.cpp 
 249   guarantee(self->is_Java_thread() || self->is_VM_thread(), "must be");
 250   if (self->is_Java_thread()) {

Maybe instead

  if (self->is_Java_thread()) {
    ...
  } else {
    guarantee(self->is_VM_thread(), "must be");
  }

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.cpp 
Both ObjectMonitor::object() and ObjectMonitor::object_peek() have
 265   if (_object.is_null()) {
 266     return NULL;

Should we really be calling those functions when in such a state? That seems
like it might be a bug in the callers?

OK, I think I see some places where object_peek() might need such protection
because of races, in src/hotspot/share/runtime/synchronizer.cpp.  And
because we don't seem to ever release() the underlying WeakHandles.

514     if (m->object_peek() == NULL) {
703       if (cur_om->object_peek() == NULL) {

But it still seems like it might be a bug to call object() in such a state.

Related, see next comment.

Later: Looks like Coleen questioned this too.  I'm not sure I understand
Erik's response though.  When do we look at monitors that might not be
constructed / initialized?  That seems like a bad thing to do.  Oh, but the
whole creation path for OMs is kind of evil right now.  I see...  And that's
planned to be fixed in later work.  OK.

------------------------------------------------------------------------------
src/hotspot/share/runtime/synchronizer.cpp  
1548   guarantee(m->object_peek() == NULL, "invariant");

Later: But see previous comment.  Some of this might be relevat later though.

object_peek() seemed like the wrong operation here. I thought this was
attempting to verify that the underlying WeakHandle has been released. But
peeking doesn't ensure that.  Oh, but we don't actually release() the
WeakHandle when we "free" an OM.  We're just pushing the OM on the free
list.  Which means the GC will continue to examine the associated OopStorage
entry (and discover it's NULL).  So there's some cost to not releasing the
WeakHandle when putting an OM on the free list.  Of course, it means there
won't be any allocations when taking off the free list; I'm not sure which
way is better.

But this makes me go back and wonder about whether object_peek() should have
the _object.is_null() check.  After creation it seems like that should never
be true.

------------------------------------------------------------------------------
src/hotspot/share/runtime/synchronizer.cpp 

The old code in chk_in_use_entry seems wrong.  It checked for a null
object() and recorded that as an error.  But then it went on and attempted
to use it as if it was not null.  That's been fixed by the change.  However,
the change no longer treats a null as an error.  Probably this is because
it's weak, and so could have become null.  But is that really possible for
an "in use" monitor?

------------------------------------------------------------------------------
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ObjectMonitor.java
 94   public OopHandle object() {
 95     Address objAddr = addr.getAddressAt(objectFieldOffset);
 96     if (objAddr == null) {
 97       return null;
 98     }
 99     return objAddr.getOopHandleAt(0);

How about something a little bit less abstraction smashing?

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