RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v11]

[John R Rose]

On Mon, 1 May 2023 14:56:27 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 a check to the class data. 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.
>> 
>> [^1]: single abstract method
>
> Chen Liang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 24 additional commits since the last revision:
> 
>  - Renames
>  - Merge branch 'master' into explore/mhp-iface
>  - Consolidate iteration over public methods
>  - Define MH proxy class in a dynamic module
>  - Fix test that depend on WrapperInstance
>  - Require an original lookup in constructor arguments to prevent unintended instantiation
>  - pass interface info via condy,
>    drop explicit ea flags
>    initialize benchmark work variable randomly
>    benchmarks just need 1 fork
>  - Nuke WrapperInstance
>  - Merge branch 'master' into explore/mhp-iface
>  - Whitespace, cleanup, rename benchmarks to be informative
>  - ... and 14 more: https://git.openjdk.org/jdk/compare/c839bed1...f5d0ef0a

I wonder if we should, in this API point, try harder to avoid spinning so many HCs.

I’ve been thinking a lot about Leyden lately, and HCs are (in their current usage) a problem for Leyden, because they are hard to characterize in static analysis; they tend to behave like opaque “live objects” even if they are mostly just customized code.  I have a number of possible fixes in mind, but the common thread is to try to make fewer of them, one per “code shape” if possible.  (Whatever “code shape” ends up being!)

I think you could do this by having the HC have a final non-static field for each required MH (one per overload point); each os an asType adaptation of the original MH.  The actual code of each HC implementation method would be a single invokeVirtual of a strongly typed `MH::invokeExact` call.  This is the code shape in the current PR (see the `createTemplate` method) *except* that I am saying that the MH in question should come from `getfield` not `ldc/condy`.

HC fields are supposed to fold up; if they don’t we can help them in this case.  (That is, constant folding of the MH pointer should not be a roadblock; that’s on the JIT to fix by adjusting the `TrustFinalFields` logic.)

The only other method that needs adjustment is `<init>`.  That needs a series of `asType` calls (on `ldc/MethodType`) followed by `putField`.

I see that the error-checking logic in `<init>` is hand-coded assembly code.  Please, let’s not do that.  The code should call a consolidated helper method that handles all error logic, out of line.  The helper method should also be split into (a) parts that run every time, and (b) parts that almost never run, which make and throw an exception.

One reason to avoid assembly is readability/maintainability.  Another reason is that the JIT actually prefers smaller methods bodies, which is why I say you should code that stuff out of line (in as maintainable Java helper).

So the HC should look like:


__Hidden class HC implements IFace {
private final MethodHandle mh1,  mh2…;

HC(Lookup lookup, MethodHandle target) {
  SomeHelperClass.checkAccess(lookup, IFace.class);
  mh1 = target.asType(LDC[ methodType( mh1Info.desc) ]);
  //init mh2...
}

@Override R1 iFaceMethod(A1a a1a, …) {
  return (R1) mh1.invokeExact(a1a, …);  //lowers to invokeBasic
}

//declare mh2...
}


It shouldn’t be necessary to have a class-data for the HC, in this case.  This would make the HC more shareable and support pregeneration in Leyden.

Did I miss a problem with this approach?

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

PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1530257003