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

[Lois Foltan]

Thank you again Karen for the review!
Lois

On 2/4/2019 3:57 PM, Karen Kinnear wrote:
> Lois,
>
> Apologies for my suggestion which caused you extra work - I did not read it carefully enough.
> Thank you for the careful testing.
>
> I don’t need to see a new webrev.
>
> thanks,
> Karen
>
>> On Feb 4, 2019, at 2:18 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>>
>> On 2/4/2019 1:45 PM, Lois Foltan wrote:
>>> On 2/4/2019 11:53 AM, Karen Kinnear wrote:
>>>> Lois,
>>>>
>>>> Code looks good. Couple of minor questions.comments
>>> Thank you Karen for the review!  Addressed all your comments below please see additional responses.  Updated webrev at:
>>>
>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8217998.2/webrev/
>>>
>>> Rerunning hs-tier1-3 & jdk-tier1-3 testing.
>>>
>>>> 1. rewriter.hpp: comment line 162 needs updating - we just use 1 entry now
>>> Fixed the comment and also renamed the method from "add_invokedynamic_resolved_references_entries" to "add_invokedynamic_resolved_references_entry".  We were always only adding 1 entry even though previously that entry spanned multiple slots in the _invokedynamic_references_map.
>>>
>>>> 2. jvmciCompilerToVM.cpp
>>>> You removed an assertion about method_type_if_resolved.
>>>> Would it make sense to add an assertion that !has_local_sig - which if I am reading this correctly,
>>>> you are setting to indicate sig-poly methods?
>>> Added.
>> Clarification on webrev .2 above, it does not contain the addition of the assertion as I indicated in my response.  The original assertion was not simply about whether a method_type was present or not.  It was about either the method_type is not present or the method_type is present and has not yet been resolved.  So a replacement of that vmassert with a call to has_local_signature does not equate and numerous compiler/aot tests were failing when I retested.
>>
>> Thanks,
>> Lois
>>
>>>> Does jvmci use the method_type field?  Or was removing this assertion the only change needed?
>>> This was the only instance of the use.  However, I have sent email to Dean Long and cc'd Vladimir Kozlov asking for review.
>>>
>>>> 3. ciStreams.cpp
>>>> comment line 367: “There is are” -> “There are”
>>> Fixed.
>>>
>>>> 4. ciStreams.cpp
>>>> line 336 sets local_signature
>>>> Would it make sense to only do that inside the check for if (has_local_signature()) - e.g. after line 393?
>>> Fixed.
>>>
>>>> 5. Testing - the comments in ciStreams.cpp get_method are very helpful
>>>> Are there existing tests for these cases or do we need any new ones?
>>> I did check the tests within jdk/java/lang/invoke, numerous scenarios for VH.get(), Lambda form for a method call dealing with linkStatic as the comments indicate, and tests dealing with variable arity, etc.  So I believe the cases are covered.
>>>
>>> Thanks,
>>> Lois
>>>
>>>> thanks,
>>>> Karen
>>>>
>>>>> On Feb 4, 2019, at 7:32 AM, Lois Foltan <lois.foltan at oracle.com> 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