> On Sep 8, 2020, at 9:27 AM, David Holmes <dhol...@openjdk.java.net> wrote:
> David Holmes has updated the pull request incrementally with one additional
> commit since the last revision:
>
> This is a simpler approach to use the static_cast. Changes:
> - Change C-style cast to static_cast and move function definitions so this
> works.
> - Use overloading for const and non-const versions of as_Java_thread().
> - Update copyright years.
>
> Re-testing builds in tiers 1-5.
>
> Thanks.
>
> -------------
>
> Changes:
> - all: https://git.openjdk.java.net/jdk/pull/37/files
> - new: https://git.openjdk.java.net/jdk/pull/37/files/202af760..50dd8b38
>
> Webrevs:
> - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=04
> - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=37&range=03-04
>
> Stats: 31 lines in 12 files changed: 10 ins; 6 del; 15 mod
> Patch: https://git.openjdk.java.net/jdk/pull/37.diff
> Fetch: git fetch https://git.openjdk.java.net/jdk pull/37/head:pull/37
>
> PR: https://git.openjdk.java.net/jdk/pull/37
I reviewed all of v4, not just the gc parts this time.
------------------------------------------------------------------------------
src/hotspot/share/classfile/systemDictionary.cpp
The block from line 1646-1655 seems to be misindented. And was before too.
------------------------------------------------------------------------------
src/hotspot/share/gc/shared/barrierSet.cpp
[removed]
45 assert(Thread::current()->is_Java_thread(),
46 "Expected main thread to be a JavaThread");
This was an intentional redundancy with the following JavaThread::current(),
to provide a more informative error message.
[Sorry about missing this on my first pass through.]
------------------------------------------------------------------------------
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.)
It also looks like the THREAD variable could be eliminated and it's one use
changed to JavaThread::current().
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
[Sorry about missing this on my first pass through.]
------------------------------------------------------------------------------
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!
------------------------------------------------------------------------------
src/hotspot/share/services/management.cpp
2074 if (thread_id == 0) { // current thread
2075 return thread->cooked_allocated_bytes();
Indentation got messed up.
------------------------------------------------------------------------------
It seems like there are some places where if a function parameter were
JavaThread* rather than Thread* then there wouldn't need to be a call to
as_Java_thread. That is, use the type system to make sure the types are
correct, rather than a debug-only runtime check. But that's something to
look at as a followup.
------------------------------------------------------------------------------
