RFR: 8252406: Introduce Thread::as_Java_thread() convenience function [v5]

[Kim Barrett]

> On Sep 8, 2020, at 9:27 AM, David Holmes <dholmes at openjdk.java.net> wrote:
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>  This is a simpler approach to use the static_cast. Changes:
>  - Change C-style cast to static_cast and move function definitions so this works.
>  - Use overloading for const and non-const versions of as_Java_thread().
>  - Update copyright years.
> 
>  Re-testing builds in tiers 1-5.
> 
>  Thanks.
> 
> -------------
> 
> Changes:
>  - all: https://git.openjdk.java.net/jdk/pull/37/files
>  - new: https://git.openjdk.java.net/jdk/pull/37/files/202af760..50dd8b38
> 
> Webrevs:
> - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=04
> - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=03-04
> 
>  Stats: 31 lines in 12 files changed: 10 ins; 6 del; 15 mod
>  Patch: https://git.openjdk.java.net/jdk/pull/37.diff
>  Fetch: git fetch https://git.openjdk.java.net/jdk pull/37/head:pull/37
> 
> PR: https://git.openjdk.java.net/jdk/pull/37

I reviewed all of v4, not just the gc parts this time.

------------------------------------------------------------------------------
src/hotspot/share/classfile/systemDictionary.cpp

The block from line 1646-1655 seems to be misindented.  And was before too.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/barrierSet.cpp
[removed]
45   assert(Thread::current()->is_Java_thread(),
46          "Expected main thread to be a JavaThread");

This was an intentional redundancy with the following JavaThread::current(),
to provide a more informative error message.

[Sorry about missing this on my first pass through.]

------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciEnv.cpp
 243 void JVMCIEnv::describe_pending_exception(bool clear) {
 244   JavaThread* THREAD = JavaThread::current();

This change looks suspicious.  The old code used Thread::current() here and
later cast to a JavaThread*.  But that use and cast is conditional, and
might not be executed at all, depending on the result of !is_hotspot().  So
previously if !is_hotspot() then the current thread could be a non-JavaThread.

(At least locally; there could be things being called in the !is_hotspot()
case that also require a current JavaThread.)

It also looks like the THREAD variable could be eliminated and it's one use
changed to JavaThread::current().

------------------------------------------------------------------------------
src/hotspot/share/prims/jvm.cpp
992   assert(THREAD->is_Java_thread(), "must be a JavaThread");

It's not obvious why this assert is being removed.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/concurrentGCBreakpoints.cpp

63 #define assert_Java_thread() \
 64   assert(Thread::current()->is_Java_thread(), "precondition")
 65 
 66 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
 67   assert_Java_thread();
 68   MonitorLocker ml(monitor());

=>

63 void ConcurrentGCBreakpoints::run_to_idle_impl(bool acquiring_control) {
 64   MonitorLocker ml(JavaThread::current(), monitor());

and related later changes in this file.

I prefer the original code, which intentionally made the precondition check
explicit.

[Sorry about missing this on my first pass through.]

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.cpp
1255 bool ObjectMonitor::reenter(intx recursions, TRAPS) {
1256   Thread * const Self = THREAD;
1257   JavaThread * jt = Self->as_Java_thread();

The only use of Self could just be THREAD, which is also used in this
function. And I don't see any references to jt at all here.  Maybe that
should just be an `assert(THREAD->is_Java_thread(), "precondition");`.

Hm, there are other functions in this file that have a peculiar mix of
Self/THREAD usage and bound but unused jt.

Cleaning that up should probably be a separate task; this changeset is
already quite big enough!

------------------------------------------------------------------------------
src/hotspot/share/services/management.cpp
2074 if (thread_id == 0) { // current thread
2075     return thread->cooked_allocated_bytes();

Indentation got messed up.

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

It seems like there are some places where if a function parameter were
JavaThread* rather than Thread* then there wouldn't need to be a call to
as_Java_thread. That is, use the type system to make sure the types are
correct, rather than a debug-only runtime check. But that's something to
look at as a followup.

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