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

[David Holmes]

Thanks Serguei!

David

On 22/07/2020 5:25 am, serguei.spitsyn at oracle.com wrote:
> Hi David,
> 
> The fix looks good to me.
> 
> Thanks,
> Serguei
> 
> 
> 
> On 7/19/20 22:53, 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.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
>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>