On Tue, 4 Jul 2023 14:14:03 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Chen Liang has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 44 commits: >> >> - Merge branch 'master' into explore/mhp-iface >> - stage >> >> Signed-off-by: liach <li...@users.noreply.github.com> >> - Review comments >> - Code cleanup, thanks mandy! >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> explore/mhp-iface >> - 1. Change WRAPPER_TYPES to WeakHashMap to accurately determine if the >> given class is >> the proxy class >> >> Discussion: >> 2. I dropped ProxyClassInfo and use Lookup just to see the simplication. >> If wrapperInstanceTarget and wrapperInstanceType are frequently >> called, it makes >> sense to cache the method handles. >> >> 3. Should it use SoftReference or WeakReference? It depends if >> asInterfaceInstance >> will be used heavily. >> >> 3. I also dropped SamInfo and getStats as it can be inlined in the >> caller, which >> I think it's clearer to see what it does in place. >> - SecurityManager fixed, minimize changes >> - Merge branch 'master' into explore/mhp-iface >> - Some changes per Mandy's review. SecurityManager test fails in this patch >> - Merge branch 'master' into explore/mhp-iface >> - ... and 34 more: https://git.openjdk.org/jdk/compare/0e3d91dc...44e62271 > > src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line > 324: > >> 322: r = PROXY_LOOKUPS.get(intfc); >> 323: } >> 324: return r.get(); > > This doesn't look right to me. AFAICT the Lookup can be GC'd before we call > `get`. > >> No, it wouldn't. PROXY_LOOKUPS.get(intfc) returns the WeakReference of the >> lookup class which is strongly reachable in the stack frame. > > I'm not sure how the lookup class (`intfc` ?) is keeping the `Lookup` alive > here? Sorry I re-read Chen's question and missing the last half of the sentence. My reply was about: If the referent is not GC'ed, `r.get()` returns non-null which will be returned to the caller. If the referent becomes weakly reachable, `r.get()` will return null. OK. In any case, I was thinking to use a local variable to keep the lookup object strongly reachable. I adjusted the patch to keep a class value of a holder of `WeakReference` ([ab3d8b2](https://github.com/mlchung/jdk/commit/ab3d8b2f086dfac4f8fda1ddce415270c0c53d2b)): private static Lookup getProxyClassLookup(Class<?> intfc) { WeakReferenceHolder<Lookup> r = PROXY_LOOKUPS.get(intfc); Lookup lookup = r.get(); if (lookup == null) { // If the referent is cleared, create a new value and update cached weak reference. lookup = newProxyLookup(intfc); r.set(lookup); } return lookup; } ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1252356427
