On Tue, 11 Jul 2023 13:40:27 GMT, Jorn Vernee <jver...@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
