RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v22]
Mandy Chung
mchung at openjdk.org
Mon Jul 10 18:18:31 UTC 2023
On Fri, 7 Jul 2023 06:45:46 GMT, Chen Liang <liach at 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 refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>
> Fix broken null behaviors
Looks good in general. Can you please add a test to verify that the hidden class is unloaded and then call `asInterfaceInstace` again on the same interface to spin a new class.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 185:
> 183: *
> 184: * The shared-class implementation is also closer in behavior to the original
> 185: * proxy-backed implementation. We might add another API for super-customized instances.
The implementor note mainly specifies that there is no guarantee on a stable mapping of the SAM interface to the implementation class.
I see this new note you added is to document the current implementation and alternatives. I would move this closer to the code (see below). I made some suggested edits. I avoid using the term "super-customized" since it's not clear what it means unless reading JBS comments. I also avoid referring to Project Leyden but instead describes it with some reasonable details say "more friendly for pre-generation....".
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 209:
> 207: mh = target;
> 208: }
> 209:
What about this comment rephrased from the new note you added:
// Define one hidden class for each interface. Create an instance of
// the hidden class for a given target method handle which will be
// accessed via getfield. Multiple instances may be created for a
// hidden class. This approach allows the generated hidden classes
// more shareable.
//
// An alternative approach is to define one hidden class with the
// target method handle as class data and the target method handle
// is loaded via ldc/condy. If more than one target method handles
// are used, the extra classes will pollute the same type profiles.
// In addition, hidden classes without class data is more friendly
// for pre-generation (shifting the dynamic class generation from
// runtime to an earlier phrase).
//
test/jdk/java/lang/invoke/MethodHandleProxies/BasicTest.java line 183:
> 181:
> 182: @Test
> 183: public void testModule() throws Throwable {
This test case belongs more to `ProxyForMethodHandleTest` test, which verifies if it's a dynamic module. We can move the package exports tests to `ProxyForMethodHandleTest`.
test/jdk/java/lang/invoke/MethodHandleProxies/ProxyForMethodHandleTest.java line 61:
> 59:
> 60: public static void assertDynamicModule(Module m, ClassLoader ld, Class<?> proxyClass) {
> 61: if (!m.isNamed() || !m.getName().startsWith("jdk.MHProxy")) {
This can also check if the package of the proxy class is not unconditionally exported.
test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 54:
> 52: } catch (Throwable ex) {
> 53: throw new AssertionError("Test failed for " + cl, ex);
> 54: }
Nit: formatting - try block inside the for-loop
Suggestion:
for (Class<?> cl : List.of(Runnable.class, Client.class, NestedInterface.class)) {
try {
Object o = MethodHandleProxies.asInterfaceInstance(cl, originalMh);
testWrapperInstanceTarget(o, originalMh);
testWrapperInstanceType(o, cl);
} catch (Throwable ex) {
throw new AssertionError("Test failed for " + cl, ex);
}
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/13197#pullrequestreview-1519328472
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1258671075
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1258671964
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1256560775
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1256562501
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1256158328
More information about the core-libs-dev
mailing list