On Thu, 27 Jun 2024 15:32:38 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 >> Swi... > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java > > Co-authored-by: Chen Liang <li...@openjdk.org> Since `labels` is no longer eagerly cloned, it’s important to store the converted labels into a local array to avoid leaking them to user code. src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 287: > 285: if (constantsOnly) > 286: constantsOnly = > EnumDesc.class.isAssignableFrom(convertedLabel.getClass()); > 287: } Suggestion: boolean constantsOnly = true; int len = labels.length; Object[] convertedLabels = new Object[len]; for (int i = 0; i < len; i++) { Object convertedLabel = convertEnumConstants(lookup, enumClass, labels[i]); convertedLabels[i] = convertedLabel; if (constantsOnly) constantsOnly = EnumDesc.class.isAssignableFrom(convertedLabel.getClass()); } src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 297: > 295: //else return "typeSwitch(labels)" > 296: EnumDesc<?>[] enumDescLabels = > 297: Arrays.copyOf(labels, labels.length, > EnumDesc[].class); Suggestion: EnumDesc<?>[] enumDescLabels = Arrays.copyOf(convertedLabels, len, EnumDesc[].class); src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 300: > 298: target = > MethodHandles.insertArguments(StaticHolders.MAPPED_ENUM_SWITCH, 2, lookup, > enumClass, enumDescLabels, new MappedEnumCache()); > 299: } else { > 300: target = generateTypeSwitch(lookup, > invocationType.parameterType(0), labels); Suggestion: target = generateTypeSwitch(lookup, invocationType.parameterType(0), convertedLabels); ------------- PR Review: https://git.openjdk.org/jdk/pull/19906#pullrequestreview-2146292192 PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1657681541 PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1657681804 PR Review Comment: https://git.openjdk.org/jdk/pull/19906#discussion_r1657682159