RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v22]

[Chen Liang]

On Tue, 11 Jul 2023 13:40:27 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> @mlchung Thank you for the new round of review.
>> I have split the large test into 3 parts, testing general contracts, against different types of interfaces, and implementation-related tests. The old MethodHandleProxies test, which tests against different types of interfaces, is merged into the interface one. The module one, being an implementation detail, is merged into the implementation test.
>> 
>> I have a few problems that need help:
>> 1. I think the type profile pollution only happens with the instance-field approach, as I recall different instance fields' MHs pollute profiling. The comment need to be corrected if I'm right.
>> 2. The weak reference test in the impl-related test breaks in weird ways:
>>    1. https://github.com/openjdk/jdk/pull/13197/files#diff-effc9e10eca79d537b6321e9d72b8bbcf0fc6c1c2ac9d13f3524951bc54d48fdR196
>>       When this line is removed, the next call to `asInterfaceInstance` will return a wrapper with a new implementation class, even if the old instance is still in a local variable; as in jshell:
>> 
>>       ```java
>>       import java.lang.invoke.*;
>>       var mh = MethodHandles.zero(void.class);
>>       var c1 = MethodHandleProxies.asInterfaceInstance(Runnable.class, mh);
>>       System.gc();
>>       var c2 = MethodHandleProxies.asInterfaceInstance(Runnable.class, mh);
>>       c1.getClass() == c2.getClass()
>>       ```
>>       The last `==` line shows that `c1` and `c2` are both reachable, so there are 2 coexistent implementation classes somehow; not sure how the original one is lost in gc. Without gc, the 2 wrappers have the same implementation class.
>>    2. https://github.com/openjdk/jdk/pull/13197/files#diff-effc9e10eca79d537b6321e9d72b8bbcf0fc6c1c2ac9d13f3524951bc54d48fdR202
>>        The MHP implementation class weak reference is not cleared by gc, even though the wrapper is no longer reachable.
>
>> 1. I think the type profile pollution only happens with the instance-field approach, as I recall different instance fields' MHs pollute profiling. The comment need to be corrected if I'm right.
> 
> What happens is not really 'profile pollution'. The issue is that C2 can not inline through non-constant method handles. So we always get an indirect call through the MethodHandle's [invokeBasic stub](https://github.com/openjdk/jdk/blob/a1cfc9695405fe517fae1b9f760ae42b85f66be9/src/hotspot/cpu/x86/methodHandles_x86.cpp#L165).
> 
> EDIT: Sorry, I should have read Mandy's suggestion more closely. The issue when we generate many classes (one per method handle), is that when we call the resulting interface instances at the same call site, the different implementation classes will cause profile pollution.
> 
>> When this line is removed, the next call to asInterfaceInstance will return a wrapper with a new implementation class, even if the old instance is still in a local variable.
> 
> The WeakReference inside the implementation points to the `Lookup` object. I think this Lookup goes out of scope after `asInterfaceInstance` returns, right? (it's not saved inside the returned instance or class). So, it would be immediately eligible for GC.
> 
> I think a SoftReference would work better for the cache (which the GC clears less eagerly). Or the returned interface instance could save a reference to the Lookup object in order to keep it reachable.
> 
>> The MHP implementation class weak reference is not cleared by gc, even though the wrapper is no longer reachable.
> 
> Note that when running in interpreted mode (which is likely for these tests), a variable is 'alive' from the perspective of the GC until the method ends. i.e. the `{ ... }` used in the test does nothing WRT GC. You would have to explicitly make `c1` `null` for the class to be unreachable.

Thank you so much @JornVernee: The WeakReference should point to the impl class. The Lookup is a cheap wrapper, so I changed it to be created each time instead. The test comes back green on my device with your suggestions.

I think this patch is stable now; feel free to review again, especially that I've rearranged the tests.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1631015105