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.
