Hi Kim,
On 9/09/2020 1:30 pm, Kim Barrett wrote:
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.
Thank you.
Note that I've been using the version as given in the email subject
(i.e. v5 here) not the "range" value (04 here) listed in the webrev
links. Sorry for any confusion.
------------------------------------------------------------------------------
src/hotspot/share/classfile/systemDictionary.cpp
The block from line 1646-1655 seems to be misindented. And was before too.
Fixed.
------------------------------------------------------------------------------
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.]
Reverted.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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. :(
[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!
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
------------------------------------------------------------------------------
src/hotspot/share/services/management.cpp
2074 if (thread_id == 0) { // current thread
2075 return thread->cooked_allocated_bytes();
Indentation got messed up.
Fixed. (I must remember to try the emacs fix to handle the JVM_ENTRY and
other macros.)
------------------------------------------------------------------------------
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.
Yep - that same RFE:
https://bugs.openjdk.java.net/browse/JDK-8252685
Thanks,
David
-----
------------------------------------------------------------------------------
