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.
