Clean up inlining of trait methods / super accessors / mixin forwarders

[SethTisue]

in a clean checkout of 2.12.x,

> junit/testOnly scala.tools.nsc.backend.jvm.opt.InlinerTest
...
[info] Test scala.tools.nsc.backend.jvm.opt.InlinerTest.inlineArrayForeach started
[error] Test scala.tools.nsc.backend.jvm.opt.InlinerTest.inlineArrayForeach failed: java.lang.AssertionError: assertion failed: List(InvokeDynamic(INVOKEDYNAMIC, apply$mcVI$sp, (LC;)Lscala/runtime/java8/JFunction1$mcVI$sp;, MethodHandle(6,java/lang/invoke/LambdaMetafactory,altMetafactory,(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;,false), List([Ljava.lang.Object;@4c09493a, MethodHandle(6,C,$anonfun$t$1,(LC;I)V,false), [Ljava.lang.Object;@4c09493a, [Ljava.lang.Object;@4c09493a, [Ljava.lang.Object;@4c09493a, [Ljava.lang.Object;@4c09493a))), took 0.172 sec
[error]     at scala.Predef$.assert(Predef.scala:219)
[error]     at scala.tools.testing.BytecodeTesting$.assertNoIndy(BytecodeTesting.scala:235)
[error]     at scala.tools.testing.BytecodeTesting$.assertNoIndy(BytecodeTesting.scala:232)
[error]     at scala.tools.nsc.backend.jvm.opt.InlinerTest.inlineArrayForeach(InlinerTest.scala:1499)
[error]     ...

attn @lrytz

[lrytz]

uh - did this show up on CI?

[lrytz]

in any case, it reproduces for me, so i'll take a look.

[SethTisue]

beats the hell out of me why it doesn't fail on Jenkins

[lrytz]

probably because

• CI builds do a full bootsrap before running tests, or
• the second stage of that bootstrap is built with the optimizer enabled

[lrytz]

Yeah, it's because the scala-library is not built using -opt locally, but on CI. Array.foreach is ArrayOps$ofInt.foreach, which inherits the implementation from trait collection.IndexedSeqOptimized.

Since we re-introduced mixin forwarders, the inliner will just inline the forwarder and end up with a call to the static accessor method INVOKESTATIC IndexedSeqOptimized.foreach$ (which itself does INVOKESPECIAL foreach). Note that the current inliner does not do traces of inlining.

If the optimizer is enabled, the body of foreach is inlined into the static accessor foreach$, and from there also into the mixin forwarder foreach generated within ArrayOps$ofInt.

This points out an issue we have in 2.12.0 with code duplication: every higher-order trait method (these are picked by the heuristics) body is duplicated into its static super accessor, and from there into every mixin forwarder of classes extending that trait.

My plan here:

• Special-case inliner to look through mixin forwarders and static super accessors - this will fix the test.
• Prevent inlining into static super accessors and mixin forwarders - with the special-case in the inliner, this duplication is not useful.

I expect that this will bring down the size of scala-library.jar in 2.12.1 by quite a bit. Luckily the change is binary compatible.

[lrytz]

It's not an easy fix, unfortunately, because the current inliner infrastructure is not fully there to support this case. We have

interface T {
  @inline int m() { return 1 }
  // static super accessor
  @inline static int m$() { invokespecial m() }
}
final class C implements T {
  // mixin forwarder
  @inline int m() { invokestatic T.m$() }
}
class D {
  int test() { return (new C).m() }
}

There is some infrastructure in place do "inline C.m, then in the inlined code, inline T.m$, then in the inlined code, inline T.m".

We can inline the call to C.m into D. But then we cannot inline T.m$() into D, because the invokespecial T.m would not be valid, so there's a check that fails. The step-by-step approach does not see that the invokespecial will go away by the last inlining step.

We also cannot just assume the invokespecial goes away. Here's an example:

trait T {
  def i = 1
  @inline final def m = try { i } catch { case _: Throwable => () }
}
object O extends T
class C {
  def f = println(O.m)
}

We cannot inline the body of the trait method m at the callsite O.m because the call stack is non-empty, and m has an exception handler. When entering the handler, the stack is cleared, which would eliminate too many values (the Predef$ module). The way the analysis is implemented right now, we cannot easily check ahead of time if a chain of inlining will succeed. If we just go ahead and inline the mixin forwarder and the static accessor, we have an invokespecial T.m in class C which is invalid, but continuing to inline it would also generate invalid bytecode.

I guess we need infrastructure to speculatively start inlining, and roll-back if necessary. Inlining modifies multiple things in-place (the bytecode, the call graph, need to check if something else).

So it looks like a bit of work is needed for 2.12.1 here. On the positive side, speculative inlining and the ability to roll back was on my roadmap for optimizer infrastructure, to enable better heuristics.

[lrytz]

OK, I have a prototype for this, I think looks feasible for 2.12.1 https://github.com/scala/scala/compare/2.12.x...lrytz:sd259?expand=1

[lrytz]

scala/scala#5529