[Nestmates] RFR: 8189163: [Nestmates] Updated invokeInterface selection/resolution rules

[David Holmes]

Hi Karen,

Thanks for going through this.

On 8/03/2018 8:26 AM, Karen Kinnear wrote:
> David,
> 
> Code looks good. Wouldn’t hurt to have a compiler person (Tobias?) look at the
> dependencies is_witness().

I actually directly cc'd Tobias on this RFR, but have not heard from 
him. Perhaps he is away.

> Totally agree with making the SelectionResolution cleanup a separate bug. Had a couple
> of questions on expected behavior changes for that one that I will put in 8194857.

Ok.

> I very much appreciate your test design - how clear it is what is happening and
> how few generated/jcod classes you needed for these parts of the matrix.
> And that you tested direct, MH and reflection. Many thanks!
> 
> Really minor:
> 1. TestMethodSelection.java, TestInterfaceMethodSelection.java
> Could you possibly modify the comment - if you agree with me :-)
> “The second run disables inflation” to “the second run disables the inflation threshold”
> (I know the names says noInflation, but to me it means “inflate immediately”)

Hmmm ... It's the inflation that is disabled, you can't disable a 
"threshold". This comment is in all the reflection tests so I'd need to 
change them all. Let's think about this some more before the real RFR 
before integration.

> And many thanks for testing  both reflection approaches!
> 
> 2. TestInterfaceMethodSelection.java
> line 80: did you mean to have I.m public in this case?

Yes - thanks.

> 3. TestMethodSelection.java
> line 78: did you mean to have A.m public in this case?

Yes - thanks.

> What was the reason for having class A in the exercise?
> I would understand if A.m were the target, then you could experiment
> with skipping e.g. C.m but finding B.m, which might have been an issue
> if we had a different behavior for local method finding for selection - like
> we do for resolution, but we don’t. (e.g. interface method resolution will find a local private
> method but not a super interface private method), i.e. I can imagine
> writing a bug that would catch.

Given we always resolve to B.m I wanted there to be both a super A.m and 
a sub C.m, to test the selection process. It's a negative test if you 
like, that we never select A.m. The non-interface version of the test 
serves as a sanity check of the interface version - even though 
non-interface selection behaviour has not been changed.

Thanks,
David
-----

> thanks!
> Karen
> 
>> On Mar 5, 2018, at 1:54 AM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189163
>> webrev: http://cr.openjdk.java.net/~dholmes/8189163/webrev/
>> VM Spec update: http://cr.openjdk.java.net/~dholmes/8010319/specs/JVMS/nestmates.html
>>
>> The "resolution" part of this is actually already addressed by virtue of being able to use invokeinterface for private interface methods.
>>
>> This part addresses the key spec change in relation to existing method selection rules for invokeinterface - which is intended to bring it into line with how invokevirtual already behaves. The key points to note in method selection are:
>>
>> 1. If the resolved method is private then the selected method is always the resolved method. (And that is always the implementation in the interface when dealing with invokeinterface.)
>>
>> 2. A private method never overrides any method by the same name/signature in the inheritance hierarchy.
>>
>> The key consequence of (2) is that private methods are skipped when selecting the method to call when the resolved method is not private. So in the past, for example, if you were looking for an implementation of a public interface method m() in a class C, and you found a private definition of m(), you would "select" it and then throw IllegalAccessError. Under the new rules you skip-over/ignore it and so continue looking for a suitable m() implementation.
>>
>> In terms of implementing this change there were three very similar and very small changes to cause private methods to be skipped:
>>
>> 1. src/hotspot/share/interpreter/linkResolver.cpp: runtime_resolve_interface_method
>>
>> More appropriately named "select_interface_method", in this method we now call LinkResolver::lookup_instance_method_klasses with the "skip_private" flag.
>>
>> The bulk of the changes in LinkResolver and src/hotspot/share/oops/* are simply to expose the PrivateLookupMode parameter so that it can be passed through explicitly to uncached_lookup_method and find_instance_method. By default we assume "find_private", so we did not have to modify all the existing callsites.
>>
>> 2.  src/hotspot/share/oops/klassVtable.cpp: klassItable::initialize_itable_for_interface
>>
>> When initializing the itable for a class, skip private class methods as they can never be the implementation of an interface method.
>>
>> 3.  src/hotspot/share/code/dependencies.cpp: bool is_witness(Klass* k)
>>
>> When looking for an implementation for "m" in k to see if the uniqueness of "m" has changed, skip private methods as they cannot override the known "m" and so do not affect its uniqueness.
>>
>> The change in src/hotspot/share/prims/methodHandles.cpp corrects a terminology issue whereby a private interface method is referred to as a "default method", which it is not.
>>
>> Tests:
>>
>> There are two new tests:
>>
>> - runtime/Nestmates/methodSelection/TestInterfaceMethodSelection.java
>> - runtime/Nestmates/methodSelection/TestMethodSelection.java
>>
>> Although only invokeinterface selection rules changed, I wrote the equivalent invokevirtual version as a sanity check. The tests start from a known resolved method and then tests the selection process for private/public variants of a method in a hierarchy of 2 or 3. There is very elaborate documentation in the tests explaining exactly what they do (and don't do). The obvious variants of these tests with different resolution and accessibility are already tested elsewhere, so I did not duplicate that effort.
>>
>> The following existing tests had to be modified:
>>
>> - compiler/jsr292/methodHandleExceptions/TestAMEnotNPE.java
>>
>> The cases which previously selected a private class method and then threw IllegalAccessError, no longer do that. Whether a test case results in a successful call or an exception depends on the overall structure of the test case.
>>
>> - runtime/SelectionResolution/classes/selectionresolution/Template.java
>>
>> As discussed in the open issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8194857
>>
>> the selectionResolution tests need to be updated for the changes delivered in Nestmates. Unfortunately they are very difficult tests to refactor and so at this stage the failing tests have been excluded. In this particular case I was able to make an adjustment at the second level in the Template IfaceMethodrefSelectionOverrideNonPublic by commenting out all the cases involving private interface methods. That meant that, compared to commenting out the top-level tests, instead of reducing the test count by 7800, it only reduced by 2600.
>>
>> Testing:
>> - jtreg hotspot/runtime
>>          hotspot/compiler
>> - hs-tier1-2
>> - jdk tier1-3
>> - internal resolution/selection tests
>>
>> Thanks,
>> David
>