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

[Vladimir Kozlov]

Hi David,

Changes look good.

On 7/20/20 10:07 AM, Daniel D. Daugherty wrote:
 > src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
 >      L1021:   HandleMark hm;
 > Can this be 'hm(THREAD)'? (Not your problem, but while you're in that file?)

There are several cases like this in jvmciCompilerToVM.cpp and may be in other places.
I think it should be done as separate clean up.

Thanks,
Vladimir

On 7/19/20 10:53 PM, 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
> 
>> ------------------------------------------------------------------------------
>>