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

David Holmes david.holmes at oracle.com
Wed Jul 22 02:46:26 UTC 2020


Hi Dan,

On 21/07/2020 3:07 am, Daniel D. Daugherty wrote:
> 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/
> 
> I like this cleanup very much!

Thanks for looking at it.

> 
> src/hotspot/share/classfile/javaClasses.cpp
>      No comments.
> 
> src/hotspot/share/classfile/verifier.cpp
>      L298:   JavaThread* thread = (JavaThread*)THREAD;
>      L307:   ResourceMark rm(THREAD);
>          Since we've gone to the trouble of creating the 'thread' variable,
>          I would prefer it to be used instead of THREAD where possible.

Okay I made this change as we already use "thread" throughout that method.

> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>      L1021:   HandleMark hm;
>          Can this be 'hm(THREAD)'? (Not your problem, but while you're
>          in that file?)

It probably could but there are around 8 such uses and I don't want to 
expand this change any further than necessary for the current issue. I 
filed a general RFE for things that should take advantage of having a 
current thread reference already (that will encompass Coleen's 
make_local(obj) change as well).

https://bugs.openjdk.java.net/browse/JDK-8249837

> src/hotspot/share/prims/jni.cpp
>      No comments.
> 
> src/hotspot/share/prims/jvm.cpp
>      L140:   ResourceMark rm;
>          Can this be 'rm(THREAD)'? (Not your problem, but while you're
>          in that file?)
> 
>      L611:   Handle stackStream_h(THREAD, 
> JNIHandles::resolve_non_null(stackStream));
>      L617:   objArrayHandle frames_array_h(THREAD, fa);
>      L626:   return JNIHandles::make_local(THREAD, result);
>          Since we've gone to the trouble of creating the 'jt' variable,
>          I would prefer it to be used instead of THREAD where possible.
> 
>      L767:   vframeStream vfst(thread);
>      L788         return (jclass) JNIHandles::make_local(THREAD, 
> m->method_holder()->java_mirror());
>          Can we use 'thread' on L788? (preferred)
>          Can we use 'THREAD' on L767? (less preferred)
> 
>      L949:   ResourceMark rm(THREAD);
>      L951:   Handle class_loader (THREAD, JNIHandles::resolve(loader));
>      L955:                            THREAD);
>      L957:   Handle protection_domain (THREAD, JNIHandles::resolve(pd));
>      L968:   return (jclass) JNIHandles::make_local(THREAD, 
> k->java_mirror());
>          Since we've gone to the trouble of creating the 'jt' variable,
>          I would prefer it to be used instead of THREAD where possible.

As per our slack chat, and the fact you are okay with things as-is, I 
will forego a more general "consistency" pass as it is unclear what is 
best here. As Coleen notes THREAD is generally understood to always be 
the current thread, while thread/jthread/jt could be any old thread in 
general. Also THREAD usage can highlight a Thread* API, while "thread" 
has to be used for JavaThread* API - but obviously that needs to be 
carefully and consistently applied to be useful. :)

>      L986:   JavaThread* jt = (JavaThread*) THREAD;
>          This 'jt' is unused and can be deleted (Not your problem, but 
> while you're
>          in that file?)

Fixed (and another case elsewhere).

>      L1154:   while (*p != '\0') {
>      L1155:       if (*p == '.') {
>      L1156:           *p = '/';
>      L1157:       }
>      L1158:       p++;
>          Nit - the indents are wrong on L1155-58. (Not your problem, but 
> while you're
>          in that file?)

Fixed

>      L1389:   ResourceMark rm(THREAD);
>      L1446:     return JNIHandles::make_local(THREAD, result);
>      L1460:   return JNIHandles::make_local(THREAD, result);
>          Can we use 'thread' on L1389? (preferred) And then the line you
>          touched could also be 'thread' and we'll be consistent in this
>          function...

Left as-is.

>      L3287:   oop jthread = thread->threadObj();
>      L3288:   assert (thread != NULL, "no current thread!");
>          I think the assert is wrong. It should be:
> 
>              assert(jthread != NULL, "no current thread!");
> 
>          If 'thread == NULL', then we would have crashed at L3287.
>          Also notice that I deleted the extra ' ' before '('. (Not
>          your problem, but while you're in that file?)

Fixed. I was initially concerned about bootstrapping but it is fine - we 
ensure we set threadObj() before executing any Java code.

>      L3289:   return JNIHandles::make_local(THREAD, jthread);
>          Can you use 'thread' instead of 'THREAD' here for consistency?
> 
>      L3681:     method_handle = Handle(THREAD, 
> JNIHandles::resolve(method));
>      L3682:     Handle receiver(THREAD, JNIHandles::resolve(obj));
>      L3683:     objArrayHandle args(THREAD, 
> objArrayOop(JNIHandles::resolve(args0)));
>      L3685:     jobject res = JNIHandles::make_local(THREAD, result);
>          Can you use 'thread' instead of 'THREAD' here for consistency?
> 
>      L3705:   objArrayHandle args(THREAD, 
> objArrayOop(JNIHandles::resolve(args0)));
>      L3707   jobject res = JNIHandles::make_local(THREAD, result);
>          Can you use 'thread' instead of 'THREAD' here for consistency?

Left as-is.

> src/hotspot/share/prims/methodHandles.cpp
>      No comments.
> 
> src/hotspot/share/prims/methodHandles.hpp
>      No comments.
> 
> src/hotspot/share/prims/unsafe.cpp
>      No comments.
> 
> src/hotspot/share/prims/whitebox.cpp
>      No comments.
> 
> src/hotspot/share/runtime/jniHandles.cpp
>      No comments.
> 
> src/hotspot/share/runtime/jniHandles.hpp
>      No comments.
> 
> src/hotspot/share/services/management.cpp
>      No comments.
> 
> 
> None of my comments above are "must do". If you choose to make the
> changes, a new webrev isn't required, but would be useful for a
> sanity check.

In addition to the tweak above I found a bunch of make_locasl(obj) 
usages in jvm.cpp and jni.cpp thanks to Coleen, which I have also fixed. 
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
-----

> Thumbs up.
> 
> Dan
> 
> 
>>
>> 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
>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
> 


More information about the serviceability-dev mailing list