New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prune polymorphic implicits more aggressively (2.12.x backport) #6582

Closed
wants to merge 3 commits into
base: 2.12.x
from

Conversation

Projects
None yet
9 participants
@milessabin
Contributor

milessabin commented Apr 30, 2018

This is a backport to 2.12.x of #6580.

In rankImplicits, before we attempt to fully typecheck the pending candidate implicit, we first attempt to partially instantiate type variables in both the candidate and the target type and check for compatibility. If the compatibility check fails we can immediately prune the the candidate without having to fully typecheck it.

In the kinds of implicit searches typical of the inductive style found in shapeless and related libraries this can result in a drastic reduction in the search space and a corresponding reduction in compile times.

This commit is much simpler, more generally applicable, and less invasive than my earlier work on inductive implicits in #6481 which was doing similar pruning almost by accident. It turns out that almost all of the speedups in that earlier PR were due to the pruning rather than to any special casing of inductive patterns.

The compilation benchmark (a shapeless-style "select element by type from a large HList") from that PR is carried over here (in test/induction) and shows the same performance improvements,

  1) baseline - scalac 2.12.5
  2)            scalac 2.12.5 with matchesPtInst

               (1)   (2)
  HList Size
   50           4     3
  100           7     4
  150          14     4
  200          25     5
  250          46     5
  300          74     6
  350         118     8
  400         183    10
  450         298    12
  500         421    15

           Compile time in seconds

As an added bonus users of shapeless and shapeless-based libraries which use shapeless's Lazy type will see benefits immediately without needing to wait for and port to byname implicit arguments.

Trying this change with your project

To test this with your shapeless-using project checkout out this branch and fire up the sbt repl then clean, compile and dist/mkPack. This should give you a Scala distribution in <path to scala checkout>/build/pack.

If you now fire up an sbt repl on the project you want to build with this compiler you can then set the Scala version at the prompt via ++2.12.5=<path to scala checkout>/build/pack. You should now be able to compile and test:compile as if with 2.12.5, but with this optimization in place.

@scala-jenkins scala-jenkins added this to the 2.12.7 milestone Apr 30, 2018

@SethTisue

This comment has been minimized.

Member

SethTisue commented Apr 30, 2018

should I run a community build on this one?

@milessabin

This comment has been minimized.

Contributor

milessabin commented Apr 30, 2018

@SethTisue yes, that'd be great!

sderosiaux added a commit to sderosiaux/every-single-day-i-tldr that referenced this pull request Apr 30, 2018

jvican added a commit to scalacenter/bloop that referenced this pull request Apr 30, 2018

Make compilation of frontend faster
by:

1. Upgrading to a custom version of 2.12.7 that includes scala/scala#6582.
2. Removing the metals settings by default.

smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 30, 2018

smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 30, 2018

smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 30, 2018

smarter added a commit to dotty-staging/dotty that referenced this pull request Apr 30, 2018

jvican added a commit to scalacenter/bloop that referenced this pull request Apr 30, 2018

Make compilation of frontend faster
by:

1. Upgrading to a custom version of 2.12.7 that includes scala/scala#6582.
2. Removing the metals settings by default.
@SethTisue

This comment was marked as resolved.

Member

SethTisue commented May 1, 2018

@jvican

This comment has been minimized.

Member

jvican commented May 1, 2018

For reference, I just tried this change in Bloop. The frontend of bloop is the biggest module in our build, and it uses Shapeless via caseapp. Our compile times have been unusually slow because of Lazy. I'm seeing a 2x speedups in cold compilation, which is a great number. We're not even big users of Shapeless (only one file using Shapeless in the whole repository), so I would expect Shapeless-based codebases (many in our ecosystem) to benefit from this change.

@fommil

This comment has been minimized.

Contributor

fommil commented May 1, 2018

Some numbers, on a cold JVM, post sbt clean update:

  • ensime-server, api/compile (heavy user of shapeless)
  • scalaz-deriving, test:compile
    • 2.12.5: 19s
    • this: 19s
    • the ADTs are very small, so I wouldn't expect much change
  • my work project, (~500 files, 30k LOC) compile in main and all test configurations
    • 2.12.5: 167s
    • this: 146s (but sbt-assembly failed due to duplicate classfile entries, seems there is an sbt bug about pruning the scala-library.jar when using a custom scala dist)
    • just compile: 49s vs 45s (a small speedup)

All these projects are heavy users of shapeless generic derivation. However, I am using "semiauto" style, i.e. def gen is not implicit, and therefore not recursive. It is more boilerplate, but I feel it addresses all the problems that were highlighted in export-hook whilst being much faster to compile (I use the @scalaz.deriving annotation to generate the boilerplate).

I suspect semiauto is equivalent to the automated polymorphic pruning. Perhaps it would be feasible to disable semiauto and just derive the top level of the ADT, but I don't have time to do that refactor at the moment. My work project also has some scanamo and pureconfig uses, which are doing full auto derivation at the point of use, so that's where I guess the benefits are coming from.

sbt outputs this which gives me reason to believe it is working as expected:

> ++2.12.5=/home/fommil/Projects/scala/build/pack!
[info] Using Scala home /home/fommil/Projects/scala/build/pack with actual version 2.12.7-20180430-184100-af4ffa8
[info] Forcing Scala version to 2.12.5 on all projects.
@milessabin

This comment has been minimized.

Contributor

milessabin commented May 1, 2018

I suspect semiauto is equivalent to the automated polymorphic pruning. Perhaps it would be feasible to disable semiauto and just derive the top level of the ADT

That would be a very useful comparison to make: semiauto style trades more boilerplate for lower compilation times, so if with this change fully automatic derivation is on a par with semiauto wrt compile times that's a big win.

@jvican

This comment has been minimized.

Member

jvican commented May 1, 2018

That would be a very useful comparison to make: semiauto style trades more boilerplate for lower compilation times, so if with this change fully automatic derivation is on a par with semiauto wrt compile times that's a big win.

Note that there's still value in having semiauto over fully automatic derivation: runtime performance and code size. Semiauto is more boilerplate-y but allows programmers to control better where the instances live, and even ensure there's only one instance per type in the codebase.

@fommil

This comment has been minimized.

Contributor

fommil commented May 1, 2018

@jvican I have a plan up my sleeve to solve the boilerplate problem and improve the runtime performance even further, but I don't want to hijack this thread 😉

@milessabin

This comment has been minimized.

Contributor

milessabin commented May 1, 2018

@jvican the missing part of the picture is here IMO: #6139.

@SethTisue

This comment has been minimized.

Member

SethTisue commented May 2, 2018

results from https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/2789/consoleFull. a lot of projects were green, but specs2 crashed in typer:

[specs2] scala.MatchError: ?T (of class scala.reflect.internal.Types$TypeVar$$anon$4)
[specs2] 	at scala.reflect.internal.Variances.inType$1(Variances.scala:211)
[specs2] 	at scala.reflect.internal.Variances.$anonfun$varianceInType$1(Variances.scala:206)
[specs2] 	at scala.reflect.internal.Variances.inArgs$1(Variances.scala:206)
[specs2] 	at scala.reflect.internal.Variances.inType$1(Variances.scala:219)
[specs2] 	at scala.reflect.internal.Variances.$anonfun$varianceInType$1(Variances.scala:206)
[specs2] 	at scala.reflect.internal.Variances.inArgs$1(Variances.scala:206)
[specs2] 	at scala.reflect.internal.Variances.inType$1(Variances.scala:219)
[specs2] 	at scala.reflect.internal.Variances.varianceInType(Variances.scala:228)
[specs2] 	at scala.reflect.internal.Variances.varianceInType$(Variances.scala:205)
[specs2] 	at scala.reflect.internal.SymbolTable.varianceInType(SymbolTable.scala:18)
[specs2] 	at scala.tools.nsc.typechecker.Implicits$ImplicitSearch.$anonfun$matchesPtInst$3(Implicits.scala:582)
[specs2] 	at scala.tools.nsc.typechecker.Implicits$ImplicitSearch.scala$tools$nsc$typechecker$Implicits$ImplicitSearch$$matchesPtInst(Implicits.scala:582)
[specs2] 	at scala.tools.nsc.typechecker.Implicits$ImplicitSearch$ImplicitComputation.rankImplicits(Implicits.scala:1008)
[specs2] 	at scala.tools.nsc.typechecker.Implicits$ImplicitSearch$ImplicitComputation.findBest(Implicits.scala:1038)
[specs2] 	at scala.tools.nsc.typechecker.Implicits$ImplicitSearch.searchImplicit(Implicits.scala:1095)

with lots more stack trace where that came from.

losing specs2 knocks out many downstream projects, of course.

@fommil

This comment has been minimized.

Contributor

fommil commented May 2, 2018

Hmm, https://blog.stephenn.com/2017/07/circe-argonaut-shapless-play-json-compile-time.html suggests that semiauto is no faster than full auto in batch compile (it is, of course, significantly faster on a per case class basis, and hence better in the incremental compiler and presentation compiler).

I don't know how to explain this. I would really like to see @stephennancekivell redo the benchmarks. It would be very interesting if full auto was somehow faster than semi-auto with this patch applied! Perhaps because it finds things in the import scope and never has to check companions and nested package objects.

@stephennancekivell

This comment has been minimized.

Contributor

stephennancekivell commented May 2, 2018

hey @fommil, Im happy to help.

I updated those json codec deriving benchmarks, but unfortunately the results aren’t very promising. Maybe my benchmarks are more impacted by the case class to Hlist macro, than the amount of implicits looked up.

Here are some results.

Here building codecs for 300 case classes with up to 15 values.
circe
71s
circe.topic
81s
circe-derivation
31s
circe-derivation.topic
30s

More at https://gist.github.com/stephennancekivell/82b481eda1653f5c0922a229fa4577a1

I tried increasing the number of case class values to 100, but typer crashed with a stack overflow error. At 50x50 it wasn’t much different.

Edit: To be clear, my benchmark just creates a bunch of codes like this. https://github.com/stephennancekivell/circe-argonaut-compile-times/blob/master/circe/src/main/scala/Codecs.scala

@milessabin milessabin force-pushed the milessabin:topic/implicit-poly-prune-2.12.x branch from af4ffa8 to d013b17 May 3, 2018

@milessabin

This comment has been minimized.

Contributor

milessabin commented May 3, 2018

Rebased, incorporated feedback from #6580 and hopefully fixed the Specs2 issue seen in the community build.

@milessabin

This comment has been minimized.

Contributor

milessabin commented May 3, 2018

@SethTisue I had a tough time reproducing the issue you saw, but I think it should be fixed now. If you could run another community build when this goes green that would be awesome :-)

@SethTisue

This comment has been minimized.

Member

SethTisue commented May 4, 2018

@milessabin you might prefer to just do it yourself, rather than wait for me? you can just hit "rebuild" on the last one and change the SHA in the Scala version parameter.

the disk space issues we used to have on the Jenkins workers are fixed now, so it's fine to launch community build jobs without worrying about what else is going on

@milessabin

This comment has been minimized.

Contributor

milessabin commented May 7, 2018

Rerun here ... a failure in http4s which I think is probably spurious and a failure in scala-refactoring, which I'll investigate, otherwise all green :-)

@SethTisue

This comment has been minimized.

Member

SethTisue commented May 7, 2018

the http4s failure looks spurious to you in what sense...?

@milessabin

This comment has been minimized.

Contributor

milessabin commented May 7, 2018

@SethTisue my mistake ... I was confusing the http4s output with the blaze output.

I'll investigate.

milessabin added some commits May 4, 2018

Faster implicit search: lazier error messages
Don't eagerly compute unseen, yet expensive error messages
(because we're in a nested search, or -Xlog-implicits is not enabled).
Prune polymorphic implicits more aggressively
In rankImplicits, before we attempt to fully typecheck the pending
candidate implicit, we first attempt to partially instantiate type
variables in both the candidate and the target type and check for
compatibility. If the compatibility check fails we can immediately prune
the the candidate without having to fully typecheck it.

In the kinds of implicit searches typical of the inductive style found
in shapeless and related libraries this can result in a drastic
reduction in the search space and a corresponding reduction in compile
times.

This commit is much simpler, more generally applicable, and less
invasive than my earlier work on inductive implicits in,

  #6481

which was doing similar pruning almost by accident. It turns out that
almost all of the speedups in that earlier PR were due to the pruning
rather than to any special casing of inductive patterns.

The compilation benchmark (a shapeless-style "select element by type
from a large HList") from that PR is carried over here (in
test/induction) and shows the same performance improvements,

               (1)   (2)
  HList Size
   50           4     3
  100           7     3
  150          15     4
  200          28     4
  250          48     5
  300          81     6
  350         126     8
  400         189    11
  450         322    13
  500         405    16

           Compile time in seconds

As an added bonus users of shapeless and shapeless based libraries which
use shapeless's Lazy type will see benefits immediately without needing
to wait for and port to byname implicit arguments.

@milessabin milessabin force-pushed the milessabin:topic/implicit-poly-prune-2.12.x branch from d013b17 to 1244c83 Jun 12, 2018

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jun 12, 2018

Rebased, incorporated #6612, and fix for http4s issue seen in the community build.

I'll rerun the community build when this goes green.

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jun 13, 2018

Failed community build run here: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3010/

@SethTisue the failures here look pretty random and mostly IO and environment related. I find it fairly hard to believe that they're related to this PR. What's the current expected state of the 2.12.x community build?

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 16, 2018

@milessabin

  • scala-js: known failure, pursuing it at scala-js/scala-js#3381
  • akka-more: looks like a transient failure to resolve a dependency, should go away on re-run
  • http4s, breeze, eff, classutil: idk what these are, I have not seen these in any other runs, so I think you have to suspect your PR first, I'm afraid. but also, just try re-running the whole thing (it shouldn't take 13 hours this time since dbuild will have previous results cached)

once https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3014/ finishes, you can take it as a representative recent run. (in other recent runs specs2 failed which took out many downstream projects, but 3014 has a workaround for that)

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 16, 2018

What's the current expected state of the 2.12.x community build?

iur

(if you're a William Pène du Bois fan, if not, don't worry about it)

@SethTisue

This comment has been minimized.

Member

SethTisue commented Jun 16, 2018

I hit "rebuild" for you, it'll be https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3015/ (404 til Jenkins queue clears)

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jun 18, 2018

I'm still puzzled by the results. I'll wait for a green build and then try again from there.

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jun 22, 2018

Another unsuccessful run here: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3030/.

We now have breeze, classutil, specs2-scalaz, scala-debugger, http4s failing. I think this is due to implicit conversions being resolved differently ... I'll investigate.

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jul 4, 2018

@SethTisue the classutil failure is reproducible, but unrelated to this PR ... one of the tests is expecting a non-None result from scala.util.Properties.releaseVersion. Do you have something similar to the below for the main 2.12.x community build? Or is there something missing in the configuration for this this build?

diff --git src/test/scala/org/clapper/classutil/ClassFinderSpec.scala src/test/scala/org/clapper/classutil/ClassFinderSpec.scala
index cb8849f..a5cef52 100644
--- src/test/scala/org/clapper/classutil/ClassFinderSpec.scala
+++ src/test/scala/org/clapper/classutil/ClassFinderSpec.scala
@@ -66,7 +66,7 @@ class ClassFinderSpec extends BaseSpec {
 
   private val (runtimeClassFiles, runtimeClassFinder) = {
     import scala.util.Properties
-    val version = Properties.releaseVersion.get
+    val version = Properties.releaseVersion.getOrElse("2.12.6")
     val shortVersion = version.split("""\.""").take(2).mkString(".")
 
     val targetDirectory: Option[File] = Array(
@milessabin

This comment has been minimized.

Contributor

milessabin commented Jul 4, 2018

@SethTisue @adriaanm @etorreborre ...

The failure in specs2-scalaz is a bit of a weird one. It turns out that the backport of #6612 is the culprit. It seems that the the failing line in TaskMatchers was relying on side-effects in the unseen and now suppressed TypeDiagnostics.findReqMsg.

Partially reverting the definition of ContextErrors.typeErrorMsg to,

def typeErrorMsg(context: Context, found: Type, req: Type) =
  // if (context.openImplicits.nonEmpty && !settings.XlogImplicits.value) "type mismatch"
  // else "type mismatch" + foundReqMsg(found, req)
  "type mismatch" + foundReqMsg(found, req)

fixes the problem but defeats the object of #6612.

#6612 is present in 2.13.0-M4 which spec2-scalaz builds with, so what's happening there? It turns out that the problem has been worked around, I guess more or less incidentally as part of the process of updating for the new collections,

--- scalaz/shared/src/main/scala/org/specs2/matcher/TaskMatchers.scala
+++ scalaz/shared/src/main/scala/org/specs2/matcher/TaskMatchers.scala
@@ -26,8 +26,8 @@ trait TaskMatchers {
   def returnBefore[T](duration: Duration): TaskMatcher[T] =
     attemptRun(ValueCheck.alwaysOk, Some(duration))
 
-  def failWith[T <: Throwable : ClassTag]: Matcher[Task[_]] =
-    returnValue(be_-\/(haveClass[T])) ^^ { t: Task[_] => t.attempt }
+  def failWith[T <: Throwable : ClassTag]: Matcher[Task[Any]] =
+    returnValue(be_-\/[Throwable](haveClass(implicitly[ClassTag[T]]))) ^^ { t: Task[Any] => t.attempt }
 
   private[specs2] def attemptRun[T](check: ValueCheck[T], duration: Option[Duration]): TaskMatcher[T] =
     TaskMatcher(check, duration)

Applying the same change to specs2 for the 2.12.x build also fixes the problem with the backport of #6612 here too.

@SethTisue I'd like to push on with this community build ... would you be happy to fork specs2 with the above change? Is there some way it can be done without disrupting the normal 2.12.x community build?

The change in specs2 can be minimized slightly: the following,

--- scalaz/shared/src/main/scala/org/specs2/matcher/TaskMatchers.scala
+++ scalaz/shared/src/main/scala/org/specs2/matcher/TaskMatchers.scala
@@ -27,7 +27,7 @@ trait TaskMatchers {
     attemptRun(ValueCheck.alwaysOk, Some(duration))
 
   def failWith[T <: Throwable : ClassTag]: Matcher[Task[_]] =
-    returnValue(be_-\/(haveClass[T])) ^^ { t: Task[_] => t.attempt }
+    returnValue(be_-\/[Throwable](haveClass[T])) ^^ { t: Task[_] => t.attempt }
 
   private[specs2] def attemptRun[T](check: ValueCheck[T], duration: Option[Duration]): TaskMatcher[T] =
     TaskMatcher(check, duration)

appears to do the trick. @etorreborre ... would you be happy to make that change in specs2 for 2.12+?

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jul 4, 2018

This also appears to be responsible for the http4s failure in dsl/test:compile.

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jul 4, 2018

The scala-debugger failure appears to be a spurious runtime glitch.

@tpunder

This comment has been minimized.

tpunder commented Jul 4, 2018

I noticed one of our larger Scala projects fails to build using this patch (assuming I correctly built Scala using this branch).

The code that fails is basically this:

implicit def toOption[T](v: T): Option[T] = Option(v)
val a: Int = 123
val b: Option[Long] = a // Works under 2.12.6 but not with the implicit-poly-prune-2.12.x PR

I'm not really sure if this was intended to work in the first place but this pattern
showed up in a large Scala project we have. I'll probably remove the implicit from
our code and make all the conversions explicit since that is not a very good implicit
to have in the first place. However I'm not sure if the PR intended to change this
behavior.

Here is an SBT project that demonstrates the issue: https://github.com/tpunder/implicit-poly-prune-2.12.x-failure

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jul 5, 2018

@tpunder that looks similar to the breeze failure ... I'm working on a fix at the moment.

@etorreborre

This comment has been minimized.

etorreborre commented Jul 5, 2018

@milessabin my understanding of this change

returnValue(be_-\/[Throwable](haveClass[T])) ^^ { t: Task[_] => t.attempt }

is that [Throwable] has to be added to help type inference later on with ^^. Is that right? This use of ^^ (or adapt) is generally localized, meaning it is only used when defining matchers, not using them. If on the other hand every time people wanted to write be_-\/(haveClass[T]) they would have to write be_-\/[Throwable](haveClass[T]) that would be breaking a lot more code.

@milessabin

This comment has been minimized.

Contributor

milessabin commented Jul 5, 2018

@etorreborre yes, I believe so.

@milessabin milessabin referenced this pull request Jul 5, 2018

Closed

Publish for 2.13.0-M4 #2267

@milessabin

This comment has been minimized.

Contributor

milessabin commented Aug 7, 2018

I've tracked down and fixed the issue with breeze now so, given that this isn't going to compile cleanly with #6612 I'm going to declare victory on this and wrap up #6580.

@milessabin milessabin closed this Aug 7, 2018

@joroKr21

This comment has been minimized.

Contributor

joroKr21 commented Aug 9, 2018

I got ~8% improvement on my current work project 👍 (just tried it out though, no benchmarks or anything reliable)

@milessabin

This comment has been minimized.

Contributor

milessabin commented Aug 9, 2018

@joroKr21 comments on #6580 would be helpful :-)

@SethTisue SethTisue removed this from the 2.12.7 milestone Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment