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

[Daniel D.Daugherty]

On Wed, 3 Mar 2021 23:08:06 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> ObjectMonitors can only be used by JavaThreads (modulo some interactions with hashcodes and deflation) but we use "Thread*" almost everywhere mainly due to use of TRAPS (and TRAPS will itself use JavaThread once JDK-8252685 is done). Also some uses of TRAPS in the API's are wrong as, for example, monitor entry can never throw an exception.
>> 
>> So this cleanup tackles:
>> - remove incorrect use of TRAPS
>> - change "Thread*" to "JavaThread*" where applicable
>> - don't use THREAD for things not related to exception processing
>> - standardise the naming so that we have "JavaThread* current" rather a mix if Self/THREAD/jt etc.
>> - remove unnecessary as_Java_thread() conversions
>> - other misc cleanup I noticed in some functions
>> 
>> The cleanup is predominantly in objectMonitor.* and synchronizer.* but with a fan out to the users of those APIs. No attempt is made to cleanup the callers beyond ensuring we have a suitable JavaThread reference for the calls. 
>> 
>> Thanks,
>> David
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   More pointer declaration style fixups

I think I only have nits and typos.
Thanks for doing this massive cleanup!

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

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.

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?

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

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/

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.

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/

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?

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

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

Marked as reviewed by dcubed (Reviewer).

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