On Tue, 11 Jul 2023 17:36:33 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix the lazy test, thanks Jorn Vernee! > > test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java > line 198: > >> 196: System.gc(); >> 197: var c2 = asInterfaceInstance(ifaceClass, mh); >> 198: assertTrue(cl.refersTo(c2.getClass()), "MHP should reuse >> implementation class when available"); > > We can simplify the test a little bit. Just to check if the class of `c1` > and `c2` is the same first. > > Suggestion: > > var c2 = asInterfaceInstance(ifaceClass, mh); > assertTrue(c1.getClass() == c2.getClass(), "MHP should reuse > implementation class when available"); I will keep the gc, which ensures a scenario like that incorrectly weakref'd Lookup doesn't happpen again. > test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesInterfaceTest.java line > 143: > >> 141: } >> 142: >> 143: //<editor-fold desc="Infrastructure"> > > this comment can be deleted. It was a leftover from when I tried to organize the tests with IDE editor folds. No longer necessary indeed. > test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstanceCall.java > line 49: > >> 47: /** >> 48: * Benchmark evaluates the call performance of >> MethodHandleProxies.asInterfaceInstance >> 49: * return value, compared to > > incomplete comment? On a side note, these benchmarks were added for the class-per-MH implementation; I probably need to reevaluate if these 2 benchmarks are needed, or if the original `MethodHandleProxiesAsIFInstance.java` suffices. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260124340 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260116789 PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260116076
