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

[John R Rose]

On Thu, 20 Feb 2025 02:33:59 GMT, Chen Liang <liach 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.

You are on the right track.  Some of the details don’t look right to me, so I left comments on the PR.

 I realize it is tricky to get this stuff bootstrapped, and sometimes it helps to copy initialization code to avoid hitting a common path too soon.  But I think the right answer here is probably to lean hard on common paths in BMH, for both the zero and the constant cases.

Another matter:  The intrinsicData thingy bothers me.  Do we really need to use it?  Why not just have a clean identity NF, and then call NF(mycon) in a LF?

It looks like intrinsicData is under-used, and perhaps can be removed completely.  It only carries the length of a lookup table, before this PR…?  If so, I’d support lazily making N different NFs for table lookup, for each table arity in N. (I.e., N names, not one name, and no side-data.)  But maybe that would be equally bad.

Anyway, those are my first reactions.  Thanks!

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.

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)`.

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).

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.

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).

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?

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?

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

PR Review: https://git.openjdk.org/jdk/pull/23706#pullrequestreview-2634451086
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966311674
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966322804
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966326986
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966306954
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966339208
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966337750
PR Review Comment: https://git.openjdk.org/jdk/pull/23706#discussion_r1966338094