SD-98 don't emit unnecessary mixin forwarders

[lrytz]

In most cases when a class inherits a concrete method from a trait we
don't need to generate a forwarder to the default method in the class.

[lrytz]

PR for running CI, test will follow

[lrytz]

Hehe, JUnit 4 does not test default methods :-)

Example:

$ cat Test.scala
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

@RunWith(classOf[JUnit4])
class SomeTest extends C with T {
  @Test
  def direct(): Unit = { println("direct") }
}

class C {
  @Test
  def fromClass(): Unit = { println("fromClass") }
}

trait T {
  @Test
  def fromTrait(): Unit = { println("fromTrait") }
}

Using 2.12.0-M4, which generates a forwarder C.fromTrait:

$ scalac12 -cp /Users/luc/.ivy2/cache/junit/junit/jars/junit-4.11.jar Test.scala
$ java -cp .:/Users/luc/.ivy2/cache/junit/junit/jars/junit-4.11.jar:/Users/luc/.ivy2/cache/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.3.jar:/Users/luc/scala/scala-2.12/lib/scala-library.jar org.junit.runner.JUnitCore SomeTest
JUnit version 4.11
.direct
.fromTrait
.fromClass

Time: 0.413

OK (3 tests)

After this PR:

$ qsc -cp /Users/luc/.ivy2/cache/junit/junit/jars/junit-4.11.jar Test.scala
$ java -cp .:/Users/luc/.ivy2/cache/junit/junit/jars/junit-4.11.jar:/Users/luc/.ivy2/cache/org.hamcrest/hamcrest-core/jars/hamcrest-core-1.3.jar:/Users/luc/scala/scala/build/quick/classes/library org.junit.runner.JUnitCore SomeTest
JUnit version 4.11
.direct
.fromClass

Time: 0.413

OK (2 tests)

It seems we'd need JUnit 5 (junit-team/junit4#1078), but they are still working on the first milestone (http://junit.org/junit4/junit5.html).

[lrytz]

bytecode sizes:

current 2.12.x (d2b6576):

4.9M scala-library.jar
3.3M scala-reflect.jar
8.7M scala-compiler.jar

this PR (177fa36):

4.2M scala-library.jar
3.1M scala-reflect.jar
8.5M scala-compiler.jar

both built in a full bootstrap with the optimizer enabled (rm -rf build && ant build-opt)

[lrytz]

review by @adriaanm

[adriaanm]

Nice! Could you elaborate a bit on the pros & cons in terms of safe binary compatible changes. Can a trait method safely become concrete when forwarders are always emitted whereas it can't if don't emit them until needed? Do we defer more errors to linkage from compile time? Not saying these are dealbreakers, but would be good to collect some trade offs.

[lrytz]

after this patch there are two situations where a trait forwarder is emitted:

1. a class inherits the same method from two traits, and the two traits are not in a inheritance relationship. example:

trait T1 { def f = 1 }
trait T2 { self: T1 => override def f = 2 } // T2 does not implement T1 at the bytecode level
class C extends T1 with T2

here, C needs a forwarder to T2.f

if the two traits are in an inheritance relationship, the JVM will select the method of the subtrait, so we don't need a forwarder:

trait T1 { def f = 1 }
trait T2 extends T1 { override def f = 2 }
class C1 extends T2
class C2 extends T1 with T2
class C3 extends T2 with T1

none of these classes need a forwarder, the JVM always selects T2.f.

2. a class inherits a concrete method from a trait, and the same method also exists (abstract or concrete) in a superclass. the JVM method lookup gives priority to class members. example:

class A { def f = 1 }  // even if `A.f` is abstract, the forwarder is needed
trait T extends A { override def f = 2 }
class C extends T

this case also covers the abstract override pattern:

trait T6 { def f: Int }
trait T7 extends T6 { abstract override def f = super.f + 1 }
class A extends T6 { def f = 1 }
class C extends A with T7

so what kind of change would be binary compatible? removing a method is incompatible anyway, so only adding (or making an abstract one concrete) can be compatible. adding a method is incompatible if it introduces the need for a forwarder, so cases 1. and 2. above. this seems rare enough in practice. before this change, there would always be a forwarder, so adding a method would always be binary compatible (although the "wrong" method might be chosen in the forwarder, i.e., re-compiling the subclass would could change the forwarder to pick the new concrete trait method).

finally, note that this change also has a positive impact on incremental compilation: adding or removing a method in a trait doesn't require re-compiling subclasses in most cases.

[sjrd]

Hehe, JUnit 4 does not test default methods :-)

This should be made prominent in 2.12 release notes! Not only scala/scala JUnit tests will be affected. Arbitrary tests in Scala codebases using JUnit 4 will silently not run anymore when upgrading to 2.12.

[lrytz]

Wait, what, now I confused myself.. I merged this one!?! I probably clicked on the wrong PR...

This one still needs review by @retronym and @adriaanm!

[lrytz]

Ah, I know what happened: I merged #5096, which includes the commits of this PR. So this one definitely still needs an LGTM.

[retronym]

I tend to think the following is >= readable, and faster.

@tailrec
def loop(baseClasses: List[Symbol]): Boolean = {
  baseClasses match {
    case Nil => false
    case baseClass :: rest =>
      if (baseClass ne mixinClass) {
         val m = member.overriddenSymbol(baseClasses)
         if (m.exists && (!m.owner.isTraitOrInterface || (!m.isDeferred && !mixinClass.isNonBottomSubclass(m.owner)))
            true
          else loop(rest)
      } else loop(rest)
  }
}
val needsForwarder = loop(clazz.baseClasses)

[retronym]

In this benchmark: scala/compiler-benchmark@3b4594c

I tested the difference between mono-/mega-morphic calls to a method in a base class, base 2.11 trait, and a base 2.12 trait (as proposed by this encoding, simulated by using Java interfaces).

Seems that we pay for the megamorphic call with either 2.11 style explicit forwarder, or with the proposed use of default methods. HotSpot internally installs vtable entries for resolved default methods at class load time, so this isn't all that suprising.

@sjrd noted on gitter that Scala.JS can "multi-inline" megamorphic calls to a set of identical methods (e.g. trait forwarders) and is agnostic to this change.

[lrytz]

thanks for the benchmark -- to bad that avoiding forwarders doesn't improve performance. on the other hand we can hope for the JVM to improve over time, as there's a single target method.

what would help is going back to the 2.11 scheme and add a static method for every concrete trait method. then the optimizer could inline forwarders and get to the invokestatic. we could actually make a mixed approach where the non-static interface method is also a default method (with an invokestatic of the impl) and save the forwarders.

[retronym]

I think that is worth exploring. It would also allow us to avoid the redundant parents to enable invokesspecial super calls in favour of an invokestatic

[lrytz]

most of these redundant parents stem from the $init$ calls. i guess the $init$ methods could be made static, without a non-static method in the interfce on the side. this is even independent of the question what we do for user-defined trait methods.

actually we are in the funny situation that $init$ methods override each other, but we're just calling them with invokespecial anyway:

trait T { println("hi") }
trait U extends T { println("ho") }
class C extends U

both T and U get a public default $init$()V, so the one in U is an override. C.<init> has INVOKESPECIAL T.$init$ ()V and INVOKESPECIAL U.$init$ ()V.

[retronym]

Yeah, unfortunately we can't just have U.$init$ call T.$init$ because that could lead to the wrong eval order.

[retronym]

One thing to note about mixin forwarders is that they have an identical bytecode descriptor to the overriden method (rather than one sharpened by generic substitution).

scala> trait T[A] { def u(a: A) = () }; class C extends T[Int]
defined trait T
defined class C

scala> :javap -private -c C
Compiled from "<console>"
public class C implements T<java.lang.Object> {
  public void u(java.lang.Object);
    Code:
       0: aload_0
       1: aload_1
       2: invokestatic  #15                 // Method T$class.u:(LT;Ljava/lang/Object;)V
       5: return

  public C();
    Code:
       0: aload_0
       1: invokespecial #23                 // Method java/lang/Object."<init>":()V
       4: aload_0
       5: invokestatic  #27                 // Method T$class.$init$:(LT;)V
       8: return
}

This is good: it means that we have some wiggle room to change our policy about whether to emit them or not, without introducing binary incompatibilities.

[retronym]

Welcome to Scala 2.12.0-M4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_71).
Type in expressions for evaluation. Or try :help.

scala> trait T[A] { def u(a: A) = () }; class C extends T[Int]
defined trait T
defined class C

scala> :javap -private -c C
Compiled from "<console>"
public class $line3.$read$$iw$$iw$C implements $line3.$read$$iw$$iw$T<java.lang.Object> {
  public void u(java.lang.Object);
    Code:
       0: aload_0
       1: aload_1
       2: invokespecial #22                 // Method $line3/$read$$iw$$iw$T.u:(Ljava/lang/Object;)V
       5: return

  public $line3.$read$$iw$$iw$C();
    Code:
       0: aload_0
       1: invokespecial #29                 // Method java/lang/Object."<init>":()V
       4: aload_0
       5: invokespecial #32                 // Method $line3/$read$$iw$$iw$T.$init$:()V
       8: return
}