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

[Lois Foltan]

On 2/11/2019 2:58 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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 Coleen for another review of it!
Lois

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