JDK-8217998: Remove method_type field associated with the appendix field of an indy or method handle call

[Lois Foltan]

On 2/5/2019 6:01 PM, coleen.phillimore at oracle.com wrote:
>
> Hi Lois,  I like that there are no longer two resolved references for 
> indy instructions, especially if it wasn't used.

Thanks Coleen for the review!  More below.

>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.2/webrev/src/hotspot/share/interpreter/linkResolver.cpp.udiff.html 
>
>
> I can't find a call where set_handle() passes false for the 
> _has_local_sig field.

There is no call to set_handle() that passes false for the 
_has_local_sig field.  It is set to false initially in 
CallInfo::set_common.  I considered not passing the "has_local_sig" 
parameter to set_handle and just always set CallInfo::_has_local_sig to 
true within that method but I am assuming John wanted to allow for the 
possibility that it could be set to false in the future.

>
> Can you rename CallInfo::_has_local_signature, since it's not that 
> much longer and reads better?
>
> + const bool has_local_sig = call_info.has_local_signature();
> + assert(has_local_sig, "MHs and indy are always sig-poly");

I'm confused.  The field currently within CallInfo is named 
CallInfo::_has_local_sig.  Are you referring to the method 
CallInfo::has_local_signature()?  Do you want that shortened?

>
> So why do you need this field if it's always true?  Was hoping for a 
> cpCache bit back :)

It's not always true.

>
> Perhaps there's a future use?
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.2/webrev/src/hotspot/share/oops/cpCache.hpp.udiff.html 
>
>
> + has_local_signature_shift = 25, // (M) does the call site have a 
> per-site signature (sig-poly methods)?
>
>
> Can you make this an "S" vs. "M" and change the coment in the top of 
> cpCache.hpp:
>
> // _flags     [tos|0|F=0|M|A|I|f|0|vf|indy_rf|000|00000|psize] (for 
> method entries)
> // bit length [ 4 |1| 1 |1|1|1|1|1|1 |-4--|--8--|--8--]

Done, changed the "M" to "S" in both places.  The comment in the top of 
cpCache.hpp does not refer to the method_type in any form, so unclear as 
to how you want me to change it?  I am assuming you want me to remove 
the "method_type info (t_section) from this comment?

// _flags   = method type info (t section),
//            virtual final bit (vfinal),


>
> And a line under this:
>
> // A      = call site has an appendix argument (loaded from resolved 
> references)

Not sure what you mean by this?

Thanks,
Lois

>
> Although I don't like this comment because it gets out of date.
>
> Thanks,
> Coleen
>
> On 2/4/19 7:32 AM, Lois Foltan wrote:
>> Please review this change that removes the unused method_type field 
>> associated with the appendix field of an indy or method handle call.  
>> This is the first of a series of changes working towards JDK-8210685: 
>> Bootstrap method consolidation.
>>
>> open webrev at: 
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.1/webrev/
>> bug link: https://bugs.openjdk.java.net/browse/JDK-8217998
>> co-contributors: Lois Foltan & John Rose
>>
>> Testing: hs-tier1-4 & jdk-tier1-3 (all platforms), hs-tier5-8 (linux 
>> only), JCK vm & lang (linux only)
>>
>> Thanks,
>> Lois
>