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

[Karen Kinnear]

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