RFR: 8300543 Compiler Implementation for Pattern Matching for switch

[Jan Lahoda]

On Wed, 12 Apr 2023 19:47:25 GMT, Christian Wimmer <cwimmer 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).

Hi Christian,

Thanks for the comment.

First, the "Dumbest possible strategy" comment should be interpreted as "we would like to do something better", not that using something slow is OK permanently. There's an attempt to do performance improvements here:
https://github.com/openjdk/jdk/pull/9779

Talking of `enumSwitch` for now (some more on `typeSwitch` later), one thing what I would like allow for the long(er) term are fast(er) implementations. If we just used `String` comparison, it would be extremely difficult to get a really good performance, even for cases where e.g. all the input values are enum constants, where the method can be reduced to a map lookup (as 9779 does).

I think we could probably still avoid the "eager" class initialization, at the cost of using the `MutableCallSite` - first, we would produce a temporary MethodHandle, and on the first non-`null` call (when the class apparently must be initialized), we would produce the real (possibly optimized) MethodHandle.

I suspect (although I may be wrong) that the eager systems may not be able to work with such a call site, but those should be able to replace the bootstrap with a custom static version without changing the semantics in any way.

A slightly bigger problem is `typeSwitch`: a new feature is that enum constants can be part of any pattern matching switch. Like:


enum E {A}
...
Object o = ...;
switch (o) {
    case E.A -> ...
    ...
}


For this, even `typeSwitch` permits enum constants now, and `java.lang.invoke.ConstantBootstraps.enumConstant` is used for that currently. We would probably need a wrapper to describe the enum constant that could be used by `typeSwitch` to avoid initialization (+the `MutableCallSite`, of course).

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

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