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

[David Holmes]

Hi Kim,

On 9/09/2020 1:30 pm, Kim Barrett wrote:
>> 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.

Thank you.

Note that I've been using the version as given in the email subject 
(i.e. v5 here) not the "range" value (04 here) listed in the webrev 
links. Sorry for any confusion.

> ------------------------------------------------------------------------------
> src/hotspot/share/classfile/systemDictionary.cpp
> 
> The block from line 1646-1655 seems to be misindented.  And was before too.

Fixed.

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

Reverted.

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

Yes - if you look at JNIAccessMark it manifests JavaThread::current() by 
default.

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

Or I can keep it and also use it for the JNIAccessMark constructor.

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

The fact THREAD must be a JavaThread is already established by the 
JVM_ENTRY calls that then invoke this local method.

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

The same precondition is already present in the use of 
JavaThread::current(). Is that not explicit enough? Plus the old code 
will manifest the current thread twice in debug builds. Cleaning up this 
kind of redundancy is one of the key aims here. :(

> 
> [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!

Right, I tried to avoid the temptation of dealing with the 
Self/THREAD/jt mess in this change. I have another RFE filed that will 
do further cleanup here:

https://bugs.openjdk.java.net/browse/JDK-8252685

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

Fixed. (I must remember to try the emacs fix to handle the JVM_ENTRY and 
other macros.)

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

Yep - that same RFE:

https://bugs.openjdk.java.net/browse/JDK-8252685

Thanks,
David
-----

> ------------------------------------------------------------------------------
> 
>