RFR: 8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code [v3]

[David Holmes]

Hi again Dan,

(I've given up trying to figure out how PR review emails get split up. :) )

Trimming ...

On 5/03/2021 9:46 am, Daniel D.Daugherty wrote:
> src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 261:
> 
>> 259:   volatile_nonstatic_field(ObjectMonitor,      _cxq,                                          ObjectWaiter*)                         \
>> 260:   volatile_nonstatic_field(ObjectMonitor,      _EntryList,                                    ObjectWaiter*)                         \
>> 261:   volatile_nonstatic_field(ObjectMonitor,      _succ,                                         JavaThread*)                               \
> 
> nit - please fix the indent before the backslash...

Fixed.

> src/hotspot/share/oops/instanceKlass.cpp line 946:
> 
>> 944:     HandleMark hm(THREAD);
>> 945:     Handle h_init_lock(THREAD, init_lock());
>> 946:     ObjectLocker ol(h_init_lock, jt);
> 
> I was going to mumble about adding a new 'jt' here, but this isn't an
> ObjectMonitor related file so you probably held off here.

Yes, and "jt" already exists as the JavaThread manifestation of Thread.

> src/hotspot/share/runtime/objectMonitor.hpp line 49:
> 
>> 47:   ObjectWaiter* volatile _next;
>> 48:   ObjectWaiter* volatile _prev;
>> 49:   JavaThread*   _thread;
> 
> So no more uses of ObjectWaiter by non-JavaThreads?

See other email. AFAIA non-JavaThreads can wait on ObjectMonitors.

> src/hotspot/share/runtime/synchronizer.cpp line 437:
> 
>> 435:       BiasedLocking::revoke(obj, current);
>> 436:     } else {
>> 437:       guarantee(false, "VMThread should not be involved with ObjectMonitor");
> 
> Interesting change. Seems out of context for this change.
> Since you have "guarantee(false," you can use "fatal(" instead...

This is an incomplete change. Earlier I still had TRAPS on enter() even 
though it must be a JavaThread so I changed the BL code fragment to the 
above. But then when I realized enter() never produces an exception, the 
TRAPS became JavaThread* and so it is impossible to be at a safepoint 
(or in the VMThread.). So I've reformulated that block. Thanks for noticing!

> src/hotspot/share/runtime/synchronizer.cpp line 612:
> 
>> 610:   ObjectMonitor* monitor = inflate(current, obj, inflate_cause_jni_exit);
>> 611:   // If this thread has locked the object, exit the monitor. We
>> 612:   // intentionally do not use CHECK on check_owner because we must exit the
> 
> s/CHECK on check_owner/check_owner/

No, the comment is explaining why we do not have this:

   bool owned = monitor->check_owner(CHECK);
   if (owned) {
     monitor->exit(true, current);
   }

the exception we are concerned about is not the one that might be posted 
by check_owner, but any pre-existing pending exception that might be 
present. We must release the monitor regardless.


> src/hotspot/share/runtime/synchronizer.cpp line 678:
> 
>> 676:   // after ownership is regained.
>> 677:   ObjectMonitor* monitor = inflate(current, obj(), inflate_cause_wait);
>> 678:   monitor->wait(0 /* wait-forever */, false /* not interruptible */, current);
> 
> Didn't expect to see this change from "millis" to "0" either.
> Seems out of context for this change.
> 
> Update: I see that you deleted the 'millis' param now. I missed that before.

Yes a "target of opportunity" given the other changes to this method.

> src/hotspot/share/runtime/synchronizer.cpp line 1071:
> 
>> 1069:       // monitors_iterate() is only called at a safepoint or when the
>> 1070:       // target thread is suspended or when the target thread is
>> 1071:       // operating on itcurrent. The current closures in use today are
> 
> typo - s/itcurrent/itself/

Well spotted! (Did that on purpose just to see who was paying attention 
- NOT! :) )

> src/hotspot/share/runtime/synchronizer.hpp line 206:
> 
>> 204:   void wait(TRAPS)  { ObjectSynchronizer::wait(_obj, 0, CHECK); } // wait forever
>> 205:   void notify_all(TRAPS)  { ObjectSynchronizer::notifyall(_obj, CHECK); }
>> 206:   void wait_uninterruptibly(JavaThread* current) { ObjectSynchronizer::wait_uninterruptibly(_obj, current); }
> 
> Any reason for not use 'current_thread' here?

?? I'm using "current" everywhere.

> src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java line 516:
> 
>> 514:     public final int objectMonitorCxq = getFieldOffset("ObjectMonitor::_cxq", Integer.class, "ObjectWaiter*", -1, jdk13Backport);
>> 515:     public final int objectMonitorEntryList = getFieldOffset("ObjectMonitor::_EntryList", Integer.class, "ObjectWaiter*", -1, jdk13Backport);
>> 516:     public final int objectMonitorSucc = getFieldOffset("ObjectMonitor::_succ", Integer.class, JDK < 17 ? "Thread*" : "JavaThread*", -1, jdk13Backport);
> 
> That makes my brain hurt...

Yeah it needs to maintain backward compatibility.

Thanks for the review!

David
-----

> -------------
> 
> Marked as reviewed by dcubed (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/2802
>