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

[David Holmes]

Trimming ...

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

I personally think that the use of JavaThread::current() makes a clear 
statement that a JavaThread is expected here, but I have reverted the 
change.

Thanks,
David
-----

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