8350607: Consolidate MethodHandles::zero into MethodHandles::constant #23706

liach · 2025-02-20T02:33:59Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8350607: Consolidate MethodHandles::zero into MethodHandles::constant (Enhancement - P4)

Reviewers

John R Rose (@rose00 - Reviewer)
Claes Redestad (@cl4es - Reviewer)
Jorn Vernee (@JornVernee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23706/head:pull/23706
$ git checkout pull/23706

Update a local copy of the PR:
$ git checkout pull/23706
$ git pull https://git.openjdk.org/jdk.git pull/23706/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23706

View PR using the GUI difftool:
$ git pr show -t 23706

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23706.diff

Using Webrev

Link to Webrev Comment

LF editor spins classes

bridgekeeper · 2025-02-20T02:34:47Z

👋 Welcome back liach! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-20T02:35:32Z

@liach This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: jrose, redestad, jvernee

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 174 new commits pushed to the master branch:

c3b4819: 8351074: Disallow null prefix and suffix in DecimalFormat
6012e8d: 8350808: Small typos in JShell method SnippetEvent.toString()
661bd5b: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs)
61d9ab9: 8350854: Include thread counts in safepoint logging
ea9e3cf: 8281511: java/net/ipv6tests/UdpTest.java fails with checkTime failed
caaf409: 8350546: Several java/net/InetAddress tests fails UnknownHostException
062b7c7: 8351115: Test AOTClassLinkingVMOptions.java fails after JDK-8348322
a88e8cd: 8350952: Remove some non present files from OPT_SPEED_SRC list
de29ef3: 8343191: Cgroup v1 subsystem fails to set subsystem path
75f028b: 8348657: compiler/loopopts/superword/TestEquivalentInvariants.java timed out
... and 164 more: https://git.openjdk.org/jdk/compare/92efab90db24a76cc28fc1ae1db870a0dd670266...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-02-20T02:35:52Z

@liach The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

rose00

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!

rose00 · 2025-02-21T23:09:38Z

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java

@@ -1343,7 +1343,7 @@ enum Intrinsic {
        ARRAY_STORE,
        ARRAY_LENGTH,
        IDENTITY,
-        ZERO,
+        CONSTANT,
        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?

we should just select a MH via hash table lookup and invoke that MH

I had something like this in an early prototype of the tableSwitch combinator, but it does not work, as it prevents the method handle calls for each case from being inlined.

FYI this is being addressed in #23763

src/java.base/share/classes/java/lang/invoke/LambdaForm.java

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

src/java.base/share/classes/java/lang/invoke/MethodTypeForm.java

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java

liach · 2025-02-24T22:42:30Z

I think this version should be fine: Perfstartup-LambdaNoop's MethodHandle.constant instructions have decreased from 99032 to 48507 by eliding the generation of a LF via LambdaFormEditor. Tests seem to be fine too.

mlbridge · 2025-02-24T22:56:02Z

Webrevs

01: Full - Incremental (beee1ad6)
00: Full (6f5c8873)

cl4es

Just got back home. Some comments inline, will need to run some tests and mull this over before approval.

cl4es · 2025-02-25T21:38:58Z

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java

+        var basicType = BasicType.basicType(type);
+        var form = constantForm(basicType);
+
+        if (type.isPrimitive()) {


I think you could simplify this using Wrapper.forBasicType; all variants should be able to use wrapper.convert

The cast is not performed if the type is an interface; this is a behavioral disparity with the old value = ptype.cast(value); in insertArguments.

Sure, we'd need an explicit cast in the OBJECT case and that would be a bit redundant. Might still amount to a code simplification.

cl4es · 2025-02-25T21:49:43Z

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

@@ -4881,7 +4870,8 @@ public static MethodHandle identity(Class<?> type) {
     */
    public static MethodHandle zero(Class<?> type) {
        Objects.requireNonNull(type);
-        return type.isPrimitive() ?  zero(Wrapper.forPrimitiveType(type), type) : zero(Wrapper.OBJECT, type);
+        return type.isPrimitive() ? primitiveZero(Wrapper.forPrimitiveType(type))
+                : MethodHandleImpl.makeConstantReturning(type, null);


Not sure if it's important but the existing impl would cache zero(Object.class) while this new impl won't. Behaviorally neutral for any other reference type, though.

type == Object.class can use the primitive path, but I am too lazy to add another of this check.

JornVernee · 2025-03-04T11:56:50Z

src/java.base/share/classes/java/lang/invoke/LambdaForm.java

+        var species = SimpleMethodHandle.BMH_SPECIES.extendWith(type);
+        var carrier = argument(0, L_TYPE).withConstraint(species); // BMH bound with data
+        Name[] constNames = new Name[] { carrier, new Name(species.getterFunction(0), carrier) };
+        return LF_constant[type.ordinal()] = create(1, constNames, Kind.CONSTANT);


I think this caching logic should be in constantForm, which also does the cache lookup.

Putting the field write in the create method is to help reduce the code that JIT needs to compile by reducing the getter's code size.

Fair enough.

JornVernee · 2025-03-04T13:32:54Z

src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java

@@ -969,7 +969,7 @@ LambdaForm filterReturnForm(BasicType newType, boolean constantZero) {
            if (newType == V_TYPE)
                callFilter = null;
            else
-                callFilter = new Name(constantZero(newType));
+                callFilter = new Name(LambdaForm.identity(newType), newType.btWrapper.zero());


Maybe we could have a Name constructor that just takes an Object and returns it like this.

I don't think that is necessary - the majority of constant values like this are immediately sent to a NamedFunction, which already accepts such constants natively. The only case such a Name is necessary is for LF return values.

JornVernee · 2025-03-04T13:35:50Z

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java

@@ -1343,7 +1343,7 @@ enum Intrinsic {
        ARRAY_STORE,
        ARRAY_LENGTH,
        IDENTITY,
-        ZERO,
+        CONSTANT,
        NONE // no intrinsic associated


we should just select a MH via hash table lookup and invoke that MH

I had something like this in an early prototype of the tableSwitch combinator, but it does not work, as it prevents the method handle calls for each case from being inlined.

JornVernee · 2025-03-04T13:39:21Z

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/TestDynamicRegenerateHolderClasses.java

@@ -39,7 +39,6 @@
 public class TestDynamicRegenerateHolderClasses extends DynamicArchiveTestBase {
    static String CHECK_MESSAGES[] = {"java.lang.invoke.Invokers$Holder source: shared objects file (top)",
                                      "java.lang.invoke.DirectMethodHandle$Holder source: shared objects file (top)",
-                                      "java.lang.invoke.DelegatingMethodHandle$Holder source: shared objects file (top)",


I don't understand why this change is needed. Could you please explain it?

With the simplification of MethodHandles.constant, it seems invoking MethodHandles.constant no longer need to initialize DelegatingMethodHandle because there is no longer need to rebind some MethodHandle; anyways we are loading fewer classes now.

liach · 2025-03-04T17:56:02Z

@cl4es @rose00 I think I have addressed your existing comments; can you review again?

rose00

I hope that the intrinsic mechanism can be further simplified, at some point. But I defer to Jorn's comments about tableswitch.

Excellent cleanups. Lots and lots of deleted code, and other code regularized. Thank you!

rose00 · 2025-03-04T20:36:02Z

src/java.base/share/classes/java/lang/invoke/SimpleMethodHandle.java

@@ -32,6 +32,8 @@

 /**
 * A method handle whose behavior is determined only by its LambdaForm.
+ * Access to SimpleMethodHandle should ensure BoundMethodHandle is initialized
+ * first.


Did you try factoring UNSAFE.ensureInit(BMH) into a static block in SimpleMethodHandle.java?
Sometimes that works.

To be exact, I did not encounter any initialization issue during my use of BMH/SimpleMH. We don't need ensureInit as SimpleMH already extends BMH. Maybe my concern is just red herring, as I saw a warning on BMH$Species_L that its access should go through BMH first.

cl4es · 2025-03-05T18:56:12Z

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java

+        var basicType = BasicType.basicType(type);
+        var form = constantForm(basicType);
+
+        if (type.isPrimitive()) {


Sure, we'd need an explicit cast in the OBJECT case and that would be a bit redundant. Might still amount to a code simplification.

liach · 2025-03-07T20:34:44Z

Tier 1-3 tests only have 2 unrelated failures. Thanks for the reviews.

/integrate

openjdk · 2025-03-07T20:35:38Z

Going to push as commit 8ed6c1d.
Since your change was applied there have been 196 commits pushed to the master branch:

f6a8db2: 8348261: assert(n->is_Mem()) failed: memory node required
5cd4fe6: 8348309: MultiNST tests need more debugging and timing
7c22b81: 8350811: [JMH] test foreign.StrLenTest failed with StringIndexOutOfBoundsException for size=451
54fe643: 8347433: Deprecate XML interchange in java.management/javax/management/modelmbean/DescriptorSupport for removal
155697f: 8349623: [ASAN] Gtest os_linux.glibc_mallinfo_wrapper_vm fails
4066f33: 8350565: NMT: remaining memory flag/type to be replaced with memory tag
7314efc: 8351377: Fix the ProblemList for com/sun/management/OperatingSystemMXBean cpuLoad tests on AIX
7c9a784: 8351223: Update localized resources in keytool and jarsigner
a23fb0a: 8348110: Update LCMS to 2.17
0892913: 8351086: (fc) Make java/nio/channels/FileChannel/BlockDeviceSize.java test manual
... and 186 more: https://git.openjdk.org/jdk/compare/92efab90db24a76cc28fc1ae1db870a0dd670266...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-07T20:35:44Z

@liach Pushed as commit 8ed6c1d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

ExE-Boss · 2025-05-17T14:32:43Z

src/java.base/share/classes/java/lang/invoke/MethodHandles.java

@@ -4900,7 +4890,7 @@ private static MethodHandle identityOrVoid(Class<?> type) {
     * @param type the type of the desired method handle
     * @return a constant method handle of the given type, which returns a default value of the given return type
     * @throws NullPointerException if the argument is null
-     * @see MethodHandles#zero
+     * @see MethodHandles#primitiveZero


MethodHandles.primitiveZero(…) is a private internal API.

Suggested change

* @see MethodHandles#primitiveZero

* @see MethodHandles#zero

There's a general rfe for checking inaccessible links. Don't recall why thisv was even changed, most likely ide being too smart.

Special LF treatment for MHs.constant

16df141

LF editor spins classes

openjdk bot added the core-libs [email protected] label Feb 20, 2025

liach added 2 commits February 20, 2025 19:49

Eliminate zero form

2f8d7b3

Years

7de1d0f

rose00 reviewed Feb 22, 2025

View reviewed changes

liach added 2 commits February 24, 2025 16:10

Simplify constant making

723151a

Years, roll back unrelated changes

6f5c887

liach changed the title ~~Special LF treatment for MHs.constant~~ 8350607: Consolidate MethodHandles::zero into MethodHandles::constant Feb 24, 2025

liach marked this pull request as ready for review February 24, 2025 22:52

openjdk bot added the rfr Pull request is ready for review label Feb 24, 2025

We no longer load DelegateMH as we no longer rebind

beee1ad

graalvmbot mentioned this pull request Feb 25, 2025

[JDK-8350639] Adapt JDK-8350607: Consolidate MethodHandles::zero into MethodHandles::constant oracle/graal#10754

Closed

cl4es reviewed Feb 25, 2025

View reviewed changes

JornVernee reviewed Mar 4, 2025

View reviewed changes

JornVernee approved these changes Mar 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 4, 2025

rose00 approved these changes Mar 4, 2025

View reviewed changes

cl4es approved these changes Mar 5, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 7, 2025

openjdk bot closed this Mar 7, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 7, 2025

graalvmbot mentioned this pull request Mar 14, 2025

[GR-63061] Update labsjdk to 25+14-jvmci-b01 oracle/graal#10858

Merged

ExE-Boss suggested changes May 17, 2025

View reviewed changes

ExE-Boss mentioned this pull request May 18, 2025

8356632: Fix remaining {@link/@linkplain} tags with refer to private/protected types in java.base #25287

Closed

3 tasks

8350607: Consolidate MethodHandles::zero into MethodHandles::constant #23706

8350607: Consolidate MethodHandles::zero into MethodHandles::constant #23706

Uh oh!

Conversation

liach commented Feb 20, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 20, 2025

Uh oh!

openjdk bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 20, 2025

Uh oh!

rose00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liach commented Feb 24, 2025

Uh oh!

mlbridge bot commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

cl4es left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach commented Mar 4, 2025

Uh oh!

rose00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

liach commented Feb 20, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 20, 2025 •

edited

Loading

mlbridge bot commented Feb 24, 2025 •

edited

Loading