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

Fri Aug 5 21:54:05 UTC 2022

On 05. 08. 22 22:52, forax at univ-mlv.fr wrote:

> [...]
>
>>>> In any case, I'm a bit afraid that we could enhance a translation patch
>>>> forever, always adding new cases that can be optimized. So, I think we
>>>> need to draw a line somewhere where the patch provides a benefit while
>>>> still being testable and reviewable. With a possible separate
>>>> improvements, if they prove to be useful. It is possible I haven't drawn
>>>> the line far enough, but looking at the patch, it seems complex enough.
>>> I think the issue is that a list of pattern is not the right intermediary
>>> structure to try to optimize the pattern matching, a decision tree is a better
>>> intermediary representation (this is how pattern matching is usually optimized,
>>> i've not invented something new :) )
>>>
>>> Here is an example of such decision tree:
>>> https://urldefense.com/v3/__https://github.com/forax/switch-pattern-combinator__;!!ACWV5N9M2RV99hQ!JevAQE8IkRf4n-BMKA2sMTNFfJ5sAyJ1xopR2O9rq_45-xqaHOgirecR8E4zIGB_RgMb27G3tlcHEuGADjk$
>>
>> I am not really opposed to having an intermediate structure, but I think
>> the overall cost of the structure is likely to be significant, and it
>> should provide significantly better code to offset that.
> I agree.
>
>>
>> Looking at the page, I see:
>>
>> The advantage of a decision tree is that if two patterns have a common
>> prefix, the code for the common prefix is generated only once.
>>
>>
>> I wonder if there's a *conceptual* difference between what this PR is
>> doing and what your code is doing. The PR here is trying to take common
>> prefixes (of viable cases) and generate code for them only once. I don't
>> think that not optimizing
>>
>> case A(String s) -> {}
>>
>> case A a -> {}
>>
>> is a conceptual difference, but if this is something that is seen as
>> critical, I am fairly sure I can implement that without creating a brand
>> new structure and code to convert the AST into it and back.
> You need the intermediary structure when the common prefix are not subsequent
>
> case A(B b) ->
> case B(A a) ->
> case A(C c) ->
>
> Here the first and the last case shares a common prefix.

Sorry, but I don't see why I can't do this particular thing without a 
specialized intermediate representation. A bigger question, I think, is 
when it is safe to do so, and when not.

>
>>
>> It is also true the current code does not reorder the cases. I am not
>> sure if your code does reordering, but I didn't see a set of rules for
>> safe reorderings, which I think is the biggest problem here. My thinking
>> of safe rules for reordering was limited so far, and the ability to
>> reorder cases may be limited due to guards and separate compilation
>> considerations (we can also have new pattern types which may limit our
>> ability to reorder cases). But even though this PR does not reorder
>> cases, it is in principle doable.
> One of the reason to backpedal on the idea of guarded patterns is to try to cleanly separate what can do side effects thus limit the reordering and what should not do side effects.
> A guard can do side effect thus it's not a pattern. A deconstructing pattern using a deconstructor should not do side effect ("should not" like in a stream, if you do it, you are on your own) because it's a pattern.
>
>>
>> Situation would, of course, be different if we tried to optimize not
>> only prefixes of the patterns - but this does not seem to be the
>> proposal in switch-pattern-combinator.
>>
>>
>> As for casting to the last permitted type - I don't see how this relates
>> to decision trees.  While casting and catching might look interesting
>> from the bytecode perspective, it is something that is likely to be
>> relatively complex to implement in javac, and using an (effective)
>> instanceof and an explicit fallback does not really sound terrible.
> The idea of using a cast is that you provide a good error message to the users because the ClassCastException stores the class that is not handled by the pattern matching.
> You loose that information if you use an explicit fallback.

Sorry, but if I understand what you are proposing is:

B b = (B) obj; //magical catch handler somewhere handling the CCE

$rest$;

what I am saying is that this can be achieved by an analog of:

if (obj instanceof B b) {

     $rest$;

} else {

      throw <an-exception-we-want>(obj.getClass());

}

What information is lost?

>
>> There is also a question on when this is safe to do - if there's a `case
>> Object o` at the end of the switch, then probably this is not correct to
>> do anyway, etc.
> It is safe to do if the compiler has type-checked the patterns before, the cast is only done in case of a sealed type, not if there is a common supertype at the end.

I mean - consider:

sealed interface I {}

final class OnlySubtypeAtCompileTime implements I {}

record Box(I i) {}

Object b = new Box(<a-new-subtype-not-known-at-compile-time>);

switch (b) {

      case Box(OnlySubtypeAtCompileTime t) -> System.err.println("Should 
probably not fail here,");

      case Object obj -> System.err.println("but should end up here.");

}

>
>>
>> Regarding deferring all this to runtime, that has definitely a lot of
>> desirable properties. And there were multiple experiments with that.
>> But, the API for that is fairly complex - probably easier to do now when
>> we have guards on cases, not guarded patterns, but still likely
>> significant. (Overall, guard are really tough, because they a) capture;
>> b) provide new bindings.)
> "provide new bindings" => do you have an example of that ?

Sure:

public class TA {
    public static void main(String... args) {
        Object o = new Box("");
        switch (o) {
            case Box b when b.o() instanceof String s -> 
System.err.println(s);
            default -> {}
        }
    }
    record Box(Object o) {}
}

(There can be any number of new bindings from the guard, of course.)

> I don't think a guard should be allowed to do that. It looks like a big gunfoot to me.

I believe the current specification supports guards that introduce new 
bindings:

http://cr.openjdk.java.net/~gbierman/jep427%2b405/jep427+405-20220601/specs/patterns-switch-record-patterns-jls.html#jls-6.3.4

To me, it seems desirable. In any case, an implementation PR is probably 
not very well suited to discuss opinions on what should be specced. In 
particular (but not only) a PR that is not expected to change the semantics.

Jan

>
>>
>> Thanks,
>>
>>      Jan
>>
>>
> regards,
> Rémi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20220805/93c245e7/attachment-0001.htm>