RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v19]
Mandy Chung
mchung at openjdk.org
Fri Jun 30 21:34:27 UTC 2023
On Thu, 29 Jun 2023 03:16:15 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 updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 41 commits:
>
> - 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
> - Merge branch 'master' into explore/mhp-iface
> - Merge branch 'master' into explore/mhp-iface
> - stage, needs to fix is mh proxy instance check
> - ... and 31 more: https://git.openjdk.org/jdk/compare/6f58ab2b...340b0751
This looks quite good. I have some comments below. Also pushed some of my suggestion to my [mh-proxies](https://github.com/mlchung/jdk/tree/mh-proxies) branch.
I'd like to have a test to verify that `isWrapperInstance` and other `wrapperXXX` methods determine a random object whose class happens to have the same fields as the wrapper class expects but not a wrapper instance.
I will review BasicTest in details next.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 226:
> 224: * The hidden classes defined for I is defined in a dynamic module M
> 225: * which has access to the types referenced by the members of I including
> 226: * the parameter types, return type and exception types.
This comment can be moved to where it creates the dynamic module.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 241:
> 239: }
> 240:
> 241: private record LocalMethodInfo(MethodTypeDesc desc, List<ClassDesc> thrown, String fieldName) {}
Perhaps a simpler name `MethodInfo`
Suggestion:
private record MethodInfo(MethodTypeDesc desc, List<ClassDesc> thrown, String fieldName) {}
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 247:
> 245:
> 246: private static final Set<Class<?>> WRAPPER_TYPES = Collections.newSetFromMap(new WeakHashMap<>());
> 247: private static final ClassValue<SoftReference<Lookup>> PROXY_LOOKUPS = new ClassValue<>() {
Let's use weak references for this implementation so that the hidden classes can be unloaded when it becomes weakly reachable. We can reevaluate this when we get more feedback on this usage.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 265:
> 263: continue;
> 264:
> 265: String mname = m.getName();
Better documentation, more comments, would be helpful. Some suggestions below.
Suggestion:
// ensure it's SAM interface
String mname = m.getName();
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 284:
> 282: Stream.concat(DEFAULT_RETHROWS.stream(),
> 283: Arrays.stream(thrown).map(MethodHandleProxies::desc)).distinct().toList();
> 284: methods.add(new LocalMethodInfo(mtDesc, exceptionTypeDescs, fieldName));
Suggestion:
var exceptionTypeDescs =
thrown.length == 0 ? DEFAULT_RETHROWS
: Stream.concat(DEFAULT_RETHROWS.stream(),
Arrays.stream(thrown).map(MethodHandleProxies::desc))
.distinct().toList();
methods.add(new MethodInfo(desc(mt), exceptionTypeDescs, fieldName));
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 297:
> 295:
> 296: var loader = intfc.getClassLoader();
> 297: Module targetModule = newDynamicModule(loader, referencedTypes);
Suggestion:
// create a dynamic module for each proxy class, which needs access
// to the types referenced by the members of the interface including
// the parameter types, return type and exception types
var loader = intfc.getClassLoader();
Module targetModule = newDynamicModule(loader, referencedTypes);
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 306:
> 304: ClassDesc proxyDesc = ClassDesc.of(cn);
> 305: byte[] template = createTemplate(loader, proxyDesc, desc(intfc), uniqueName, methods);
> 306: var definer = new Lookup(intfc).makeHiddenClassDefiner(cn, template, Set.of(), DUMPER);
Suggestion:
// define the dynamic module to the class loader of the interface
var definer = new Lookup(intfc).makeHiddenClassDefiner(cn, template, Set.of(), DUMPER);
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 336:
> 334: private static final ClassDesc CD_IllegalAccessException = desc(IllegalAccessException.class);
> 335: private static final MethodTypeDesc MTD_void_Throwable = MethodTypeDesc.of(CD_void, CD_Throwable);
> 336: private static final MethodType MT_void_Lookup_MethodHandle_MethodHandle = methodType(void.class, Lookup.class, MethodHandle.class, MethodHandle.class);
Several of these MethodType declarations are long lines that can be wrapped.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 347:
> 345: private static final MethodTypeDesc MTD_void_String = MethodTypeDesc.of(CD_void, CD_String);
> 346: private static final String ORIGINAL_TARGET_NAME = "originalTarget";
> 347: private static final String ORIGINAL_TYPE_NAME = "originalType";
Can drop "original" prefix?
Suggestion:
private static final String TARGET_NAME = "target";
private static final String TYPE_NAME = "interfaceType";
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 367:
> 365: clb.withInterfaceSymbols(ifaceDesc);
> 366:
> 367: // individual handle fields
Suggestion:
// static and instance fields
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 381:
> 379: });
> 380:
> 381: // <init>(Lookup, MethodHandle originalHandle, MethodHandle implHandle)
Suggestion:
// <init>(Lookup, MethodHandle target, MethodHandle callerBoundTarget)
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 386:
> 384: cob.invokespecial(CD_Object, INIT_NAME, MTD_void);
> 385:
> 386: // WrapperInstance.ensureOriginalLookup
Suggestion:
// call ensureOriginalLookup to verify the given Lookup has access
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 391:
> 389:
> 390: // keep original target
> 391: // this.originalTarget = originalHandle;
Suggestion:
// this.target = target;
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 396:
> 394: cob.putfield(proxyDesc, ORIGINAL_TARGET_NAME, CD_MethodHandle);
> 395:
> 396: // convert individual handles
Suggestion:
// adjust the callerBoundTarget to the method type for each method
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 398:
> 396: // convert individual handles
> 397: for (var mi : methods) {
> 398: // this.handleField = implHandle.asType(xxType);
Suggestion:
// this.m<i> = callerBoundTarget.asType(xxType);
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 410:
> 408: });
> 409:
> 410: clb.withMethodBody(ENSURE_ORIGINAL_LOOKUP, MTD_void_Lookup, ACC_PRIVATE | ACC_STATIC, cob -> {
Suggestion:
+ // void ensureOriginalLookup(Lookup) checks if the given Lookup has ORIGINAL
+ // access to this class, i.e. the lookup class is this class; otherwise,
+ // IllegalAccessException is thrown
clb.withMethodBody(ENSURE_ORIGINAL_LOOKUP, MTD_void_Lookup, ACC_PRIVATE | ACC_STATIC, cob -> {
test/jdk/java/lang/invoke/MethodHandleProxies/ProxyForMethodHandleTest.java line 34:
> 32: import org.junit.jupiter.api.Test;
> 33:
> 34: /*
Add `@bug` tag
test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 37:
> 35: public static void main(String... args) {
> 36: var originalMh = MethodHandles.zero(void.class);
> 37: Object o = MethodHandleProxies.asInterfaceInstance(Untrusted.class, originalMh);
Is it better to define an interface in this test? It seems that this test has nothing to do with Untrusted. It may confuse readers.
test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 42:
> 40: assert originalMh == untrustedTarget : "Got " + untrustedTarget;
> 41:
> 42: var runnableTarget = MethodHandleProxies.wrapperInstanceTarget(MethodHandleProxies.asInterfaceInstance(Runnable.class, originalMh));
please break this long line. Perhaps adding a local variable may make it easy to read too.
test/jdk/java/lang/invoke/MethodHandleProxies/WithSecurityManagerTest.java line 42:
> 40: assert originalMh == untrustedTarget : "Got " + untrustedTarget;
> 41:
> 42: var runnableTarget = MethodHandleProxies.wrapperInstanceTarget(MethodHandleProxies.asInterfaceInstance(Runnable.class, originalMh));
Just to be completed, also add test cases for `isWrapperInstance` and `wrapperInstanceType`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13197#pullrequestreview-1507889870
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248283212
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248281976
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248281584
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248285455
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248284374
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248285933
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248286077
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248287026
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248287738
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289389
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289527
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289943
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248290172
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248290731
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248290809
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248289730
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248292045
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248294487
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248292958
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1248295035
More information about the core-libs-dev
mailing list