On Fri, 7 Jul 2023 06:45:46 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 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
