8332522: SwitchBootstraps::mappedEnumLookup constructs unused array #19906

lahodaj · 2024-06-26T12:32:20Z

For general pattern matching switches, the SwitchBootstraps class currently generates a cascade of if-like statements, computing the correct target case index for the given input.

There is one special case which permits a relatively easy faster handling, and that is when all the case labels case enum constants (but the switch is still a "pattern matching" switch, as tranditional enum switches do not go through SwitchBootstraps). Like:

enum E {A, B, C}
E e = ...;
switch (e) {
     case null -> {}
     case A a -> {}
     case C c -> {}
     case B b -> {}
}

We can create an array mapping the runtime ordinal to the appropriate case number, which is somewhat similar to what javac is doing for ordinary switches over enums.

The SwitchBootstraps class was trying to do so, when the restart index is zero, but failed to do so properly, so that code is not used (and does not actually work properly).

This patch is trying to fix that - when all the case labels are enum constants, an array mapping the runtime enum ordinals to the case number will be created (lazily), for restart index == 0. And this map will then be used to quickly produce results for the given input. E.g. for the case above, the mapping will be {0 -> 0, 1 -> 2, 2 -> 1} (meaning {A -> 0, B -> 2, C -> 1}).

When the restart index is != 0 (i.e. when there's a guard in the switch, and the guard returned false), the if cascade will be generated lazily and used, as in the general case. If it would turn out there are significant enum-only switches with guards/restart index != 0, we could improve there as well, by generating separate mappings for every (used) restart index.

I believe the current tests cover the code functionally sufficiently - see SwitchBootstrapsTest.testEnums. It is only that the tests do not (and regression tests cannot easily, I think) differentiate whether the special-case or generic implementation is used.

I've added a new microbenchmark attempting to demonstrate the difference. There are two benchmarks, both having only enum constants as case labels. One, enumSwitchTraditional is an "old" switch, desugared fully by javac, the other, enumSwitchWithBootstrap is an equivalent switch that uses the SwitchBootstraps. Before this patch, I was getting values like:

Benchmark                           Mode  Cnt   Score   Error  Units
SwitchEnum.enumSwitchTraditional    avgt   15  11.719 ± 0.333  ns/op
SwitchEnum.enumSwitchWithBootstrap  avgt   15  24.668 ± 1.037  ns/op

and with this patch:

Benchmark                           Mode  Cnt   Score   Error  Units
SwitchEnum.enumSwitchTraditional    avgt   15  11.550 ± 0.157  ns/op
SwitchEnum.enumSwitchWithBootstrap  avgt   15  13.225 ± 0.173  ns/op

So, this seems like a clear improvement to me.

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-8332522: SwitchBootstraps::mappedEnumLookup constructs unused array (Bug - P4)

Reviewers

Claes Redestad (@cl4es - Reviewer) 🔄 Re-review required (review applies to 8ab9ee64)
Chen Liang (@liach - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19906

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-26T12:33:09Z

👋 Welcome back jlahoda! 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 · 2024-06-26T12:33:47Z

@lahodaj 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:

8332522: SwitchBootstraps::mappedEnumLookup constructs unused array

Reviewed-by: liach, redestad

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 211 new commits pushed to the master branch:

4f312d6: 8336152: Remove unused forward declaration in classLoadInfo.hpp
34d8562: 8335902: Parallel: Refactor VM_ParallelGCFailedAllocation and VM_ParallelGCSystemGC
2fc7eb4: 8155030: The Menu Mnemonics are always displayed for GTK LAF
559826c: 8332474: Tighten up ToolBox' JavacTask to not silently accept javac crash as a failure
eec0e15: 8335619: Add an @APinote to j.l.i.ClassFileTransformer to warn about recursive class loading and ClassCircularityErrors
9b6f6c5: 8336082: Fix -Wzero-as-null-pointer-constant warnings in SimpleCompactHashtable
7a62032: 8336081: Fix -Wzero-as-null-pointer-constant warnings in JVMTypedFlagLimit ctors
f677b90: 8267887: RMIConnector_NPETest.java fails after removal of RMI Activation (JDK-8267123)
1fe3ada: 8336284: Test TestClhsdbJstackLock.java/TestJhsdbJstackLock.java fails with -Xcomp after JDK-8335743
c703d29: 8335710: serviceability/dcmd/vm/SystemDumpMapTest.java and SystemMapTest.java fail on Linux Alpine after 8322475
... and 201 more: https://git.openjdk.org/jdk/compare/57f8b91e558e5b9ff9c2000b8f74e3a1988ead2b...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 · 2024-06-26T12:34:12Z

@lahodaj 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.

mlbridge · 2024-06-26T12:37:22Z

Webrevs

cl4es · 2024-06-26T12:46:18Z

test/micro/org/openjdk/bench/java/lang/runtime/SwitchEnum.java

+        int sum = 0;
+        for (E e : inputs) {
+            sum += switch (e) {
+                case null -> -1;


As this null case adds a case relative to the -Traditional test then maybe removing one of the E0, E1, ... cases would make the test a little bit more apples-to-apples?

Using case null -> will push javac to use the new code, but all switches do some kind of null check for the selector value. The difference is that if there's no case null, there will be Objects.requireNonNull generated for the selector value (which will throw an NPE if the value is null), while here it will jump to the given case.

So, case null does not have the same weight as a normal case, so I don't think it would be fair to remove a full case to compensate for it.

Fair enough, and I guess ~1.6ns/op is reasonable overhead for the added semantics.

cl4es · 2024-06-26T12:56:04Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

@@ -289,20 +280,16 @@ public static CallSite enumSwitch(MethodHandles.Lookup lookup,
        labels = Stream.of(labels).map(l -> convertEnumConstants(lookup, enumClass, l)).toArray();


You could create EnumDesc[] enumDescLabels here and remove the Arrays.copyOf on line 290. The labels.clone() on line 277 also seem redundant since we only iterate over the labels argument once.

While this case is likely fine I generally recommend using a minimal amount of streams/lambdas in bootstrap sensitive code like these dynamic bootstraps methods.

liach · 2024-06-26T12:57:09Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

        @Stable
-        public int[] map;
+        public volatile MethodHandle generatedSwitch;


We probably don't need these 2 volatiles:

Arrays are already safely published (See https://bugs.openjdk.org/browse/JDK-8333791?focusedId=14680594&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14680594) and we can only access array elements after the array is fully initialized then published. Thus it's a safe publication, and reads don't require explicit volatile read.

MethodHandle is immutable and safely published, thus volatile read is redundant as well.

liach · 2024-06-26T13:37:51Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

+                        generatedSwitch =
+                                generateTypeSwitch(lookup, enumClass, labels)
+                                        .asType(MethodType.methodType(int.class,
+                                                                      enumClass,


We might have to do Object.class so we can call invokeExact below

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

….java Co-authored-by: Chen Liang <liach@openjdk.org>

ExE-Boss

Since labels is no longer eagerly cloned, it’s important to store the converted labels into a local array to avoid leaking them to user code.

ExE-Boss · 2024-06-27T19:03:58Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

+        boolean constantsOnly = true;
+        int len = labels.length;
+
+        for (int i = 0; i < len; i++) {
+            Object convertedLabel =
+                    convertEnumConstants(lookup, enumClass, labels[i]);
+            labels[i] = convertedLabel;
+            if (constantsOnly)
+                constantsOnly = EnumDesc.class.isAssignableFrom(convertedLabel.getClass());
+        }


Suggested change

boolean constantsOnly = true;

int len = labels.length;

for (int i = 0; i < len; i++) {

Object convertedLabel =

convertEnumConstants(lookup, enumClass, labels[i]);

labels[i] = convertedLabel;

if (constantsOnly)

constantsOnly = EnumDesc.class.isAssignableFrom(convertedLabel.getClass());

}

boolean constantsOnly = true;

int len = labels.length;

Object[] convertedLabels = new Object[len];

for (int i = 0; i < len; i++) {

Object convertedLabel =

convertEnumConstants(lookup, enumClass, labels[i]);

convertedLabels[i] = convertedLabel;

if (constantsOnly)

constantsOnly = EnumDesc.class.isAssignableFrom(convertedLabel.getClass());

}

ExE-Boss · 2024-06-27T19:04:15Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

+            EnumDesc<?>[] enumDescLabels =
+                    Arrays.copyOf(labels, labels.length, EnumDesc[].class);


Suggested change

EnumDesc<?>[] enumDescLabels =

Arrays.copyOf(labels, labels.length, EnumDesc[].class);

EnumDesc<?>[] enumDescLabels =

Arrays.copyOf(convertedLabels, len, EnumDesc[].class);

ExE-Boss · 2024-06-27T19:04:38Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

-            target = MethodHandles.permuteArguments(body, MethodType.methodType(int.class, Object.class, int.class), 1, 0);
+            EnumDesc<?>[] enumDescLabels =
+                    Arrays.copyOf(labels, labels.length, EnumDesc[].class);
+            target = MethodHandles.insertArguments(StaticHolders.MAPPED_ENUM_SWITCH, 2, lookup, enumClass, enumDescLabels, new MappedEnumCache());
        } else {
            target = generateTypeSwitch(lookup, invocationType.parameterType(0), labels);


Suggested change

target = generateTypeSwitch(lookup, invocationType.parameterType(0), labels);

target = generateTypeSwitch(lookup, invocationType.parameterType(0), convertedLabels);

lahodaj · 2024-06-27T19:28:04Z

Since labels is no longer eagerly cloned, it’s important to store the converted labels into a local array to avoid leaking them to user code.

True. But it is easier and cleaner, IMO, to simply put back cloning of the labels.

lahodaj · 2024-07-02T12:47:34Z

Any input on the current version of the patch?

liach · 2024-07-02T13:16:32Z

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java

+                    convertEnumConstants(lookup, enumClass, labels[i]);
+            labels[i] = convertedLabel;
+            if (constantsOnly)
+                constantsOnly = EnumDesc.class.isAssignableFrom(convertedLabel.getClass());


Feels a bit weird when we can use convertedLabel instanceof EnumDesc

…eFrom

liach

Looks great to me, thanks for the cleanup!

lahodaj · 2024-08-06T09:05:33Z

/integrate

openjdk · 2024-08-06T09:07:45Z

Going to push as commit 958786b.
Since your change was applied there have been 458 commits pushed to the master branch:

f92c60e: 8337810: ProblemList BasicDirectoryModel/LoaderThreadCount.java on Windows
0d8ec42: 8337642: Remove unused APIs of GCPolicyCounters
2057594: 8337782: Use THROW_NULL instead of THROW_0 in pointer contexts in prims code
73718fb: 8337788: RISC-V: Cleanup code in MacroAssembler::reserved_stack_check
965d6b9: 8335836: serviceability/jvmti/StartPhase/AllowedFunctions/AllowedFunctions.java fails with unexpected exit code: 112
7146dae: 8337783: Use THROW_NULL instead of THROW_0 in pointer contexts in misc runtime code
431d4f7: 8337785: Fix simple -Wzero-as-null-pointer-constant warnings in x86 code
e2c07d5: 8337299: vmTestbase/nsk/jdb/stop_at/stop_at002/stop_at002.java failure goes undetected
08f697f: 8337819: Update GHA JDKs to 22.0.2
42652b2: 8337787: Fix -Wzero-as-null-pointer-constant warnings when JVMTI feature is disabled
... and 448 more: https://git.openjdk.org/jdk/compare/57f8b91e558e5b9ff9c2000b8f74e3a1988ead2b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-06T09:07:50Z

@lahodaj Pushed as commit 958786b.

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

8332522: SwitchBootstraps::mappedEnumLookup constructs unused array

8ab9ee6

openjdk bot added the rfr Pull request is ready for review label Jun 26, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 26, 2024

cl4es reviewed Jun 26, 2024

View reviewed changes

cl4es approved these changes Jun 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 26, 2024

liach reviewed Jun 26, 2024

View reviewed changes

Reflecting review feedback.

79b3a76

liach reviewed Jun 27, 2024

View reviewed changes

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java Outdated Show resolved Hide resolved

Update src/java.base/share/classes/java/lang/runtime/SwitchBootstraps…

d7c9784

….java Co-authored-by: Chen Liang <liach@openjdk.org>

ExE-Boss reviewed Jun 27, 2024

View reviewed changes

Clone the labels.

512a802

lahodaj mentioned this pull request Jun 28, 2024

8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final #19933

Closed

3 tasks

liach approved these changes Jul 2, 2024

View reviewed changes

Reflecting review feedback - using instanceof rather than isAssignabl…

d09fa40

…eFrom

openjdk bot removed the ready Pull request is ready to be integrated label Jul 8, 2024

liach approved these changes Jul 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 12, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 6, 2024

openjdk bot closed this Aug 6, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 6, 2024

		@@ -289,20 +280,16 @@ public static CallSite enumSwitch(MethodHandles.Lookup lookup,
		labels = Stream.of(labels).map(l -> convertEnumConstants(lookup, enumClass, l)).toArray();

		EnumDesc<?>[] enumDescLabels =
		Arrays.copyOf(labels, labels.length, EnumDesc[].class);

	target = generateTypeSwitch(lookup, invocationType.parameterType(0), labels);
	target = generateTypeSwitch(lookup, invocationType.parameterType(0), convertedLabels);

8332522: SwitchBootstraps::mappedEnumLookup constructs unused array #19906

8332522: SwitchBootstraps::mappedEnumLookup constructs unused array #19906

Uh oh!

Conversation

lahodaj commented Jun 26, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 26, 2024

Uh oh!

openjdk bot commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 26, 2024

Uh oh!

mlbridge bot commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

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!

cl4es Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ExE-Boss 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!

lahodaj commented Jun 27, 2024

Uh oh!

lahodaj commented Jul 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

lahodaj commented Aug 6, 2024

Uh oh!

openjdk bot commented Aug 6, 2024

Uh oh!

openjdk bot commented Aug 6, 2024

Uh oh!

Uh oh!

lahodaj commented Jun 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 26, 2024 •

edited

Loading

mlbridge bot commented Jun 26, 2024 •

edited

Loading

cl4es Jun 26, 2024 •

edited

Loading