On Wed, 26 Jun 2024 12:32:20 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
> For general pattern matching switches, the `SwitchBootstraps` class currently
> generates a cascade of `if`-like statements, computing the correct target
> case index for the given input.
>
> There is one special case which permits a relatively easy faster handling,
> and that is when all the case labels case enum constants (but the switch is
> still a "pattern matching" switch, as tranditional enum switches do not go
> through `SwitchBootstraps`). Like:
>
>
> enum E {A, B, C}
> E e = ...;
> switch (e) {
> case null -> {}
> case A a -> {}
> case C c -> {}
> case B b -> {}
> }
>
>
> We can create an array mapping the runtime ordinal to the appropriate case
> number, which is somewhat similar to what javac is doing for ordinary
> switches over enums.
>
> The `SwitchBootstraps` class was trying to do so, when the restart index is
> zero, but failed to do so properly, so that code is not used (and does not
> actually work properly).
>
> This patch is trying to fix that - when all the case labels are enum
> constants, an array mapping the runtime enum ordinals to the case number will
> be created (lazily), for restart index == 0. And this map will then be used
> to quickly produce results for the given input. E.g. for the case above, the
> mapping will be `{0 -> 0, 1 -> 2, 2 -> 1}` (meaning `{A -> 0, B -> 2, C ->
> 1}`).
>
> When the restart index is != 0 (i.e. when there's a guard in the switch, and
> the guard returned `false`), the if cascade will be generated lazily and
> used, as in the general case. If it would turn out there are significant
> enum-only switches with guards/restart index != 0, we could improve there as
> well, by generating separate mappings for every (used) restart index.
>
> I believe the current tests cover the code functionally sufficiently - see
> `SwitchBootstrapsTest.testEnums`. It is only that the tests do not (and
> regression tests cannot easily, I think) differentiate whether the
> special-case or generic implementation is used.
>
> I've added a new microbenchmark attempting to demonstrate the difference.
> There are two benchmarks, both having only enum constants as case labels.
> One, `enumSwitchTraditional` is an "old" switch, desugared fully by javac,
> the other, `enumSwitchWithBootstrap` is an equivalent switch that uses the
> `SwitchBootstraps`. Before this patch, I was getting values like:
>
> Benchmark Mode Cnt Score Error Units
> SwitchEnum.enumSwitchTraditional avgt 15 11.719 ± 0.333 ns/op
> SwitchEnum.enumSwitchWithBootstrap avgt 15 24.668 ± 1.037 ...
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 336:
> 334: generateTypeSwitch(lookup, enumClass,
> labels)
> 335:
> .asType(MethodType.methodType(int.class,
> 336:
> enumClass,
We might have to do `Object.class` so we can call `invokeExact` below
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 426:
> 424: public volatile int[] constantsMap;
> 425: @Stable
> 426: public volatile MethodHandle generatedSwitch;
We probably don't need these 2 volatiles:
1. Arrays are already safely published (See
https://bugs.openjdk.org/browse/JDK-8333791?focusedId=14680594&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14680594)
and we can only access array elements after the array is fully initialized
then published. Thus it's a safe publication, and reads don't require explicit
volatile read.
2. `MethodHandle` is immutable and safely published, thus volatile read is
redundant as well.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654860151
PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654773322
