On Fri, 21 Feb 2025 23:17:50 GMT, John R Rose <jr...@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
