On Thu, 27 Jun 2024 15:32:38 GMT, Jan Lahoda <[email protected]> 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 <[email protected]>
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
