RFR: 8252406: Introduce Thread::as_Java_thread() convenience function [v5]
David Holmes
david.holmes at oracle.com
Wed Sep 9 12:50:18 UTC 2020
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.
>
>
More information about the serviceability-dev
mailing list