Emit trait method bodies in statics [rebase of #5177] #5251

adriaanm · 2016-06-28T16:25:40Z

Rebased #5177 on 2.12.x after merging #5229.

Original description:
In 2.12.0-M4 / #5003, we changed the trait encoding to do away with trait implementation classes, in favour of leaving trait methods in place as interface default methods.

However, we've since discovered that we can no longer precisely target a particular statically resolved method with invokespecial due to impedance mismatch between JVM and Scala semantics.

This pull request emits a pair of methods for each concrete trait method. The default method remains, with the same signature as before. However, it now forwards to a same-named static method (with an explicit self param) that contains the method code.

This encoding allows us to precisely target a particular method with invokestatic.

This is binary incompatible, and will require bootstrapping of the modules.

(More details to follow)

This corrects an error in the change to the trait encoding in scala#5003: getters in traits should have empty bodies and be emitted as abstract. ``` % ~/scala/2.12.0-M4/bin/scalac sandbox/test.scala && javap -c T Compiled from "test.scala" public interface T { public abstract void T$_setter_$x_$eq(int); public int x(); Code: 0: aload_0 1: invokeinterface #15, 1 // InterfaceMethod x:()I 6: ireturn public int y(); Code: 0: aload_0 1: invokeinterface #20, 1 // InterfaceMethod y:()I 6: ireturn public void y_$eq(int); Code: 0: aload_0 1: iload_1 2: invokeinterface #24, 2 // InterfaceMethod y_$eq:(I)V 7: return public void $init$(); Code: 0: aload_0 1: bipush 42 3: invokeinterface scala#29, 2 // InterfaceMethod T$_setter_$x_$eq:(I)V 8: aload_0 9: bipush 24 11: invokeinterface #24, 2 // InterfaceMethod y_$eq:(I)V 16: return } % qscalac sandbox/test.scala && javap -c T Compiled from "test.scala" public interface T { public abstract void T$_setter_$x_$eq(int); public abstract int x(); public abstract int y(); public abstract void y_$eq(int); public static void $init$(T); Code: 0: aload_0 1: bipush 42 3: invokeinterface #21, 2 // InterfaceMethod T$_setter_$x_$eq:(I)V 8: aload_0 9: bipush 24 11: invokeinterface #23, 2 // InterfaceMethod y_$eq:(I)V 16: return public void $init$(); Code: 0: aload_0 1: invokestatic scala#27 // Method $init$:(LT;)V 4: return } ```

This partially reverts the fix for SI-5278 made in 7a99c03. The original motivation for this case to avoid bytecode that stretched platform limitations in Android. For super calls to Scala defined trait methods, we won't use `invokespecial`, but rather use `invokestatic` to a static trait implementation method. As such, we can continue to prune redundant Scala interfaces. It might be worth considering removing the pruning of redundant parents altoghether, though: - We no longer include `ScalaObject` as a parent of every class, which was mentioned as a problem in SI-5728. - Scala 2.12 has left Android behind for the time being due to use of Java 8 facilities. - javac doesn't do this, so why should we?

And use this as the target of the default methods or statically resolved super or $init calls. The call-site change is predicated on `-Yuse-trait-statics` as a stepping stone for experimentation / bootstrapping. I have performed this transformation in the backend, rather than trying to reflect this in the view from Scala symbols + ASTs. We also need to add an restriction related to invokespecial to Java parents: to support a super call to one of these to implement a super accessor, the interface must be listed as a direct parent of the class. The static method names has a trailing $ added to avoid duplicate name and signature errors in classfiles.

adriaanm · 2016-06-28T16:26:09Z

Bootstrap: https://scala-ci.typesafe.com/job/scala-2.12.x-integrate-bootstrap/487/console
repo_ref 9901daf
SCALA_VER_SUFFIX -M4-9901daf
STARR_REF 9901daf
PARTEST_VER 1.0.17
publishToSonatype yes
Community build once 2.12.0-M4-7d51b3f is on maven (scala_ref=scala/scala.git#7d51b3f, repo_user =adriaanm,repo_ref=2.12.x-pr5251). Community build config in adriaanm/community-builds:2.12.x-pr5251
update starr to 2.12.0-M4-9901daf, partest to 1.0.17

adriaanm · 2016-06-28T17:00:53Z

Retrying bootstrap without building scalacheck and a new partest (1.0.17 -- not tagged yet) that doesn't depend on scalacheck: https://scala-ci.typesafe.com/job/scala-2.12.x-integrate-bootstrap/487/console

Keeping diff minimal since this will need to be reverted once 2.12.0 is final.

adriaanm · 2016-06-28T19:59:59Z

bootstrap success :)

adriaanm · 2016-06-28T20:30:34Z

Running community build with sonatype-staging added to the dbuild virtual repo: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/436/console

adriaanm · 2016-06-28T21:23:07Z

Drad, it failed with that same

[sbt-testng-interface] java.lang.NoSuchMethodError: scala.Product.$init$(Lscala/Product;)V
[sbt-testng-interface]  at scala.util.parsing.json.JSONArray.<init>(Parser.scala:103)
[sbt-testng-interface]  at scala.tools.nsc.doc.html.page.IndexScript.scala$tools$nsc$doc$html$page$IndexScript$$$anonfun$2(IndexScript.scala:54)
[sbt-testng-interface]  at scala.collection.TraversableLike.scala$collection$TraversableLike$$$anonfun$3(TraversableLike.scala:234)
[sbt-testng-interface]  at scala.collection.immutable.Map$Map3.foreach(Map.scala:176)
[sbt-testng-interface]  at scala.collection.TraversableLike.map(TraversableLike.scala:234)
[sbt-testng-interface]  at scala.collection.AbstractTraversable.map(Traversable.scala:104)
[sbt-testng-interface]  at scala.tools.nsc.doc.html.page.IndexScript.<init>(IndexScript.scala:29)
[sbt-testng-interface]  at scala.tools.nsc.doc.html.page.IndexScript$.apply(IndexScript.scala:153)
[sbt-testng-interface]  at scala.tools.nsc.doc.html.HtmlFactory.generate(HtmlFactory.scala:104)
[sbt-testng-interface]  at scala.tools.nsc.doc.html.Doclet.generateImpl(Doclet.scala:23)
[sbt-testng-interface]  at scala.tools.nsc.doc.doclet.Generator.generate(Generator.scala:23)
[sbt-testng-interface]  at scala.tools.nsc.doc.DocFactory.generate$1(DocFactory.scala:128)
[sbt-testng-interface]  at scala.tools.nsc.doc.DocFactory.document(DocFactory.scala:131)
[sbt-testng-interface]  at xsbt.Runner.run(ScaladocInterface.scala:26)

retronym · 2016-06-28T22:23:32Z

Drad, it failed with that same

Trying again without -Dskip.locker (IIRC that was what fixed this last time).

adriaanm · 2016-06-28T22:30:38Z

You recall correctly. I forgot -- time to fast track that effort to resolve Scala from maven.

retronym · 2016-06-28T22:31:03Z

LGTM

adriaanm · 2016-06-29T00:58:22Z

Ok, so next step is to push a commit that sets starr.version to 2.12.0-M4-9901daf and run the release build (in the same vein as 41d5b9f)

adriaanm · 2016-06-29T01:21:08Z

TODO: scala/scala-partest#67

adriaanm · 2016-06-29T01:32:26Z

[info] >>> The dbuild result is------------: SUCCESS (project rebuilt ok)

adriaanm · 2016-06-29T04:20:20Z

/rebuild

This commit switches to using 2.12.0-M3-dc9effe as STARR, so that we can switch to the new trait encoding where each concrete trait member gets a a static member, which has the actual implementation (as well as serving as a target for for super calls using invokestatic), and a default member (forwards to the static member). Also bump partest to 1.0.17 -- the release that goes with the in-sourcing of scalacheck. Replace a few more -Yopt with -opt (for our new STARR)

lrytz · 2016-06-29T08:26:54Z

🎈

The itf flag of `visitMethodInsn` and `Handle` in ASM needs to be true when the method is defined in an interface. Java 8 ignores this but Java 9 fails at runtime with: java.lang.IncompatibleClassChangeError: Method ... must beInterfaceMethodref constant This commit is inspired by similar changes in scalac, in particular see 7d51b3f by Jason Zaugg (from scala#5251) and e619b03 by Lukas Rytz (from scala#5293). This issue was also discussed in scala#5452 (which does not matter for Dotty since we do not run the backend optimizer).

- scala/bug#5638 was fixed by something farther back than I can build. - scala/bug#8348 was fixed by scala#5251. - scala/bug#9291 was fixed by scala#6092.

The itf flag of `visitMethodInsn` and `Handle` in ASM needs to be true when the method is defined in an interface. Java 8 ignores this but Java 9 fails at runtime with: java.lang.IncompatibleClassChangeError: Method ... must beInterfaceMethodref constant This commit is inspired by similar changes in scalac, in particular see 7d51b3f by Jason Zaugg (from scala#5251) and e619b03 by Lukas Rytz (from scala#5293). This issue was also discussed in scala#5452 (which does not matter for Dotty since we do not run the backend optimizer).

smarter · 2019-02-13T16:10:41Z

In 2.12.0-M4 / #5003, we changed the trait encoding to do away with trait implementation classes, in favour of leaving trait methods in place as interface default methods.
However, we've since discovered that we can no longer precisely target a particular statically resolved method with invokespecial due to impedance mismatch between JVM and Scala semantics.

Dotty doesn't use the scheme implemented in the PR and I'm having trouble finding a test case where this makes a difference (I couldn't find anything like that in the tests added in this PR), could you give me an example @retronym ?

smarter · 2019-02-15T04:04:59Z

I'm still confused (please help!), but my best guess, is that this PR is about code like this:

trait A { def foo = "A" }
trait B extends A { override def foo = "B" }
class C extends A with B {
  def bar = super[A].foo
}
object Test extends C {
  def main(args: Array[String]): Unit = {
    assert(bar == "A")
  }
}

This works in both Scala 2 and Dotty, but the mechanism is very different, in Scala 2 we get:

  public java.lang.String foo();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokestatic  #16                 // InterfaceMethod B.foo$:(LB;)Ljava/lang/String;
         4: areturn

and in Dotty we get:

  public java.lang.String foo();
    descriptor: ()Ljava/lang/String;
    flags: ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokespecial #19                 // InterfaceMethod B.foo:()Ljava/lang/String;
         4: areturn

But the PR description says:

However, we've since discovered that we can no longer precisely target a particular statically resolved method with invokespecial due to impedance mismatch between JVM and Scala semantics.

So how come the Dotty version works ? The only other relevant difference I see from javap is that in Scala 2 we get:

public class C implements B

But in Dotty we get:

public class C implements A,B

So if declaring all the interfaces you extend from is enough to make invokespecial work as expected, why isn't Scala 2 doing that ? Is there some more complicated scenario where this falls apart ? This PR contains commit 91b066a whose commit message seems to be advocating for not pruning any parent, so I'm even more confused now. Ping @retronym ?

retronym · 2019-02-15T04:16:08Z

@smarter IIRC, (redundantly) declaring all interfaces at each level triggered the default method related code in the JVM's classloading to be slow, which impacts startup time.

Given trait A1 <: trait Ai <: trait An, it also results in an O(N^2) cost in classfile size.

We considered doing it conditionally when a super call was actually present, but opted not to. I'd need to dig up the PR discussion to remember why.

retronym · 2019-02-15T04:24:27Z

scala/scala-dev#98 (comment)

smarter · 2019-02-15T15:07:44Z

Thanks for the pointers this is really helpful! I ended up writing a novel-sized issue to organize my thoughts here: scala/scala3#5928

retronym added 6 commits June 28, 2016 09:18

Better diagnostic for optimizer crashes

9f54af3

Remove stray .class file from version control

a9bebf8

Don't minimize parents of java defined syms.

d3c0885

scala-jenkins added this to the 2.12.0-RC1 milestone Jun 28, 2016

adriaanm modified the milestones: 2.12.0-M5, 2.12.0-RC1 Jun 28, 2016

Bootstrap skips scalacheck build step / partest dep

9901daf

Keeping diff minimal since this will need to be reverted once 2.12.0 is final.

adriaanm force-pushed the rebase-5177 branch from 7172a13 to 9901daf Compare June 28, 2016 17:42

scala-jenkins added the reviewed label Jun 28, 2016

adriaanm changed the title ~~Rebase #5177~~ Emit trait method bodies in statics [rebase of #5177] Jun 29, 2016

adriaanm force-pushed the rebase-5177 branch from a75dffd to d8c862b Compare June 29, 2016 06:10

lrytz merged commit 79e24d5 into scala:2.12.x Jun 29, 2016

lrytz mentioned this pull request Jun 29, 2016

wrong assumption about invokespecial scala/scala-dev#143

Closed

SethTisue added the release-notes worth highlighting in next release notes label Sep 6, 2016

adriaanm modified the milestone: 2.12.0-M5 Oct 29, 2016

adriaanm added 2.12 and removed 2.12 labels Oct 29, 2016

smarter mentioned this pull request Sep 17, 2017

Fix IncompatibleClassChangeError under Java 9 lampepfl/scala#24

Merged

hrhino added a commit to hrhino/scala that referenced this pull request Jan 16, 2018

Test cases for a few accidentally-fixed bugs.

eee8097

- scala/bug#5638 was fixed by something farther back than I can build. - scala/bug#8348 was fixed by scala#5251. - scala/bug#9291 was fixed by scala#6092.

hrhino mentioned this pull request Jan 16, 2018

Test cases for a few accidentally-fixed bugs. #6266

Merged

smarter mentioned this pull request Feb 15, 2019

Investigate changes to the trait encoding that could improve (cold) performance scala/scala3#5928

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit trait method bodies in statics [rebase of #5177] #5251

Emit trait method bodies in statics [rebase of #5177] #5251

adriaanm commented Jun 28, 2016 •

edited

Loading

adriaanm commented Jun 28, 2016 •

edited by lrytz

Loading

adriaanm commented Jun 28, 2016 •

edited

Loading

adriaanm commented Jun 28, 2016

adriaanm commented Jun 28, 2016

adriaanm commented Jun 28, 2016

retronym commented Jun 28, 2016

adriaanm commented Jun 28, 2016

retronym commented Jun 28, 2016

adriaanm commented Jun 29, 2016 •

edited

Loading

adriaanm commented Jun 29, 2016

adriaanm commented Jun 29, 2016

adriaanm commented Jun 29, 2016

lrytz commented Jun 29, 2016

smarter commented Feb 13, 2019

smarter commented Feb 15, 2019

retronym commented Feb 15, 2019 •

edited

Loading

retronym commented Feb 15, 2019 •

edited

Loading

smarter commented Feb 15, 2019 •

edited

Loading

Emit trait method bodies in statics [rebase of #5177] #5251

Emit trait method bodies in statics [rebase of #5177] #5251

Conversation

adriaanm commented Jun 28, 2016 • edited Loading

adriaanm commented Jun 28, 2016 • edited by lrytz Loading

adriaanm commented Jun 28, 2016 • edited Loading

adriaanm commented Jun 28, 2016

adriaanm commented Jun 28, 2016

adriaanm commented Jun 28, 2016

retronym commented Jun 28, 2016

adriaanm commented Jun 28, 2016

retronym commented Jun 28, 2016

adriaanm commented Jun 29, 2016 • edited Loading

adriaanm commented Jun 29, 2016

adriaanm commented Jun 29, 2016

adriaanm commented Jun 29, 2016

lrytz commented Jun 29, 2016

smarter commented Feb 13, 2019

smarter commented Feb 15, 2019

retronym commented Feb 15, 2019 • edited Loading

retronym commented Feb 15, 2019 • edited Loading

smarter commented Feb 15, 2019 • edited Loading

adriaanm commented Jun 28, 2016 •

edited

Loading

adriaanm commented Jun 28, 2016 •

edited by lrytz

Loading

adriaanm commented Jun 28, 2016 •

edited

Loading

adriaanm commented Jun 29, 2016 •

edited

Loading

retronym commented Feb 15, 2019 •

edited

Loading

retronym commented Feb 15, 2019 •

edited

Loading

smarter commented Feb 15, 2019 •

edited

Loading