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

[Remi Forax]

----- 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.


> 
> 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.
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)

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.

Rémi