Skip to content

Avoid ObjectRef overhead for effectively final case vars#8375

Merged
lrytz merged 1 commit intoscala:2.12.xfrom
retronym:faster/case-var-no-capture
Sep 25, 2019
Merged

Avoid ObjectRef overhead for effectively final case vars#8375
lrytz merged 1 commit intoscala:2.12.xfrom
retronym:faster/case-var-no-capture

Conversation

@retronym
Copy link
Member

Given

class C {
    lazy val s: reflect.internal.SymbolTable = ???
    import s._
    def test(t: Tree) = {
        t match {
            case ap @ Apply(sel @ Select(_, _), _) if (ap, sel).hashCode > 0 =>
                1
            case ap @ Apply(sel @ Select(_, _), _) if (ap, sel).hashCode == 0 =>
                () => ap
        }
    }
}
$ scalac  -Xprint:jvm sandbox/test.scala  2>&1 | tee /tmp/old && qscalac  -Xprint:jvm sandbox/test.scala  2>&1 | tee /tmp/new && diff -U1000 /tmp/{old,new}
--- /tmp/old    2019-08-27 10:47:15.000000000 +1000
+++ /tmp/new    2019-08-27 10:47:18.000000000 +1000
@@ -1,80 +1,80 @@
 [[syntax trees at end of                       jvm]] // test.scala
 package <empty> {
   class C extends Object {
     final <synthetic> lazy private[this] var s: scala.reflect.internal.SymbolTable = _;
     @volatile private[this] var bitmap$0: Boolean = _;
     private def s$lzycompute(): scala.reflect.internal.SymbolTable = {
       C.this.synchronized(if (C.this.bitmap$0.unary_!())
         {
           C.this.s = (scala.Predef.???(): scala.reflect.internal.SymbolTable);
           C.this.bitmap$0 = true
         });
       C.this.s
     };
-    <stable> <accessor> lazy def s(): scala.reflect.internal.SymbolTable = (if (C.this.bitmap$0.unary_!())
+    <stable> <accessor> lazy def s(): scala.reflect.internal.SymbolTable = if (C.this.bitmap$0.unary_!())
       C.this.s$lzycompute()
     else
-      C.this.s: scala.reflect.internal.SymbolTable);
+      C.this.s;
     def test(t: reflect.internal.Trees$Tree): Object = {
       <synthetic> var rc13: Boolean = false;
-      <synthetic> var x2: runtime.ObjectRef = scala.runtime.ObjectRef.create((null: reflect.internal.Trees$Apply));
+      <synthetic> <stable> var x2: reflect.internal.Trees$Apply = (null: reflect.internal.Trees$Apply);
       {
         case <synthetic> val x1: reflect.internal.Trees$Tree = t;
         case15(){
           if (x1.$isInstanceOf[reflect.internal.Trees$Apply]())
             {
               rc13 = true;
-              x2.elem = (x1.$asInstanceOf[reflect.internal.Trees$Apply](): reflect.internal.Trees$Apply);
+              x2 = (x1.$asInstanceOf[reflect.internal.Trees$Apply](): reflect.internal.Trees$Apply);
               {
-                val sel: reflect.internal.Trees$Tree = x2.elem.$asInstanceOf[reflect.internal.Trees$Apply]().fun();
+                val sel: reflect.internal.Trees$Tree = x2.fun();
                 if (sel.$isInstanceOf[reflect.internal.Trees$Select]())
                   {
                     <synthetic> val x4: reflect.internal.Trees$Select = (sel.$asInstanceOf[reflect.internal.Trees$Select](): reflect.internal.Trees$Select);
-                    if (new Tuple2(x2.elem.$asInstanceOf[reflect.internal.Trees$Apply](), x4).hashCode().>(0))
+                    if (new Tuple2(x2, x4).hashCode().>(0))
                       matchEnd14(scala.Int.box(1))
                     else
                       case16()
                   }
                 else
                   case16()
               }
             }
           else
             case16()
         };
         case16(){
           if (rc13)
             {
-              val sel: reflect.internal.Trees$Tree = x2.elem.$asInstanceOf[reflect.internal.Trees$Apply]().fun();
+              val sel: reflect.internal.Trees$Tree = x2.fun();
               if (sel.$isInstanceOf[reflect.internal.Trees$Select]())
                 {
                   <synthetic> val x9: reflect.internal.Trees$Select = (sel.$asInstanceOf[reflect.internal.Trees$Select](): reflect.internal.Trees$Select);
-                  if (new Tuple2(x2.elem.$asInstanceOf[reflect.internal.Trees$Apply](), x9).hashCode().==(0))
+                  if (new Tuple2(x2, x9).hashCode().==(0))
                     matchEnd14({
                       $anonfun(x2)
                     })
                   else
                     case17()
                 }
               else
                 case17()
             }
           else
             case17()
         };
         case17(){
           matchEnd14(throw new MatchError(x1))
         };
         matchEnd14(x: Object){
           x
         }
       }
     };
-    final <static> <artifact> def $anonfun$test$1(x2$1: runtime.ObjectRef): reflect.internal.Trees$Apply = x2$1.elem.$asInstanceOf[reflect.internal.Trees$Apply]();
+    final <static> <artifact> def $anonfun$test$1(x2$1: reflect.internal.Trees$Apply): reflect.internal.Trees$Apply = x2$1;
     def <init>(): C = {
       C.super.<init>();
       ()
     }
   }
 }

Given

```scala
class C {
    lazy val s: reflect.internal.SymbolTable = ???
    import s._
    def test(t: Tree) = {
        t match {
            case ap @ Apply(sel @ Select(_, _), _) if (ap, sel).hashCode > 0 =>
                1
            case ap @ Apply(sel @ Select(_, _), _) if (ap, sel).hashCode == 0 =>
                () => ap
        }
    }
}
```

```
$ scalac  -Xprint:jvm sandbox/test.scala  2>&1 | tee /tmp/old && qscalac  -Xprint:jvm sandbox/test.scala  2>&1 | tee /tmp/new && diff -U1000 /tmp/{old,new}
```

```diff
--- /tmp/old    2019-08-27 10:47:15.000000000 +1000
+++ /tmp/new    2019-08-27 10:47:18.000000000 +1000
@@ -1,80 +1,80 @@
 [[syntax trees at end of                       jvm]] // test.scala
 package <empty> {
   class C extends Object {
     final <synthetic> lazy private[this] var s: scala.reflect.internal.SymbolTable = _;
     @volatile private[this] var bitmap$0: Boolean = _;
     private def s$lzycompute(): scala.reflect.internal.SymbolTable = {
       C.this.synchronized(if (C.this.bitmap$0.unary_!())
         {
           C.this.s = (scala.Predef.???(): scala.reflect.internal.SymbolTable);
           C.this.bitmap$0 = true
         });
       C.this.s
     };
-    <stable> <accessor> lazy def s(): scala.reflect.internal.SymbolTable = (if (C.this.bitmap$0.unary_!())
+    <stable> <accessor> lazy def s(): scala.reflect.internal.SymbolTable = if (C.this.bitmap$0.unary_!())
       C.this.s$lzycompute()
     else
-      C.this.s: scala.reflect.internal.SymbolTable);
+      C.this.s;
     def test(t: reflect.internal.Trees$Tree): Object = {
       <synthetic> var rc13: Boolean = false;
-      <synthetic> var x2: runtime.ObjectRef = scala.runtime.ObjectRef.create((null: reflect.internal.Trees$Apply));
+      <synthetic> <stable> var x2: reflect.internal.Trees$Apply = (null: reflect.internal.Trees$Apply);
       {
         case <synthetic> val x1: reflect.internal.Trees$Tree = t;
         case15(){
           if (x1.$isInstanceOf[reflect.internal.Trees$Apply]())
             {
               rc13 = true;
-              x2.elem = (x1.$asInstanceOf[reflect.internal.Trees$Apply](): reflect.internal.Trees$Apply);
+              x2 = (x1.$asInstanceOf[reflect.internal.Trees$Apply](): reflect.internal.Trees$Apply);
               {
-                val sel: reflect.internal.Trees$Tree = x2.elem.$asInstanceOf[reflect.internal.Trees$Apply]().fun();
+                val sel: reflect.internal.Trees$Tree = x2.fun();
                 if (sel.$isInstanceOf[reflect.internal.Trees$Select]())
                   {
                     <synthetic> val x4: reflect.internal.Trees$Select = (sel.$asInstanceOf[reflect.internal.Trees$Select](): reflect.internal.Trees$Select);
-                    if (new Tuple2(x2.elem.$asInstanceOf[reflect.internal.Trees$Apply](), x4).hashCode().>(0))
+                    if (new Tuple2(x2, x4).hashCode().>(0))
                       matchEnd14(scala.Int.box(1))
                     else
                       case16()
                   }
                 else
                   case16()
               }
             }
           else
             case16()
         };
         case16(){
           if (rc13)
             {
-              val sel: reflect.internal.Trees$Tree = x2.elem.$asInstanceOf[reflect.internal.Trees$Apply]().fun();
+              val sel: reflect.internal.Trees$Tree = x2.fun();
               if (sel.$isInstanceOf[reflect.internal.Trees$Select]())
                 {
                   <synthetic> val x9: reflect.internal.Trees$Select = (sel.$asInstanceOf[reflect.internal.Trees$Select](): reflect.internal.Trees$Select);
-                  if (new Tuple2(x2.elem.$asInstanceOf[reflect.internal.Trees$Apply](), x9).hashCode().==(0))
+                  if (new Tuple2(x2, x9).hashCode().==(0))
                     matchEnd14({
                       $anonfun(x2)
                     })
                   else
                     case17()
                 }
               else
                 case17()
             }
           else
             case17()
         };
         case17(){
           matchEnd14(throw new MatchError(x1))
         };
         matchEnd14(x: Object){
           x
         }
       }
     };
-    final <static> <artifact> def $anonfun$test$1(x2$1: runtime.ObjectRef): reflect.internal.Trees$Apply = x2$1.elem.$asInstanceOf[reflect.internal.Trees$Apply]();
+    final <static> <artifact> def $anonfun$test$1(x2$1: reflect.internal.Trees$Apply): reflect.internal.Trees$Apply = x2$1;
     def <init>(): C = {
       C.super.<init>();
       ()
     }
   }
 }

```
@retronym
Copy link
Member Author

After #8371, which manually worked around the issue in the specialize phase, there is only one change to pack/lib when bootstrapped with this compiler:

diff --git a/scala/tools/nsc/interpreter/Imports.class.asm b/scala/tools/nsc/interpreter/Imports.class.asm
index aab7148..dc60ef0 100644
--- a/scala/tools/nsc/interpreter/Imports.class.asm
+++ b/scala/tools/nsc/interpreter/Imports.class.asm
@@ -703,29 +703,23 @@
     ICONST_0
     ISTORE 13
     ACONST_NULL
-    INVOKESTATIC scala/runtime/ObjectRef.create (Ljava/lang/Object;)Lscala/runtime/ObjectRef;
     ASTORE 14
     ALOAD 11
     INSTANCEOF scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     IFEQ L4
     ICONST_1
     ISTORE 13
-    ALOAD 14
     ALOAD 11
     CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
-    PUTFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
+    ASTORE 14
     ALOAD 0
     ALOAD 14
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKESPECIAL scala/tools/nsc/interpreter/Imports.checkHeader$1 (Lscala/tools/nsc/interpreter/MemberHandlers$ImportHandler;)Z (itf)
     IFEQ L4
     ALOAD 3
     INVOKEVIRTUAL scala/collection/mutable/StringBuilder.clear ()V
     ALOAD 3
     ALOAD 14
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKEVIRTUAL scala/tools/nsc/interpreter/MemberHandlers$ImportHandler.member ()Lscala/reflect/internal/Trees$Tree;
     ASTORE 15
     NEW scala/collection/immutable/StringOps
@@ -748,8 +742,6 @@
     ILOAD 13
     IFEQ L6
     ALOAD 14
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKEVIRTUAL scala/tools/nsc/interpreter/MemberHandlers$ImportHandler.importsWildcard ()Z
     IFEQ L6
     ALOAD 0
@@ -762,7 +754,7 @@
    L0
     ALOAD 1
     ALOAD 14
-    INVOKESTATIC scala/tools/nsc/interpreter/Imports.$anonfun$importsCode$8 (Lscala/collection/mutable/StringBuilder;Lscala/runtime/ObjectRef;)Lscala/collection/mutable/StringBuilder; (itf)
+    INVOKESTATIC scala/tools/nsc/interpreter/Imports.$anonfun$importsCode$8 (Lscala/collection/mutable/StringBuilder;Lscala/tools/nsc/interpreter/MemberHandlers$ImportHandler;)Lscala/collection/mutable/StringBuilder; (itf)
     GOTO L7
    L1
     ASTORE 16
@@ -790,8 +782,6 @@
     IFEQ L8
     ALOAD 0
     ALOAD 14
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKEVIRTUAL scala/tools/nsc/interpreter/MemberHandlers$ImportHandler.importedNames ()Lscala/collection/immutable/List;
     ALOAD 2
     ALOAD 1
@@ -803,8 +793,6 @@
     GETSTATIC scala/Predef$any2stringadd$.MODULE$ : Lscala/Predef$any2stringadd$;
     GETSTATIC scala/Predef$.MODULE$ : Lscala/Predef$;
     ALOAD 14
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKEVIRTUAL scala/tools/nsc/interpreter/MemberHandlers$ImportHandler.member ()Lscala/reflect/internal/Trees$Tree;
     INVOKEVIRTUAL scala/Predef$.any2stringadd (Ljava/lang/Object;)Ljava/lang/Object;
     LDC "\n"
@@ -813,8 +801,6 @@
     POP
     ALOAD 2
     ALOAD 14
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKEVIRTUAL scala/tools/nsc/interpreter/MemberHandlers$ImportHandler.importedNames ()Lscala/collection/immutable/List;
     INVOKEVIRTUAL scala/collection/mutable/HashSet.$plus$plus$eq (Lscala/collection/TraversableOnce;)Lscala/collection/generic/Growable;
     ASTORE 9
@@ -911,15 +897,13 @@
     MAXLOCALS = 21

   // access flags 0x1009
-  public static synthetic $anonfun$importsCode$8(Lscala/collection/mutable/StringBuilder;Lscala/runtime/ObjectRef;)Lscala/collection/mutable/StringBuilder;
+  public static synthetic $anonfun$importsCode$8(Lscala/collection/mutable/StringBuilder;Lscala/tools/nsc/interpreter/MemberHandlers$ImportHandler;)Lscala/collection/mutable/StringBuilder;
     // parameter final  code$1
     // parameter final  x2$1
     ALOAD 0
     GETSTATIC scala/Predef$any2stringadd$.MODULE$ : Lscala/Predef$any2stringadd$;
     GETSTATIC scala/Predef$.MODULE$ : Lscala/Predef$;
     ALOAD 1
-    GETFIELD scala/runtime/ObjectRef.elem : Ljava/lang/Object;
-    CHECKCAST scala/tools/nsc/interpreter/MemberHandlers$ImportHandler
     INVOKEVIRTUAL scala/tools/nsc/interpreter/MemberHandlers$ImportHandler.member ()Lscala/reflect/internal/Trees$Tree;

There would be more unwanted captures if we introduced a flag like -opt:omit-subpat-binders to mimic what is still done for the deprecated -optimiize and bootstrapped with that. So this PR might enable us to go ahead with such a change without fear of this performance gotcha.

Copy link
Contributor

@hrhino hrhino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

@SethTisue SethTisue added the performance the need for speed. usually compiler performance, sometimes runtime performance. label Sep 3, 2019
@SethTisue SethTisue modified the milestones: 2.12.10, 2.12.11 Sep 3, 2019
@lrytz lrytz merged commit 8a742a7 into scala:2.12.x Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance the need for speed. usually compiler performance, sometimes runtime performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants