Hi Coleen,
On 22/07/2020 4:01 am, coleen.phillim...@oracle.com wrote:
This looks like a nice cleanup.
Thanks for looking at this.
http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/src/hotspot/share/runtime/jniHandles.cpp.udiff.html
I'm wondering why you took out the NULL return for make_local() without
a thread argument? Here you may call Thread::current() unnecessarily.
jobject JNIHandles::make_local(oop obj) {
- if (obj == NULL) {
- return NULL; // ignore null handles
- } else {
- Thread* thread = Thread::current();
- assert(oopDesc::is_oop(obj), "not an oop");
- assert(!current_thread_in_native(), "must not be in native");
- return thread->active_handles()->allocate_handle(obj);
- }
+ return make_local(Thread::current(), obj);
}
I was simply using a standard call forwarding pattern to avoid code
duplication. I suspect passing NULL is very rare so the unnecessary
Thread::current() call is not an issue. Otherwise, if not NULL, the NULL
check would happen twice (unless I keep the duplicated implementations).
Beyond the scope of this fix, but it'd be cool to not have a version
that doesn't take thread, since there may be many more callers that
already have Thread::current().
Indeed! And in fact I had missed a number of these in jvm.cpp and
jni.cpp so I have fixed those. I've filed a RFE for other cases:
https://bugs.openjdk.java.net/browse/JDK-8249837
Updated webrev:
http://cr.openjdk.java.net/~dholmes/8249650/webrev.v3/
If this passes tier 1-3 re-testing then I plan to push.
Thanks,
David
-----
Coleen
On 7/20/20 1:53 AM, David Holmes wrote:
Hi Kim,
Thanks for looking at this.
Updated webrev at:
http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/
On 20/07/2020 3:22 pm, Kim Barrett wrote:
On Jul 20, 2020, at 12:16 AM, David Holmes <david.hol...@oracle.com>
wrote:
Subject line got truncated by accident ...
On 20/07/2020 11:06 am, David Holmes wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8249650
webrev: http://cr.openjdk.java.net/~dholmes/8249650/webrev/
This is a simple cleanup that touches files across a number of VM
areas - hence the cross-post.
Whilst working on a different JNI fix I noticed that in most cases
in jni.cpp we were using the following form of make_local:
JNIHandles::make_local(env, obj);
and what that form does is first extract the thread from the JNIEnv:
JavaThread* thread = JavaThread::thread_from_jni_environment(env);
return thread->active_handles()->allocate_handle(obj);
but there is also another, faster, variant for when you already
have the "thread":
jobject JNIHandles::make_local(Thread* thread, oop obj) {
return thread->active_handles()->allocate_handle(obj);
}
When you look at the JNI_ENTRY wrapper (and related JVM_ENTRY,
WB_ENTRY, UNSAFE_ENTRY etc) it has already extracted the thread
from the JNIEnv:
JavaThread* thread=JavaThread::thread_from_jni_environment(env);
and further defined:
Thread* THREAD = thread;
so we always already have direct access to the "thread" available
(or indirect via TRAPS), and in fact we can end up removing the
make_local(JNIEnv* env, oop obj) variant altogether.
Along the way I spotted some related issues with unnecessary use of
Thread::current() when it is already available from TRAPS, and some
other cases where we extracted the JNIEnv from a thread only to
later extract the thread from the JNIEnv.
Testing: tiers 1 - 3
Thanks,
David
-----
------------------------------------------------------------------------------
src/hotspot/share/classfile/javaClasses.cpp
439 JNIEnv *env = thread->jni_environment();
Since env is no longer used on the next line, move this down to where
it is used, at line 444.
Fixed.
------------------------------------------------------------------------------
src/hotspot/share/classfile/verifier.cpp
299 JNIEnv *env = thread->jni_environment();
env now seems to only be used at line 320. Move this closer.
Fixed.
------------------------------------------------------------------------------
src/hotspot/share/prims/jni.cpp
743 result = JNIHandles::make_local(THREAD, result_handle());
jni_PopLocalFrame is now using a mix of "thread" and "THREAD", where
previously it just used "thread". Maybe this change shouldn't be made?
Or can the other uses be changed to THREAD for consistency?
"thread" and "THREAD" are interchangeable for anything expecting a
"Thread*" (and somewhat surprisingly a number of API's that only work
for JavaThreads actually take a Thread*. :( ). I had choice between
trying to be file-wide consistent with the make_local calls, versus
local-code consistent, and used THREAD as it is available in both
JNI_ENTRY and via TRAPS. But I can certainly make a local change to
"thread" for local consistency.
------------------------------------------------------------------------------
src/hotspot/share/prims/jvm.cpp
The calls to JvmtiExport::post_vm_object_alloc have to use "thread"
instead of "THREAD", even though other places nearby are using
"THREAD". That inconsistency is kind of unfortunate, but doesn't seem
easily avoidable.
Everything that uses THREAD in a JVM_ENTRY method can be changed to
use "thread" instead. But I'm not sure it's a consistency worth
pursuing at least as part of these changes (there are likely similar
issues with most of the touched files).
Thanks,
David
------------------------------------------------------------------------------
