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