RFR: 8300543 Compiler Implementation for Pattern Matching for switch

[Christian Wimmer]

On Fri, 17 Mar 2023 12:15:58 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
> 
>  - the pattern matching for switch and record patterns features are made final, together with updates to tests.
>  - parenthesized patterns are removed.
>  - qualified enum constants are supported for case labels.
> 
> This change herein also includes removal record patterns in for each loop, which may be split into a separate PR in the future.

src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 1:

> 1: /*

@lahodaj  The way that the boostrap method `enumSwitch` and its corresponding implementation method `doEnumSwitch` are implemented right now has an unfortunate side effect on class initialization: since the bootstrap method already looks up the actual enum values, it triggers class initialization of the enum. This is not a problem when the switched value is non-null, because then obviously the enum is already initialized. But when the switched value is `null`, then the boostrap method triggers class initialization.

Example:

enum E { 
    A, B;

    static {
        new Throwable("Hello, World!").printStackTrace();
    }
}


public class SwitchTest {

    public static int testMethod(E selExpr) {
        return switch (selExpr) {
            case A -> 3;
            case B -> 6;
            case null -> -1;
        };
    }

    public static void main(String argv[]) {
        System.out.println(testMethod(null));
    }
}


It produces the following output (on JDK 20, but I don't see any changes in this PR that would alter the behavior):

java.lang.Throwable: Hello, World!
	at E.<clinit>(SwitchTest.java:5)
	at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized0(Native Method)
	at java.base/jdk.internal.misc.Unsafe.ensureClassInitialized(Unsafe.java:1160)
	at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.ensureClassInitialized(MethodHandleAccessorFactory.java:300)
	at java.base/jdk.internal.reflect.MethodHandleAccessorFactory.newMethodAccessor(MethodHandleAccessorFactory.java:71)
	at java.base/jdk.internal.reflect.ReflectionFactory.newMethodAccessor(ReflectionFactory.java:159)
	at java.base/java.lang.reflect.Method.acquireMethodAccessor(Method.java:724)
	at java.base/java.lang.reflect.Method.invoke(Method.java:575)
	at java.base/java.lang.Class.getEnumConstantsShared(Class.java:3938)
	at java.base/java.lang.Class.enumConstantDirectory(Class.java:3961)
	at java.base/java.lang.Enum.valueOf(Enum.java:268)
	at java.base/java.lang.invoke.ConstantBootstraps.enumConstant(ConstantBootstraps.java:144)
	at java.base/java.lang.runtime.SwitchBootstraps.convertEnumConstants(SwitchBootstraps.java:262)
	at java.base/java.lang.runtime.SwitchBootstraps.lambda$enumSwitch$0(SwitchBootstraps.java:238)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.Spliterators$ArraySpliterator.forEachRemaining(Spliterators.java:1006)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:575)
	at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:616)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:622)
	at java.base/java.lang.runtime.SwitchBootstraps.enumSwitch(SwitchBootstraps.java:238)
	at java.base/java.lang.invoke.BootstrapMethodInvoker.invoke(BootstrapMethodInvoker.java:147)
	at java.base/java.lang.invoke.CallSite.makeSite(CallSite.java:316)
	at java.base/java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:279)
	at java.base/java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:269)
	at SwitchTest.testMethod(SwitchTest.java:13)
	at SwitchTest.main(SwitchTest.java:21)
-1

I argue this is bad because in general because every unnecessary class initialization (and its side effects) should be avoided.

But it is really a problem for any kind of eager-bootstrapping system, like Project Leyden or GraalVM Native Image: if it is not possible to execute the bootstrap methods early due to possible side effects, optimizations are quite limited and the bootstrap method needs to be executed at run time.

A simple solution would be to keep the elements in the data array as `String` instead of enum elements, and in `doEnumSwitch` do a `== target.name` (the method has the comment `// Dumbest possible strategy` anyways).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1164570743