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

[Lois Foltan]

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
>