[JEP-305] Inefficient compilation of Pattern Matching for instanceof

[Remi Forax]

Hi Evgeny,
you should log a bug against javac, clearly the generated bytecode should be cleaned up.

But it's too late for Java 14, so you will need to workaround that in JavaCoCo.
Given that it's still a preview feature, the workaround should not stay forever.

By the way, what is the policy for supporting preview features (classfile minor version tagged with 65535) for JaCoCo ?

regards,
Rémi

----- Mail original -----
> De: "Evgeny Mandrikov" <mandrikov at gmail.com>
> À: "amber-dev" <amber-dev at openjdk.java.net>
> Envoyé: Samedi 18 Janvier 2020 14:55:03
> Objet: [JEP-305] Inefficient compilation of Pattern Matching for instanceof

> Hello!
> 
> I'm the project lead and developer of open source project JaCoCo [1] - one
> of the most popular code coverage tools for Java.
> 
> During our experiments with JEP-305,
> we realized that javac in JDK 14 EA b32
> compiles
> 
> class Example {
>    static void example(Object e) {
>        if (e instanceof String s) {
>            nop(s);
>        }
>    }
> 
>    public static void main(String[] args) {
>        example(new Object());
>        example("");
>    }
> 
>    static void nop(String s) {
>    }
> }
> 
> into following bytecode
> 
>  static void example(java.lang.Object);
>    descriptor: (Ljava/lang/Object;)V
>    flags: (0x0008) ACC_STATIC
>    Code:
>      stack=2, locals=3, args_size=1
>         0: aload_0
>         1: astore_2
>         2: aload_2
>         3: instanceof    #7                  // class java/lang/String
>         6: ifeq          26
>         9: aload_2
>        10: checkcast     #7                  // class java/lang/String
>        13: dup
>        14: astore_1
>        15: aload_2
>        16: checkcast     #7                  // class java/lang/String
>        19: if_acmpne     26
>        22: aload_1
>        23: invokestatic  #9                  // Method
> nop:(Ljava/lang/String;)V
>        26: return
> 
> which contains two comparisons (at offsets 6 and 19).
> Presence of the second one seems redundant and quite unfortunate for users
> of JaCoCo,
> because one of the two branches of this comparison is never executed
> and will be present in code coverage report as uncovered,
> whereas looking at the source code users will expect to see only one
> comparison.
> See attached screenshots.
> 
> And I guess that other tools that analyze bytecode might be affected,
> e.g. SpotBugs [2] which as far as I know detects redundant comparisons [3].
> 
> After further investigations [4],
> we understood that JEP-305 was implemented in javac as AST-to-AST
> transformation [5]
> and that the second comparison is a way to place an assignment into a
> condition in AST:
> 
> void example(Object e) {
>    String s;
>    Object temp = e;
>    if (
>        (temp instanceof String)
>         && ((s = (String) temp) == (String) temp)
>    ) {
>        nop(s);
>    }
> }
> 
> During the same investigations [4], we also looked at ongoing
> implementation of JEP-305 in Eclipse Compiler [6]
> and seems that it will be producing bytecode with only one comparison:
> 
>  static void example(java.lang.Object);
>    descriptor: (Ljava/lang/Object;)V
>    flags: (0x0008) ACC_STATIC
>    Code:
>      stack=1, locals=2, args_size=1
>         0: aconst_null
>         1: astore_1
>         2: aload_0
>         3: instanceof    #13                 // class java/lang/String
>         6: ifeq          18
>         9: aload_0
>        10: checkcast     #13                 // class java/lang/String
>        13: astore_1
>        14: aload_1
>        15: invokestatic  #15                 // Method
> nop:(Ljava/lang/String;)V
>        18: return
> 
> and thus also smaller class files.
> 
> So we are wondering if there is a chance to improve javac to get rid of
> this redundant comparison?
> 
> I can try to work on such improvement,
> but ultimately will need a sponsor as I have only author status in OpenJDK
> census [7].
> 
> Regards,
> Evgeny
> 
> [1] https://github.com/jacoco/jacoco
> [2] https://github.com/spotbugs/spotbugs
> [3] https://github.com/spotbugs/spotbugs/issues/756[4]
> https://groups.google.com/d/msg/jacoco-dev/XgsMix2BBGc/zbMjqi5eEAAJ
> [5] https://hg.openjdk.java.net/jdk/jdk/rev/7799a51dbe30#l19.155
> [6] https://bugs.eclipse.org/bugs/show_bug.cgi?id=531715
> [7] https://openjdk.java.net/census#godin