RFR: 6983726: remove Proxy from MethodHandleProxies.asInterfaceInstance SAM conversion [v6]

[Jorn Vernee]

On Thu, 6 Apr 2023 03:44:07 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 implements proxies with one hidden class for each requested interface and replaced WrapperInstance inheritance with an annotation. This can avoid unexpected passing of `instanceof`, and avoids the nasty problem of exporting a JDK interface to a dynamic module to ensure access.
>> 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 generated hidden classes has call performance on par with those of lambda expressions; the creation performance is slightly less than that of LambdaMetafactory: https://jmh.morethan.io/?gist=fcb946d83ee4ac7386901795ca49b224
>> 
>> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's no longer applicable. Tests in `jdk/java/lang/invoke` and `jdk/java/lang/reflect` pass.
>> 
>> In addition, I have a question: in [8161245](https://bugs.openjdk.org/browse/JDK-8161245) it seems some fields can be optimized as seen in [ciField.cpp](https://github.com/openjdk/jdk/blob/6aec6f3a842ead30b26cd31dc57a2ab268f67875/src/hotspot/share/ci/ciField.cpp#L219). Does it affect the execution performance of MethodHandle in hidden classes' Condy vs. MethodHandle in regular final field in hidden classes?
>> 
>> [^1]: single abstract method
>
> Chen Liang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Whitespace, cleanup, rename benchmarks to be informative

test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstanceCall.java line 131:

> 129:         interfaceInstance = MethodHandleProxies.asInterfaceInstance(Doable.class, target);
> 130:         lambda = (Doable) LambdaMetafactory.metafactory(LOOKUP, "doWork", MT_Doable, MT_int_int, target, MT_int_int).getTarget().invokeExact();
> 131:     }

The setup here can just re-use the constant* fields. The important part is that the benchmark methods load the value from a non-constant field (i.e. not static final).
Suggestion:

    @Setup
    public void setup() throws Throwable {
        target = constantTarget;
        doable = constantDoable;
        handle = constantHandle;
        interfaceInstance = constantInterfaceInstance;
        lambda = constantLambda;
    }

test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstanceCall.java line 176:

> 174:     public void constantLambda() {
> 175:         i = constantLambda.doWork(i);
> 176:     }

I think setting the result into an instance field like this can work, but it's imo better to let JMH handle it. So, these methods should just return the value instead of writing it to the `i` field.

test/micro/org/openjdk/bench/java/lang/invoke/MethodHandleProxiesAsIFInstanceCreate.java line 86:

> 84:         i = doable.doWork(i);
> 85:         return doable;
> 86:     }

In this case, instead of writing to `i`, the benchmark method can accept a `Blackhole` argument and use that to consume the value.
Suggestion:

    public Doable createCallLambda(Blackhole bh) throws Throwable {
        Doable doable = (Doable) LambdaMetafactory.metafactory(LOOKUP, "doWork", MT_Doable, MT_int_int, target, MT_int_int).getTarget().invokeExact();
        bh.consume(doable.doWork(i));
        return doable;
    }

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1160054577
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1160056394
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1160059392