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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (M) 8249650: Optimize JNIHandle::make_local thread variable usage
From:       coleen.phillimore () oracle ! com
Date:       2020-07-22 12:25:13
Message-ID: e6a0a004-7805-7985-d844-5a2e74cf0814 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic