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

[Jan Lahoda]

On 04. 08. 22 17:43, Remi Forax wrote:

>
> ----- Original Message -----
>> From: "Jan Lahoda" <jlahoda at openjdk.org>
>> To: compiler-dev at openjdk.org
>> Sent: Thursday, August 4, 2022 5:04:52 PM
>> Subject: RFR: 8291769: Translation of switch with record patterns could be improved
>> 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.
> How it works when you have a record pattern followed by a type pattern with the same type ?
> Something like:
>
>    switch (obj) {
>      case Box(String s) -> {}
>      case Box(Integer i) -> {}
>      
>      case Box b -> {}
>    }
>
> because you now have twice the same type pattern on the outer switch but the first one should not NPE.


Currently the `case Box b -> {}` is kept separate. It would be possible 
to merge (it would simply be a default in the nested switch), but it is 
an additional complexity in a fairly complex code already.


>
>
>> 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.
> Yes !
>
> As said above, the semantics for null is slightly changed, because now you may throw a NPE instead of a MatchException.


The idea here is that the semantics should not be changed, and changes 
to semantics are bugs. But thanks to your comment, I realized null 
record components are not handled properly, so I've attempted to fix that:

https://github.com/openjdk/jdk/pull/9746/commits/7ddeafab1d1ea23285e60765ec0e61d7a509e442


> You can add explicit nullchecks to throw a MatchException instead but i think we can do better for our users.
>
> I believe that with this transformation, we can now make the difference between the 3 kinds of errors,
> - the call to the accessor / destructor fails throwing an error similar to ExceptionInTheInitializerError (ExceptionInTheDestructorError ?)
> - the tested value is null throwing a NPE because most of them are implicit when b.o() is called. This means that the compiler has to insert a requireNonNull if the nullcheck is not implicit.
> - when there are more permitted subtypes at runtime throwing an IncompatibleClassChangeError (like with an switch expression on an enum)


(FWIW, I believe the spec is saying what should happen in specific 
cases, so I don't think this is something javac should decide on its 
own. javac can decide on how the behavior is implemented, but not the 
behavior.)


>
> for the last one, the idea is that if there is a sealed type, instead of generating an if instanceof for all cases, the last one should not be an instanceof but just a cast with a cacth handler that catch the ClassCastException and throw an IncompatibleClassChangeError wrapping the CCE instead.


Note that when switching over a type we don't generate instanceof for 
the subtypes of that type. The typeSwitch bootstrap/indy does the 
checks, and the bytecode produced by javac only produces checkcasts. (It 
may produce instanceofs for the nested patterns, though.) It would be 
possible to use a default instead of the last case, and add catch 
handler for the checkcast, but generating cases for all the checked 
types, and adding a default which throws (which I believe is the current 
implementation) feels much easier and cleaner to implement, and I don't 
really see a significant disadvantage in that.


Thanks,

     Jan


>
> Rémi