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 ... Marked as reviewed by redestad (Reviewer). src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 280: > 278: > 279: Class<?> enumClass = invocationType.parameterType(0); > 280: labels = Stream.of(labels).map(l -> convertEnumConstants(lookup, > enumClass, l)).toArray(); You could create `EnumDesc[] enumDescLabels` here and remove the `Arrays.copyOf` on line 290. The `labels.clone()` on line 277 also seem redundant since we only iterate over the `labels` argument once. While this case is likely fine I generally recommend using a minimal amount of streams/lambdas in bootstrap sensitive code like these dynamic bootstraps methods. ------------- PR Review: https://git.openjdk.org/jdk/pull/19906#pullrequestreview-2141731748 PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1654771207