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

[Coleen Phillimore]

On Wed, 3 Mar 2021 05:15:48 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

I think this change looks good.  I made some general comments on specific lines.

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

> 90:   // This is the "slow path" version of monitor enter and exit.
> 91:   static void enter(Handle obj, BasicLock* lock, JavaThread* current);
> 92:   static void exit(oop obj, BasicLock* lock, TRAPS);

So are you suggesting that we should have the convention that if a function takes a Thread/JavaThread argument as the last argument, to use "current" rather than "THREAD", since the latter implies it is supposed to be used to pass an argument for the parameter to TRAPS?  I think this is good.  It manages some confusion about trailing THREAD arguments in some callers.  Specializing further to JavaThread from Thread is also a good change.

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

> 144: 
> 145:   // Deflate idle monitors:
> 146:   static void chk_for_block_req(JavaThread* current, const char* op_name,

I like "current" better than "self" as a convention.

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

Marked as reviewed by coleenp (Reviewer).

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