On Thu, 13 Apr 2023 07:49:05 GMT, Jan Lahoda <jlah...@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
