RFR: 8300543 Compiler Implementation for Pattern Matching for switch

ExE Boss duke at openjdk.org
Fri Apr 14 12:56:55 UTC 2023

On Thu, 13 Apr 2023 07:49:05 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> 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).

Note that currently, a `switch` over an enum will already trigger initialisation of the enum class because of the need to compute the relevant mapping array by the synthetic helper class, see [JDK‑7176515] for details.
- https://github.com/openjdk/jdk/pull/10797

[JDK‑7176515]: https://bugs.openjdk.org/browse/JDK-7176515 "[JDK‑7176515] ExceptionInInitializerError for an enum with multiple switch statements"


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

More information about the compiler-dev mailing list