RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v11]

[Johannes Kuhn]

On Mon, 1 May 2023 21:00:40 GMT, John R Rose <jrose at openjdk.org> wrote:

>> 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/48d60d79...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?

@rose00  I was also thinking about if we should spin an own instance for every `MethodHandle`.

An other evaluation criteria is: How do other approaches behave, and what constrains do they have?  
Some alternatives are: lambda (interface is statically known) or invoking `LambdaMetaFactory` with `MethodHandles::invokeExact` as target.
If the type is not statically known, then a lambda can't be used, making it unlikely for the JIT to inline the invokeinterface call as well.

Final fields of hidden classes (and records) are already trusted by the JIT.
But for the JIT to constant fold, it has to inline the invokeinterface target.

So considering the use case of `MethodHandleProxies.asInterfaceInstance` should be taken into account.

------

About checking stuff & helper methods:

A problem with classes that are possibly defined in user modules is:  
Where should we put the check methods?  
If the hidden class can see it, then user code could use it, and this either requires adding a public method as part of the API, or just write it in bytecode in the same class.

This is also done in `java.lang.reflect.Proxy` - the checks if the proxy should release its lookup are written in bytecode.  
Maybe it might be useful to add such an `assertOriginalLookupOf(Lookup l, Class<?> lookupClass)` somewhere.

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

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