RFR: 8291769: Translation of switch with record patterns could be improved [v6]

[Vicente Romero]

On Tue, 20 Sep 2022 13:33:14 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> This is an attempt to improve the performance and scalability of switches with record patterns.
>> 
>> There are two main parts of this patch:
>> 1. for cases of consecutive runs of cases with the same record pattern, these are replaced with a single case and a nested switch. E.g.:
>> 
>> switch (obj) {
>>      case Box(String s) -> {}
>>      case Box(Integer i) -> {}
>>      case Box(Number n) -> {}
>>      ...
>> }
>> =>
>> switch (obj) {
>>      case Box b ->
>>           switch (b.o()) {
>>               case String s -> {}
>>               case Integer i -> {}
>>               case Number n -> {}
>>               default -> continue-with-outer-switch;
>>           };
>>     ...
>> }
>> 
>> 
>> This is done by first unrolling the record patterns into simple binding patterns a guards, and then by finding cases with the common binding pattern as a label and a viable guard. Binding patterns are reused as much as possibly, eliminating specialized handling of record patterns as much as possible.
>> 
>> 2. When a record accessor method fails with an exception, we need to wrap this exception with a `MatchException`. Currently this is being done by introducing a special proxy method which catches the exception and re-throws. The proposed patch here eliminates the need for the accessor methods by producing an appropriate `ExceptionTable` in the classfile, and a separate catch handler. This handler is attached to the innermost usable block, which is either the method block, lambda body, (static or non-static) initializer or a try block. This should ensure correct semantics, while not producing too many unnecessary catch handlers.
>> 
>> I ran the new code through a JMH benchmark:
>> [PatternsOptimizationTest.java.txt](https://github.com/openjdk/jdk/files/9260707/PatternsOptimizationTest.java.txt)
>> 
>> The results are:
>>  - for "long" testcase (a switch with many cases):
>> 
>> PatternsOptimizationTest.testExistingTranslationLongSwitch   thrpt   25  1025740.668 ± 15325.355  ops/s
>> PatternsOptimizationTest.testNewTranslationLongSwitch        thrpt   25  1588461.471 ± 15315.509  ops/s
>> 
>>  - for "short" testcase (a switch with no so many cases):
>> 
>> PatternsOptimizationTest.testExistingTranslationShortSwitch  thrpt   25  6418845.624 ± 75981.939  ops/s
>> PatternsOptimizationTest.testNewTranslationShortSwitch       thrpt   25  6894823.439 ± 67420.858  ops/s
>> 
>> 
>> So, the performance seems to be improved, at least in these cases.
>> 
>> As a follow-up work, there are several other improvements that may be worth investigating like using if cascades instead of switches with very few cases (but possibly not very trivial for switch expressions, at least for switch expressions of type `boolean`), or improving the code produced by the runtime bootstrap method (`SwitchBootstraps.typeSwitch`).
>
> Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into JDK-8291769
>  - Merge branch 'master' into JDK-8291769
>  - Reflecting review feedback.
>  - Merge branch 'master' into JDK-8291769
>  - Using a custom record instead of a generic Pair.
>  - Fixing test.
>  - Properly handle null record components.
>  - 8291769: Translation of switch with record patterns could be improved

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java line 819:

> 817:                 }
> 818:             } else if (currentBinding != null &&
> 819:                        commonBinding.type.tsym == currentBinding.type.tsym &&

question, what about having a map relating commonBindings and accumulators, shouldn't this allow you to group common patterns even if they are not consecutive?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java line 648:

> 646:         result =
> 647:                 scan(tree.mods, that.mods)
> 648:                         && (tree.name == that.name || synthetic)

I think that considering two variables equal if one is synthetic should be something that probably shouldn't occur by default, IMO there should be a way for clients of `TreeDiffer` to op-in if they want this behavior

-------------

PR: https://git.openjdk.org/jdk/pull/9746