On Fri, 3 Nov 2023 08:41:12 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).
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 338:
> 336:
> 337: @Override
> 338: public Object apply(Integer labelIndex, Object value) {
Since we already know the EnumDesc (i.e. `result`), we can just convert this to
a `BiPredicate<Integer, Object>` for simplicity.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 346:
> 344: Class<?> clazz =
> label.constantType().resolveConstantDesc(lookup);
> 345:
> 346: if (value.getClass() != clazz) {
Not related to this patch, but this appears to be wrong: we should do something
like
if (!(value instanceof Enum<?> ev) || ev.getDeclaringClass() != clazz)
return SENTINEL; // or false
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 383:
> 381: .withMethod("typeSwitch",
> MethodTypeDesc.ofDescriptor("(Ljava/lang/Object;ILjava/util/function/BiFunction;)I"),
> Classfile.ACC_STATIC, mb -> {
> 382: mb.withFlags(AccessFlag.PUBLIC, AccessFlag.FINAL,
> AccessFlag.STATIC)
> 383: .withCode(cb -> {
Can just use `clb.withMethodBody` and pass the access flags with
`Classfile.ACC_PUBLIC | Classfile.ACC_STATIC | Classfile.ACC_FINAL`
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 432:
> 430: cb.aload(2);
> 431: cb.constantInstruction(enumIdx);
> 432:
> cb.invokestatic(Integer.class.describeConstable().get(),
You can use `ConstantDescs.CD_Integer` etc.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 434:
> 432:
> cb.invokestatic(Integer.class.describeConstable().get(),
> 433: "valueOf",
> 434:
> MethodType.methodType(Integer.class,
I recommend `MethodTypeDesc.of()` instead.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381409965
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381411165
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381384302
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381388594
PR Review Comment: https://git.openjdk.org/jdk/pull/16489#discussion_r1381391495
