RFR (M) 8233913: Remove implicit conversion from Method* to methodHandle

[coleen.phillimore at oracle.com]

We meant to send this to the mailing list.
Coleen

On 11/12/19 2:44 PM, coleen.phillimore at oracle.com wrote:
>
>
> On 11/12/19 1:28 PM, Ioi Lam wrote:
>> Hi Coleen,
>>
>> I see some replacement of methodHandle to Method*.
>>
>> Is there a general rule about when we can use a Method*, and when we 
>> must use a methodHandle? I can only find this in the comments
>>
>> // Metadata Handles.  Unlike oop Handles these are needed to prevent 
>> metadata
>> // from being reclaimed by RedefineClasses.
>> // Metadata Handles should be passed around as const references to 
>> avoid copy construction
>> // and destruction for parameters.
>>
>> but it's not clear when RedefineClasses can happen.
>
> RedefineClasses can only happen at a safepoint. This is never going to 
> change to handshakes even though some people want everything to be 
> handshakes :)   Also, while classfile parsing and loading, we're not 
> going to be able to redefine the class, so the Method*/ConstantPool* 
> are safe there, but I didn't want the code to have the distinction, 
> because that's hard, so there are some 
> methodHandles/constantPoolHandles there too.   Most are left over from 
> Permgen days because they could move.
>
> At one point, I was going to write a CheckUnhandledMetadata code but 
> it's not as easy as the oop code because Method* isn't a typedef that 
> I can conveniently make into a class, like we did with oop.
>
> One thing we did with these handles long ago, was to change all the 
> parameters to take Handles rather than the oops so that we could 
> Handleize something as high up the call stack as possible, and then 
> not have to rehandle them.  So that's what I did here.
>
> Some places I went the other way like the print functions because the 
> callers didn't have a Handle already (they got the Method* out of some 
> data structure).
>
> And I changed the return types to have Method* because these were also 
> places that came out of some data structure (eg. 
> InstanceKlass::find_method()) so we'd have to call Thread::current to 
> turn it into a methodHandle then pass it back up.
>
> So I guess the general rule is to Handleize things at the top of the 
> call stack and pass them down, but not at the bottom of the call stack 
> and pass them up.   Where your stacks grow down.
>
> I don't think it's 100% consistent yet, but I'm trying to get closer.
>
> Coleen
>>
>> Thanks
>> - Ioi
>>
>> On 11/11/19 11:59 AM, coleen.phillimore at oracle.com wrote:
>>> Summary: Fix call sites to use existing THREAD local or pass down 
>>> THREAD local for shallower callsites. Make linkResolver methods 
>>> return Method* for caller to handleize if needed.
>>>
>>> There are a small number of changes to several files, mostly 
>>> obvious.  Some comments on a few of the specific changes:
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev/src/hotspot/share/aot/aotCompiledMethod.cpp.udiff.html 
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev/src/hotspot/share/code/nmethod.cpp.udiff.html 
>>>
>>> The comment and the methodHandle don't make sense since there'a s 
>>> NSV there.  Method* will not be reclaimed ever, and it doesn't 
>>> move.  There might have been a safepoint here once.
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev/src/hotspot/share/ci/ciMethod.cpp.udiff.html 
>>>
>>> If you have a methodHandle, you don't need to do 
>>> mh()->max_stack().   The -> operator will expose the underlying 
>>> Method*.
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev/src/hotspot/share/classfile/javaClasses.cpp.udiff.html 
>>>
>>> The Method* in the vframeStream is safe here because the stack frame 
>>> are followed in case of safepoint, and will mark the Method* as 
>>> live, so this was unnecessary.
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev/src/hotspot/share/compiler/tieredThresholdPolicy.hpp.udiff.html 
>>>
>>> I changed several of the TieredThresholdPolicy functions to take 
>>> const methodHandle as a parameter to avoid unhandleizing and 
>>> rehandleizing, and avoid Thread::current() calls.
>>>
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev/src/hotspot/share/interpreter/linkResolver.hpp.udiff.html 
>>>
>>> I changed LinkResolver methods to return Method* to avoid 
>>> unnecessary handlizing.  The handle copy is elided by most compilers 
>>> but it was still not needed by many callers.
>>>
>>> Tested with tier1 on all Oracle platforms, and tier 2-3 on linux-x64.
>>>
>>> I also performance tested this with slight avg 0.5% improvement, and 
>>> fewer instructions:
>>>
>>> eg: PerfStartup-Noop instructions on linux-x64 (before/after)
>>>
>>> 0.49%
>>> 149943356.15
>>> ± 262156.46
>>>     149213135.00
>>> ± 281141.80
>>> p = 0.000
>>>
>>>
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2019/8233913.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8233913
>>>
>>> thanks,
>>> Coleen
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>