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

[David Holmes]

Thanks Kim!

David

On 20/07/2020 4:15 pm, Kim Barrett wrote:
>> On Jul 20, 2020, at 1:53 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> Thanks for looking at this.
>>
>> Updated webrev at:
>>
>> http://cr.openjdk.java.net/~dholmes/8249650/webrev.v2/
> 
> Looks good.
> 
>>
>> 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:
>>> 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.
> 
> I don’t feel strongly either way.  It just struck me as a little odd to have the mix in close proximity,
> especially since I think consistently using either one might work in this function.  But being consistent
> about make_local usage has something to be said for it too.
> 
>>> 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).
> 
> Yeah, it’s not really obvious whether to use THREAD or thread in some cases.
> But I agree that addressing any inconsistencies there is mostly out of scope for
> this change.
>