RFR: 8247281: migrate ObjectMonitor::_object to OopStorage

[Coleen Phillimore]

On Fri, 11 Sep 2020 18:45:28 GMT, Daniel D. Daugherty <dcubed at openjdk.org> 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.

Changes requested by coleenp (Reviewer).

src/hotspot/share/prims/jvmtiTagMap.cpp line 3021:

> 3019:
> 3020:   // Inflated monitors
> 3021:   blk.set_kind(JVMTI_HEAP_REFERENCE_MONITOR);

So we don't have to provide the equivalent of JVMTI_HEAP_REFERENCE_MONITOR?

src/hotspot/share/runtime/objectMonitor.cpp line 246:

> 244: // Check that object() and set_object() are called from the right context:
> 245: static void check_object_context() {
> 246:   Thread *self = Thread::current();

Nit * is in the wrong place.

src/hotspot/share/runtime/objectMonitor.cpp line 251:

> 249:   guarantee(self->is_Java_thread() || self->is_VM_thread(), "must be");
> 250:   if (self->is_Java_thread()) {
> 251:     JavaThread* jt = (JavaThread*)self;

With David's new change this should use as_Java_thread().

src/hotspot/share/runtime/objectMonitor.cpp line 268:

> 266:     return NULL;
> 267:   }
> 268:   return _object.resolve();

Why would _object be NULL?  It should be non-null after creation.  It might point to null but then _object.resolve()
would return NULL.  This NULL check doesn't make sense to me, same with the peek function below.

src/hotspot/share/services/heapDumper.cpp line 1395:

> 1393:   void do_oop(oop* obj_p) {
> 1394:     u4 size = 1 + sizeof(address);
> 1395:     writer()->start_sub_record(HPROF_GC_ROOT_MONITOR_USED, size);

I had a similar question to the jvmtiTagMap question above.  Are there tools that are going to miss seeing this tag in
the heap dump?  I hope these tags are implementation defined and we can just remove them.  Otherwise, should there be a
loop through the OM list to print out these tags for live object monitors?

src/hotspot/share/runtime/synchronizer.cpp line 1548:

> 1546:                                     bool from_per_thread_alloc) {
> 1547:   guarantee(m->header().value() == 0, "invariant");
> 1548:   guarantee(m->object_peek() == NULL, "invariant");

Because of type stable memory, you don't release the WeakHandle when the OM is released.  The oop inside the WeakHandle
is replaced in when an OM is reused?

src/hotspot/share/runtime/synchronizer.cpp line 163:

> 161:
> 162: #define CHAINMARKER (cast_to_oop<intptr_t>(-1))
> 163:

Great. One less non-oop oop!

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

PR: https://git.openjdk.java.net/jdk/pull/135