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

Daniel D. Daugherty daniel.daugherty at oracle.com
Mon Jul 20 17:07:10 UTC 2020


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!


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.

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

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.

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

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

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

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

     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?

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.

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 hotspot-runtime-dev mailing list