On Tue, 11 Jul 2023 15:15:51 GMT, Chen Liang <li...@openjdk.org> wrote:
>> As John Rose has pointed out in this issue, the current j.l.r.Proxy based
>> implementation of MethodHandleProxies.asInterface has a few issues:
>> 1. Exposes too much information via Proxy supertype (and WrapperInstance
>> interface)
>> 2. Does not allow future expansion to support SAM[^1] abstract classes
>> 3. Slow (in fact, very slow)
>>
>> This patch addresses all 3 problems:
>> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method
>> clashes
>> 2. This patch obtains already generated classes from a ClassValue by the
>> requested interface type; the ClassValue can later be updated to compute
>> implementation generation for abstract classes as well.
>> 3. This patch's faster than old implementation in general.
>>
>> Benchmark for revision 17:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15
>> 1.503 ± 0.021 ns/op
>> MethodHandleProxiesAsIFInstance.baselineCompute avgt 15
>> 0.269 ± 0.005 ns/op
>> MethodHandleProxiesAsIFInstance.testCall avgt 15
>> 1.806 ± 0.018 ns/op
>> MethodHandleProxiesAsIFInstance.testCreate avgt 15
>> 17.332 ± 0.210 ns/op
>> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15
>> 19.296 ± 1.371 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 5
>> 0.419 ± 0.004 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 5
>> 0.421 ± 0.004 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 5
>> 1.731 ± 0.018 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 5
>> 0.418 ± 0.003 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 5
>> 0.263 ± 0.003 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 5
>> 0.262 ± 0.002 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 5
>> 0.262 ± 0.002 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 5
>> 0.267 ± 0.019 ns/op
>> MethodHandleProxiesAsIFInstanceCall.direct avgt 5
>> 0.266 ± 0.013 ns/op
>> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 5
>> 18.057 ± 0.182 ...
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix the lazy test, thanks Jorn Vernee!
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 325:
> 323: r.set(cl);
> 324: }
> 325: return new Lookup(cl);
Suggestion:
if (cl != null)
return new Lookup(cl);
synchronized (r) {
cl = r.get();
if (cl == null) {
// If the referent is cleared, create a new value and update
cached weak reference.
cl = newProxy(intfc);
r.set(cl);
}
return new Lookup(cl);
}
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 55:
> 53: * @run junit ProxiesImplementationTest
> 54: */
> 55: public class ProxiesImplementationTest {
how about `WrapperHiddenClassTest`? It's explicit that this verifies the
wrapper hidden class.
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 130:
> 128: @Test
> 129: public void testNoAccess() {
> 130: Client untrusted = asInterfaceInstance(Client.class,
> MethodHandles.zero(void.class));
Suggestion:
Client obj = asInterfaceInstance(Client.class,
MethodHandles.zero(void.class));
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 132:
> 130: Client untrusted = asInterfaceInstance(Client.class,
> MethodHandles.zero(void.class));
> 131: var instanceClass = untrusted.getClass();
> 132: var leakLookup = Client.leakLookup();
This is not really malicious code. It's checking the interface has no access
to the proxy class. Probably better to rename them to be less alarming.
Suggestion:
var lookup = Client.lookup();
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 134:
> 132: var leakLookup = Client.leakLookup();
> 133: assertEquals(MethodHandles.Lookup.ORIGINAL,
> leakLookup.lookupModes() & MethodHandles.Lookup.ORIGINAL,
> 134: "Leaked lookup original flag");
Suggestion:
"expect lookup has original flag");
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 194:
> 192:
> 193: var c1 = asInterfaceInstance(ifaceClass, mh);
> 194: cl = new WeakReference<>(c1.getClass());
Nit: define a new variable not to mix with `c1`.
Suggestion:
var wr = new WeakReference<>(c1.getClass());
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");
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 201:
> 199: Reference.reachabilityFence(c1);
> 200:
> 201: // allow GC in interpreter
This not only affects GC in interpreter. maybe something like this:
Suggestion:
// clear strong reference to the wrapper instances
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesImplementationTest.java
line 206:
> 204:
> 205: System.gc();
> 206: assertTrue(cl.refersTo(null), "MHP impl class should be cleared
> by gc"); // broken
Use `jdk.test.lib.util.ForceGC` to make the check more reliable. Add
`@library /test/lib` to use the test library.
Suggestion:
if (ForceGC.wait(() -> wr.refersTo(null))) {
assertTrue(wr.refersTo(null), "MHP impl class should be cleared by
gc");
}
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesInterfaceTest.java line 56:
> 54: * @run junit ProxiesInterfaceTest
> 55: */
> 56: public class ProxiesInterfaceTest {
I can't tell from the test name that is any different than the basic test.
It seems that this test can be merged with `ProxiesBasicTest.java`. It can
simply be named as `BasicTest.java`. The prefix `Proxies` seems redundant.
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesInterfaceTest.java line
143:
> 141: }
> 142:
> 143: //<editor-fold desc="Infrastructure">
this comment can be deleted.
test/jdk/java/lang/invoke/MethodHandleProxies/ProxiesInterfaceTest.java line
257:
> 255: public non-sealed interface NonSealed extends Sealed {
> 256: }
> 257: //</editor-fold>
comment inserted by IDE?
test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstanceCall.java
line 36:
> 34: import org.openjdk.jmh.annotations.Warmup;
> 35:
> 36: import java.lang.invoke.LambdaMetafactory;
this import is unused.
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?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260091588
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260084281
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260038998
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260042092
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260042412
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260063660
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260065837
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260085900
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260070232
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260082317
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260076642
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260077571
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260086803
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1260087517
