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

[coleen.phillimore at oracle.com]

On 2/11/19 2:16 PM, Lois Foltan wrote:
>
>
> On 2/6/2019 3:34 PM, Lois Foltan wrote:
>>
>> 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 :)
>
> Hi Coleen,
>
> I looked over your review comments again and you are correct 
> concerning the need for CallInfo::_has_local_sig.  It is always set to 
> true for MHs and indy and thus really is not necessary for setting the 
> local variable "has_local_sig" within the method 
> ConstantPoolCacheEntry::set_method_handle_common().  I have removed 
> CallInfo::_has_local_sig. 
> ConstantPoolCacheEntry::has_local_signature_shift is still necessary, 
> however.
>
> Updated webrev at: 
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.3/webrev/

This looks great!
Thanks!
Coleen

>
> Thanks for your review and help with this!
> Lois
>
>>
>> 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
>>>
>>
>