RFR (M) 8249650: Optimize JNIHandle::make_local thread variable usage

[David Holmes]

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.holmes at 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

> ------------------------------------------------------------------------------
>