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

[coleen.phillimore at oracle.com]

Ok, looks good to me.
Colen

On 7/21/20 10:46 PM, David Holmes wrote:
> Hi Coleen,
>
> On 22/07/2020 4:01 am, coleen.phillimore at 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.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
>>>
>>>> ------------------------------------------------------------------------------ 
>>>>
>>>>
>>