RFR: 8247281: migrate ObjectMonitor::_object to OopStorage

[Daniel D.Daugherty]

On Fri, 11 Sep 2020 20:33:11 GMT, Coleen Phillimore <coleenp 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.
>
> 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.

I'll fix that.

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

Yup. Since this is a new function, it didn't pop up as a conflict.
I'll fix that.

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

Correct. See:
+void ObjectMonitor::set_object(oop obj) {
+  check_object_context();
+  if (_object.is_null()) {
+    _object = WeakHandle(_oop_storage, obj);
+  } else {
+    _object.replace(obj);
+  }
+}

So when 'obj' == NULL, we replace the weak handle's value with NULL
and we don't release/delete/whatever the weak handle.

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

Exactly!

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

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