RFR: 8350607: Consolidate MethodHandles::zero into MethodHandles::constant

[Chen Liang]

On Fri, 21 Feb 2025 23:17:50 GMT, John R Rose <jrose at openjdk.org> wrote:

>> LF editor spins classes, this avoids the spinning overhead and should speed up non-capturing lambdas too.
>> 
>> There may need to be additional intrinsic work for MH combinator lf bytecode generation.
>
> src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1653:
> 
>> 1651:     }
>> 1652: 
>> 1653:     private static LambdaForm computeConstantForm(BasicType type) {
> 
> I think the word "create" is more conventional, at this point, than "compute".
> When you are finding some lazily computed resource, you create it if not present.
> There is "computeInvoker" and "computeValueConversions" elsewhere, but I think "create" is more frequent.

Renamed to `createConstantForm`.

> src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1665:
> 
>> 1663:         if (cached != null)
>> 1664:             return cached;
>> 1665:         return BASIC_SPECIES[ord] = SimpleMethodHandle.BMH_SPECIES.extendWith(type);
> 
> It's not clear to me what `BASIC_SPECIES` means here.  BMH species exist which correspond to all non-empty tuples of bound types.  So maybe what you are caching here is unary BMH species?  In that case `UNARY_BMH_SPECIES` might be more to the point.  Even better, place that array in BMH.java itself, call it `BoundMethodHandle.UNARY_SPECIES`, and give it a name `BoundMethodHandle.unarySpeciesData(BasicType)`.

I just noticed `BMH$SpeciesData.extensions` is a stable array, so we can just query `SimpleMethodHandle.BMH_SPECIES` (aka `BMH.SPECIALIZER.topSpecies`)'s `extendWith` and use that stable array instead.

> src/java.base/share/classes/java/lang/invoke/LambdaForm.java line 1691:
> 
>> 1689: 
>> 1690:             // Look up symbolic names.  It might not be necessary to have these,
>> 1691:             // but if we need to emit direct references to bytecodes, it helps.
> 
> Rename `createFormsFor` to `createIdentityForm`, since it now only creates that one thing (plus its NF).

Done.

> src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1347:
> 
>> 1345:         IDENTITY,
>> 1346:         CONSTANT,
>> 1347:         NONE // no intrinsic associated
> 
> I notice you are using the `intrinsicData` property to keep track of the constant, which is a good call.
> 
> A little below here, around line 1400, there is a possible bug in the handling of `intrinsicData`.
> 
> Possible fix:
> 
> 
> --- a/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> +++ b/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
> @@ -1409,7 +1409,8 @@ static MethodHandle makeIntrinsic(MethodHandle target, Intrinsic intrinsicName)
>      }
>  
>      static MethodHandle makeIntrinsic(MethodHandle target, Intrinsic intrinsicName, Object intrinsicData) {
> -        if (intrinsicName == target.intrinsicName())
> +        if (intrinsicName == target.intrinsicName() &&
> +            intrinsicData == target.intrinsicData())
>              return target;
>          return new IntrinsicMethodHandle(target, intrinsicName, intrinsicData);
>      }
> 
> 
> It should be added to this PR, lest constants get confused somehow.

I reviewed the other use of `intrinsicData`, `tableSwitch`. I believe the intrinsic is actually a regression by growing the bytecode size - we should just select a MH via hash table lookup and invoke that MH, given all MHs in that list have the same type. I have removed the use of intrinsic data here and we can move on to remove it later.

I noticed that intrinsics are useful really only as part of named functions. And named functions only reuse arbitrary MHs for the invoker form. Is my understanding here correct?

> src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 2307:
> 
>> 2305:     }
>> 2306: 
>> 2307:     static MethodHandle makeConstantI(Wrapper type, int value) {
> 
> After going over the rest of the patch, I don't think these new factories pull their weight.
> 
> They just duplicate what the BMH species factories do when adding the first bound value.
> 
> If the BMH factories don't do this job, they should be changed so they can handle it.
> 
> In other words, the details of BMH factory invocation should be kept inside the BMH files (or SMH, which is the null case of BMH).

Addressed in the same way as for https://github.com/openjdk/jdk/pull/23706#discussion_r1968478226

> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4946:
> 
>> 4944:         } else if (bt == BasicType.D_TYPE) {
>> 4945:             return MethodHandleImpl.makeConstantD(0.0D);
>> 4946:         }
> 
> This if/then/else over basic types is unpleasantly messy, just to handle one-time creation for a cached zero MH.
> 
> I suggest using `bt.basicTypeWrapper().zero()` to get the zero value generically, and then use generic binding logic such as `MHs::insertArguments` or `insertArgumentPrimitive`, just like `MHs::constant` does.  Calling `makeConstantJ` and siblings seems like a code smell.  Is there a way to get rub of `makeConstantX` and instead generalize the `insertArguments` protocols which creates BMHs?

I moved the binding logic to be with `BMH::bindSingle` and renamed the original bindSingle to be bindSingleL. It should reduce code smell a little bit; will push soon ™

> src/java.base/share/classes/java/lang/invoke/MethodTypeForm.java line 75:
> 
>> 73:             LF_INVINTERFACE            =  4,
>> 74:             LF_INVSTATIC_INIT          =  5,  // DMH invokeStatic with <clinit> barrier
>> 75:             LF_INTERPRET               =  6,  // LF interpreter, only its vmentry is used
> 
> Unrelated change here?

Indeed, however its dummy LF used the zero NF and required some updates :( Wish we can just cache it as a MemberName

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968532132
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968472249
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968500456
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968485136
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968479670
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968478226
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1968479177