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

[David Holmes]

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
>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>