new trait encoding: skip unnecessary mixin methods

[lrytz]

Can we skip emitting unnecessary mixin methods in the new trait encoding scheme?

With revision retronym/scala@8ee7aca I compiled the following example:

trait T { final def m = 1 }
class C extends T
class D extends T
class A {
  def foo(t: T) = t.m
}

• Both classes C and D get an override def m = super[T].m. Are the necessary?
• Could the default method in T be marked final?
• Could we skip the mixin methods in C and D if T.m was not final?

In the current situation, the invocation t.m in class A is polymorphic: the bytecode is INVOKEINTERFACE T.m ()I, which resolves to either C.m or D.m.

[lrytz]

Note that with impl classes, the optimizer has a chance to fix it: it rewrites INVOKEINTERFACE T.m ()I to INVOKESTATIC T$class.m (LT;)I. This won't be possible anymore, AFAIK there's no way to invoke a statically resolved default method in an interface.

[retronym]

Related: scala/scala3#1104 / dotty-staging/dotty@74d5820

Basically, we could/should try to use Java's rules to statically resolve (_: C).m and omit the forwarder in if it would hit the right method.

The forwarder would still be needed in cases like:

class A { def foo = "A" }
trait B extends A { override def foo = "B" }
class C extends B

[retronym]

One variation on our design might be to put trait impl methods in static methods with a self parameter (in the interface, rather than in the impl class). The default method would forward to these. Subclasses (or any callsite) could then call a trait implementation method, which would also avoid the hack we have that has to add transitively inherited interfaces as direct parents to allow invokespecial.

[retronym]

Or the other way around: we could create a static accessor for every trait impl method.

[DarkDimius]

Both classes C and D get an override def m = super[T].m. Are the necessary?

the are not necessary in this case. Dotty will not add them after scala/scala3#1104

[DarkDimius]

Note that they would be necessary in case of slightly changed example:

abstract class S { def m: Int }
trait T extends S { final def m = 1 }
class C extends T
class D extends T
class A {
  def foo(t: T) = t.m
}

as JVM would prefer the abstract method in S.

[retronym]

Another factor to consider is whether the analysis that leads us to omit the forwarders holds up under separate compilation. I don't have an example at hand, but I remember discussing this with @adriaanm

[retronym]

@DarkDimius BTW, are you aware that in dotty, you end up listing all transitive parent interfaces as direct parents? I wasn't sure when looking at your bridge to GenBCode whether this was intentional or accidental.

% cat sandbox/test.scala
trait A
trait B extends A
class C extends B

% ./bin/dotc sandbox/test.scala

% javap -classpath . C
Compiled from "test.scala"
public class C implements A,B {
  public C();
}

I briefly tried the same sledgehammer approach in my changes to Scalac.

It is super convenient, because you can emit arbitrary invokespecial methods in forwarder bodies of in the translation of statically resolved super calls within trait bodies.

However, it found it caused huge slowdowns in startup time, which turned out to be in the JVM's classfile parser, which does default method resolution. This isn't optimized to avoid repeating work for parent interfaces that have already been processed.

My current approach is to add redundant parents selectively (when we need to emit invokespecial).

[retronym]

For the record, I identified the slowness with the system profiler.

% instruments -t "Time Profiler" /code//jdk9/build/macosx-x86_64-normal-server-fastdebug/jdk/bin/java -Xprof -classpath . Test build/pack/lib/scala-library.jar
Instruments Trace Complete (Duration : 15.257308s; Output : /Users/jason/code/scala/instrumentscli3.trace)

[DarkDimius]

@retronym thanks, I wasn't aware of this.
This was intentional as I made this for mine convenience.

My current approach is to add redundant parents selectively (when we need to emit invokespecial).

would you kindly point me to place where this is done?
Thanks

[lrytz]

That implementation is problematic because the information is not reflected in the corresponding symbol, as discussed here: retronym/scala#19 (comment)

[retronym]

Here is the spot when I add the parents: https://github.com/scala/scala/pull/5003/files#diff-2e0fd5042aa83407a80a7557cc2a666bR1072

Lukas found the spot where I hacked his nice test to be lenient enough to deal with this inconsistency between the bytecode parents and those in the original Symbol.info.

[retronym]

Here's a patch that implements the dotty approach

diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala
index ed7ef0d..330e134 100644
--- a/src/compiler/scala/tools/nsc/transform/Mixin.scala
+++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala
@@ -8,6 +8,7 @@ package transform

 import symtab._
 import Flags._
+import scala.annotation.tailrec
 import scala.collection.mutable

 abstract class Mixin extends InfoTransform with ast.TreeDSL {
@@ -237,11 +238,24 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL {

     /* Mix in members of implementation class mixinClass into class clazz */
     def mixinTraitForwarders(mixinClass: Symbol) {
+      val baseClasses = clazz.baseClasses
       for (member <- mixinClass.info.decls ; if isImplementedStatically(member)) {
         member overridingSymbol clazz match {
           case NoSymbol =>
-            if (clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives contains member)
-              cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
+            if (clazz.info.findMember(member.name, 0, 0L, stableOnly = false).alternatives contains member) {
+              @tailrec
+              def needsForwarder(baseClasses: List[Symbol]): Boolean = baseClasses match {
+                case Nil => false
+                case baseClass :: tail =>
+                  def okay(sym: Symbol) = sym == NoSymbol || (sym.owner.isTrait && !sym.isDeferred)
+                  if ((baseClass eq member.owner) || okay(member overridingSymbol baseClass))
+                    needsForwarder(tail)
+                  else
+                    true
+              }
+              if (needsForwarder(baseClasses))
+                cloneAndAddMixinMember(mixinClass, member).asInstanceOf[TermSymbol] setAlias member
+            }
           case _        =>
         }
       }

% cat sandbox/test.scala
class A {
  def aaa = "A"
}
trait T extends A {
  private def bar = ""
  def foo = bar
  override def aaa = "T"
}
class C extends T

% qscalac sandbox/test.scala && javap -private -classpath . A T C
Compiled from "test.scala"
public class A {
  public java.lang.String aaa();
  public A();
}
Compiled from "test.scala"
public interface T {
  private java.lang.String bar();
  public java.lang.String foo();
  public java.lang.String aaa();
  public void $init$();
}
Compiled from "test.scala"
public class C extends A implements T {
  public java.lang.String aaa();
  public C();
}

[lrytz]

scala/scala#5085

[lrytz]

i looked a bit into the question of adding interfaces as direct parents to allow invokespecial.

i prefer the current solution over adding an additional static method next to every default method.

i don't see a good way to decide early what interfaces need to be direct parents and add them to the symbol info. if we care about the consistency of ClassBTypes whether they are created from a symbol or from a classifle, there's a solution: we can add the list of synthetic parent interfaces to the existing InlineInfo classfile attribute. this would allow filtering the interfaces when creating a ClassBType from a classfile.

there's no urgency to implement this, but it would be pretty straightforward. i can give it a shot and we discuss on the PR.

[lrytz]

scala/scala#5096

[adriaanm]

another advantage of only mixing in forwarders when absolutely necessary, is that we avoid the problem described in #178 (mixing in a method that has a sharper signature in a subclass that instantiates a type parameter in the method's signature)