On Fri, 3 Nov 2023 15:32:34 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> Consider code like:
>>
>> void test(Object o) {
>> switch (o) {
>> case X1 -> {}
>> case X2 -> {}
>> ...(about 100 cases)
>> ```
>>
>> javac will compile the switch into a switch whose selector is an indy
>> invocation to `SwitchBootstraps.typeSwitch`, with static arguments being the
>> types in the cases.
>>
>> `SwitchBootstraps.typeSwitch` will then create a chain of `MethodHandle`s
>> performing `instanceof` checks between the switch's selector and the given
>> case type. The problem is that when the number of cases is high enough,
>> (more than ~40-50), the chain gets too long, and the tests won't inline
>> anymore. This then leads to a very bad performance, when compared to
>> manually written if-instanceof-else-if-instanceof- chain.
>>
>> The proposal herein is to use bytecode (written using the ClassFile
>> API/library) instead of the `MethodHandle`s chain. The overall performance
>> of this seems to be similar to the manually written
>> if-instanceof-else-if-instanceof- chain.
>>
>> Using the benchmark from the bug, and this patch, I am getting:
>>
>> MyBenchmark.testIfElse100 thrpt 5 521826.326 ± 7510.042 ops/s
>> MyBenchmark.testSwitch100 thrpt 5 505440.170 ± 3757.178 ops/s
>>
>>
>> The most tricky part of this new way to generate the tests is handling of
>> non-type case labels, and in particular cases with enum constant labels. The
>> resolution of enum constants is deferred as much as possible, by using an
>> indirection through the `ResolvedEnumLabels`.
>>
>> Further improvements may be possible, esp. for some specific cases (like all
>> cases having a type, and the type being a final class).
>
> Jan Lahoda has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Some more get->orElseThrow
> - Reflecting review feedback.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 429:
> 427: Label next = element.next();
> 428: cb.labelBinding(element.target());
> 429: if (element.caseLabel() instanceof Class<?>
> classLabel &&
I think you can do a if else of isPresent inside instanceof Class<?> to avoid
to reapeat the instanceof and store `classLabel.describeConstable()` into a
local variable.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 517:
> 515:
> BiPredicate.class,
> 516:
> Class[].class));
> 517: return MethodHandles.insertArguments(typeSwitch, 2, new
> ResolvedEnumLabels(caller, enumDescs.toArray(s -> new EnumDesc<?>[s])),
you can use the method reference `EnumDesc[]::new`instead of `s -> new
EnumDesc<?>[s]` and same below Class[]::new (the wirldcard should not be
necessary)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381910073
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381908291
