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

[Kim Barrett]

Hi David.  Thanks for clarifying some bits I was confused abut.

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

I see; good.

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

Right.  I keep forgetting which of these macros does what with which kinds of threads.

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

I'm not that concerned by a little redundant work in a debug build. I think
the explicit assert is clearer here. It removes the question for the future
reader of why it's asking for a JavaThread for the mutex locker, when any
thread can be used there. It's not obvious that the reason is to get the
associated assertion.

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

Good.