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

[Daniel D.Daugherty]

On Wed, 3 Mar 2021 23:03:01 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 two additional commits since the last revision:
> 
>  - ObjectMonitor::exit can't actually throw IMSE so replaced TRAPS with JavaThread* current
>  - Style fix: Thread *  -> Thread* in type declarations

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

> 1353: //
> 1354: // complete_exit exits a lock returning recursion count
> 1355: // complete_exit/reenter operate as a wait without waiting

Thanks for detecting this stale comment and deleting it.
Also thanks for retiring ExitSuspendEquivalent().

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

> 1400:   do {                                                                 \
> 1401:     if (!check_owner(THREAD)) {                                        \
> 1402:        assert(HAS_PENDING_EXCEPTION, "expected a pending IMSE here."); \

Wow! 3 different ways to refer to the same thread in one function: THREAD, Self and jt.
Thanks for cleaning that up.

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

> 314:                                      BasicLock * lock) {
> 315:   assert(current->thread_state() == _thread_in_Java, "invariant");
> 316:   NoSafepointVerifier nsv;

Did you mean to delete this assert()? If so, why?
Update: because the next assert rules out being at a safepoint because you
can't have a thread in _thread_in_Java at a safepoint. Okay...

src/hotspot/share/runtime/synchronizer.hpp line 110:

> 108:   // that needs to wait() on a java-level object but must not respond
> 109:   // to interrupt requests and doesn't timeout.
> 110:   static void wait_uninterruptibly(Handle obj, JavaThread* current);

Ahhh - I see now. You got rid of the 'Millis' parameter.

src/hotspot/share/runtime/synchronizer.hpp line 200:

> 198:   BasicLock   _lock;
> 199:  public:
> 200:   ObjectLocker(Handle obj, JavaThread* current);

So no more non-JavaThread uses of ObjectLocker?

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

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