Faster compilation of inductive implicits. #5649

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@milessabin
Contributor

milessabin commented Jan 18, 2017

This PR provides much faster (35 x in the provided benchmark case) compilation of the kind of inductive implicit resolution that is found in shapeless and its uses for type class derivation in libraries such as Circe, Doobie and Scodec. I will follow up with a PR for a backport of this to 2.11.x later today.

The method is to spot patterns in the set of eligible implicit definitions for an implicit argument of the form F[A, B, C ...] corresponding to a singly recursive, convergent (to either success or failure) induction. When these hold the induction is solved for directly by unfolding the recursive steps until one of the base cases is reached. The patterns are a little conservative (in the sense that some inductions might be missed), but they cover many important cases, in particular most of the interesting inductions in shapeless are handled.

The strategy is enabled by the -Yinduction-heuristics scalac flag. If it isn't enabled then normal implicit resolution is always performed. If it is enabled, then the inductive strategy is applied where possible. If it cannot be applied implicit resolution falls back to the normal mechanism. In all cases it is intended that the semantics are identical to normal implicit resolution: the only effect of enabling this option should be a reduction in compile times for applicable implicit resolutions.

An @inductive annotation for types is provided with a similar intent to @tailrec. It asserts that implicit instances of types with the annotation are expected to be solved for by the new induction mechanism. It is an error if they are not and they are instead solved by the normal mechanism. Note that the annotation does not change semantics in the success case (it does change semantics in the failure case because an error is reported). Nor does it affect which algorithm is used to resolve implicits: it is simply a post hoc check that the expected solver was used successfully, in the same way that @tailrec is a post hoc check that an expected tail call optimization was performed.

The use of @inductive allows more informative error messages to be generated for inductions which fail due to missing implicit instances of types which are auxiliary to the main induction. For example, in the case of shapeless's Mapper type class (which recurses through an HList applying an auxiliary type class instance specific to the type of each element as it goes), with normal implicit search if any instance of the auxiliary type class is missing the error reported will be just that the overall Mapper instance can't be resolved. With this algorithm, and in the presence of the @inductive annotation, the compiler knows that an induction is being performed and is able to report that an induction has failed due to the absence of the specific auxiliary type class instance. This is a much requested feature by the authors and users of libraries based on shapeless type class derivation. See test/files/neg/inductive-implicts.{scala,flags,check} for an example.

One limitation that people using inductive implicits now where the induction step is wrapped in shapeless's Lazy will notice is that these will not take advantage of this algorithm. In some cases it may be possible to drop the use of Lazy, though not all. I intend to follow this PR with another which provides an interpretation of byname implicit arguments which is equivalent to shapeless's Lazy and which will get along nicely with this induction heuristic.

All (par)tests pass with this option forcibly enabled. In addition shapeless, Circe, Doobie and Scodec, which make extensive use of these forms of induction, all compile and test successfully. A benchmark is provided in test/files/induction which validates my performance claims.

@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Jan 18, 2017

+ if (context.reportErrors) {
+ context.issue(ite)
+ errorBuffer.retain {
+ case ite: ImplicitTypeError => false
case _ => true
}
}

This comment has been minimized.

@xeno-by

xeno-by Jan 18, 2017

Member

Maybe it would be possible to merge thes two cases into one and instead have a smaller pattern match to determine what error to issue?

@xeno-by

xeno-by Jan 18, 2017

Member

Maybe it would be possible to merge thes two cases into one and instead have a smaller pattern match to determine what error to issue?

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

Possibly. I found the logic about which error actually gets reported pretty impenetrable here and tried multiple variations in the hope of doing what you're suggesting ... none of them did the right thing. Advice on the right way to do it would be very much appreciated.

@milessabin

milessabin Jan 18, 2017

Contributor

Possibly. I found the logic about which error actually gets reported pretty impenetrable here and tried multiple variations in the hope of doing what you're suggesting ... none of them did the right thing. Advice on the right way to do it would be very much appreciated.

This comment has been minimized.

@xeno-by

xeno-by Jan 18, 2017

Member

Hmm I just noticed that the cases are exactly the same except for the small divergent.withPt(paramTp) vs ite diff. I'd write something like:

case Some(ite) =>
  if (context.reportErrors) {
    val err = if (ite.isInstanceOf[DivergentImplicitTypeError]) divergent.withPt(paramTp) else ite
    context.issue(err)
    errorBuffer.retain {
      case ite: ImplicitTypeError => false
      case _ => true
    }
  }
@xeno-by

xeno-by Jan 18, 2017

Member

Hmm I just noticed that the cases are exactly the same except for the small divergent.withPt(paramTp) vs ite diff. I'd write something like:

case Some(ite) =>
  if (context.reportErrors) {
    val err = if (ite.isInstanceOf[DivergentImplicitTypeError]) divergent.withPt(paramTp) else ite
    context.issue(err)
    errorBuffer.retain {
      case ite: ImplicitTypeError => false
      case _ => true
    }
  }
+
+ case class IncompleteInductionImplicitTypeError(underlyingTree: Tree, pt: Type, aux: Type)
+ extends ImplicitTypeError {
+ def errMsg: String = s"Inductive implicit expansion for type ${pt} failed due to missing auxilliary implicit ${aux}"

This comment has been minimized.

@xeno-by

xeno-by Jan 18, 2017

Member

Is double L in auxiliary a typo?

@xeno-by

xeno-by Jan 18, 2017

Member

Is double L in auxiliary a typo?

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

Yes!

@milessabin

milessabin Jan 18, 2017

Contributor

Yes!

+ val result =
+ if(solvedContext.isFailure) acc
+ else {
+ val allUndetparams = (context.undetparams ++ context.outer.undetparams ++ solvedContext.undetparams).distinct

This comment has been minimized.

@jvican

jvican Jan 18, 2017

Member

s/context.undetparams/savedUndetparams/g?

@jvican

jvican Jan 18, 2017

Member

s/context.undetparams/savedUndetparams/g?

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

No, I don't think so.

@milessabin

milessabin Jan 18, 2017

Contributor

No, I don't think so.

+ * If it is present, the compiler will issue an error if implicit values
+ * cannot be resolved inductively.
+ *
+ * @since 2.12.0

This comment has been minimized.

@fommil

fommil Jan 18, 2017

Contributor

shouldn't this be 2.12.2?

@fommil

fommil Jan 18, 2017

Contributor

shouldn't this be 2.12.2?

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

Yes it should.

@milessabin

milessabin Jan 18, 2017

Contributor

Yes it should.

+
+ // MS: The split between the original and the new inductive computation could be more
+ // cleanly factored, but deferred to make it easier to see the difference between the
+ // cases.

This comment has been minimized.

@xeno-by

xeno-by Jan 18, 2017

Member

I think it would be beneficial if there was a step-by-step example in comments explaining inductive implicit lookup as per newly implemented logic. The test suite is extensive and even has approximate performance numbers (!), but personally for me a detailed example would be useful too.

@xeno-by

xeno-by Jan 18, 2017

Member

I think it would be beneficial if there was a step-by-step example in comments explaining inductive implicit lookup as per newly implemented logic. The test suite is extensive and even has approximate performance numbers (!), but personally for me a detailed example would be useful too.

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

Agreed.

@milessabin

milessabin Jan 18, 2017

Contributor

Agreed.

+ }
+
+ val tree2 = typed(tree1, EXPRmode, wildPt)
+ if(tree2.isErroneous) acc

This comment has been minimized.

@xeno-by

xeno-by Jan 18, 2017

Member

I think the if(...) style is much less common in scalac than if (...).

@xeno-by

xeno-by Jan 18, 2017

Member

I think the if(...) style is much less common in scalac than if (...).

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

True. I'll fix.

@milessabin

milessabin Jan 18, 2017

Contributor

True. I'll fix.

@@ -0,0 +1,196 @@
+object shapeless {

This comment has been minimized.

@fommil

fommil Jan 18, 2017

Contributor

(puts on RMS halo) you may wish to include the shapeless LICENSE / NOTICE, unless this is a subset of which you are the sole author, or consider it trivial.

@fommil

fommil Jan 18, 2017

Contributor

(puts on RMS halo) you may wish to include the shapeless LICENSE / NOTICE, unless this is a subset of which you are the sole author, or consider it trivial.

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

This is a subset of which I am the sole author.

@milessabin

milessabin Jan 18, 2017

Contributor

This is a subset of which I am the sole author.

+ if(!matchesPt(tpSubst, wildPt, undetparams)) SearchFailure
+ else {
+ val subst: TreeTypeSubstituter =
+ if (okParams.isEmpty) EmptyTreeTypeSubstituter

This comment has been minimized.

@mpociecha

mpociecha Jan 18, 2017

Member

I know it's a trifle but you write ifs inconsistently. Personally I would always add one space between if and (.

@mpociecha

mpociecha Jan 18, 2017

Member

I know it's a trifle but you write ifs inconsistently. Personally I would always add one space between if and (.

This comment has been minimized.

@milessabin

milessabin Jan 18, 2017

Contributor

Agreed. My normal style is no space, the scalac style is a space. I try to conform to local customs but sometimes slip.

@milessabin

milessabin Jan 18, 2017

Contributor

Agreed. My normal style is no space, the scalac style is a space. I try to conform to local customs but sometimes slip.

@smarter

This comment has been minimized.

Show comment
Hide comment
@smarter

smarter Jan 18, 2017

Contributor

Great work! I have two remarks:

  • Would it be possible to add a mode where both the induction search and regular search are used to verify that induction does not come up with something different? It would be enabled by a flag like -Ycheck-induction-heuristics and used in tests and when debugging.
  • This stuff is not trivial at all, I think the code would benefits from more comments explaining how the different pieces of the algorithm work in details, what are the assumption, etc.
Contributor

smarter commented Jan 18, 2017

Great work! I have two remarks:

  • Would it be possible to add a mode where both the induction search and regular search are used to verify that induction does not come up with something different? It would be enabled by a flag like -Ycheck-induction-heuristics and used in tests and when debugging.
  • This stuff is not trivial at all, I think the code would benefits from more comments explaining how the different pieces of the algorithm work in details, what are the assumption, etc.
@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Jan 18, 2017

Contributor

does the scalac build produce a mima report ? I'd like to be sure that this won't break ensime or scala-ide without a recompile (which is more important for the 2.11.x branch)

Contributor

fommil commented Jan 18, 2017

does the scalac build produce a mima report ? I'd like to be sure that this won't break ensime or scala-ide without a recompile (which is more important for the 2.11.x branch)

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jan 18, 2017

Member

@fommil yes, to verify both backwards and forwards compatibility actually.

Member

dwijnand commented Jan 18, 2017

@fommil yes, to verify both backwards and forwards compatibility actually.

@mpociecha

This comment has been minimized.

Show comment
Hide comment
@mpociecha

mpociecha Jan 18, 2017

Member

@fommil Also there's the integration build for scala-ide.

Member

mpociecha commented Jan 18, 2017

@fommil Also there's the integration build for scala-ide.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jan 18, 2017

Contributor

@smarter agreed on both points.

Contributor

milessabin commented Jan 18, 2017

@smarter agreed on both points.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jan 18, 2017

Contributor

For some reason I'm having trouble finding the Jenkins log for the failure ... am I looking in the wrong place or is Jenkins borked?

Contributor

milessabin commented Jan 18, 2017

For some reason I'm having trouble finding the Jenkins log for the failure ... am I looking in the wrong place or is Jenkins borked?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jan 18, 2017

Member
[info] prev = /home/jenkins/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.12.0.jar, curr = /home/jenkins/workspace/scala-2.12.x-validate-test/build/pack/lib/scala-library.jar
Found 1 binary incompatibilities (4 were filtered out)
======================================================
 * class scala.annotation.inductive does not have a correspondent in current
   version

Generated backward filter config definition
============================================

    filter {
        problems=[
            {
                matchName="scala.annotation.inductive"
                problemName=MissingClassProblem
            }
        ]
    }


Generated forward filter config definition
===========================================

    filter {
        problems=[]
    }

available by clicking on "Details" next to the validate-test failure, then on Console Output. (https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/3997/console)

Member

adriaanm commented Jan 18, 2017

[info] prev = /home/jenkins/.ivy2/cache/org.scala-lang/scala-library/jars/scala-library-2.12.0.jar, curr = /home/jenkins/workspace/scala-2.12.x-validate-test/build/pack/lib/scala-library.jar
Found 1 binary incompatibilities (4 were filtered out)
======================================================
 * class scala.annotation.inductive does not have a correspondent in current
   version

Generated backward filter config definition
============================================

    filter {
        problems=[
            {
                matchName="scala.annotation.inductive"
                problemName=MissingClassProblem
            }
        ]
    }


Generated forward filter config definition
===========================================

    filter {
        problems=[]
    }

available by clicking on "Details" next to the validate-test failure, then on Console Output. (https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/3997/console)

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jan 18, 2017

Member

Before you backport to 2.11, let's first complete the review/merge cycle in 2.12. This is not a trivial change, so it's going to take a while to digest, review and iterate. Because the 2.11.9 deadline is so close, I expect this will not make it into 2.11.9, though a fine candidate for the TLS 2.11.x series.

Member

adriaanm commented Jan 18, 2017

Before you backport to 2.11, let's first complete the review/merge cycle in 2.12. This is not a trivial change, so it's going to take a while to digest, review and iterate. Because the 2.11.9 deadline is so close, I expect this will not make it into 2.11.9, though a fine candidate for the TLS 2.11.x series.

@adriaanm adriaanm self-assigned this Jan 18, 2017

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jan 18, 2017

Contributor

I'll make a PR against 2.11.x and update as this PR evolves. I don't expect you to merge for LBS 2.11.9, but it will be included in TLS 2.11.9.

Contributor

milessabin commented Jan 18, 2017

I'll make a PR against 2.11.x and update as this PR evolves. I don't expect you to merge for LBS 2.11.9, but it will be included in TLS 2.11.9.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jan 18, 2017

Contributor

Gah, why do I always seem to get those whitelists wrong :-(

Contributor

milessabin commented Jan 18, 2017

Gah, why do I always seem to get those whitelists wrong :-(

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Jan 18, 2017

Member

Also there's the integration build for scala-ide.

@mpociecha that build is actually not running on 2.12.x, there's no scala-ide for 2.12 yet.

Member

lrytz commented Jan 18, 2017

Also there's the integration build for scala-ide.

@mpociecha that build is actually not running on 2.12.x, there's no scala-ide for 2.12 yet.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 19, 2017

Member

I'm having some trouble compiling the pos test with and without the new option:

⚡ time  qscalac -nowarn  test/files/pos/inductive-implicits.scala
test/files/pos/inductive-implicits.scala:583: error: could not find implicit value for parameter toTraversable: shapeless.ToTraversable[shapeless.HNil,List]
  val ttnil = ToTraversable[HNil, List]
                           ^
test/files/pos/inductive-implicits.scala:586: error: could not find implicit value for parameter e: shapeless.ToTraversable.Aux[shapeless.HNil,List,Int]
  implicitly[ToTraversable.Aux[HNil, List, Int]]
            ^
test/files/pos/inductive-implicits.scala:588: error: could not find implicit value for parameter toTraversable: shapeless.ToTraversable[shapeless.::[Int,shapeless.HNil],List]
  val tti = ToTraversable[Int :: HNil, List]
                         ^
test/files/pos/inductive-implicits.scala:591: error: could not find implicit value for parameter toTraversable: shapeless.ToTraversable[shapeless.::[Test.Apple,shapeless.::[Test.Pear,shapeless.HNil]],List]
  val ttap = ToTraversable[Apple :: Pear :: HNil, List]
                          ^
four errors found

real	0m26.175s
user	0m45.837s
sys	0m0.796s

~/code/scala on topic/inductive-implicits-2.12.x*
⚡ time  qscalac -Yinduction-heuristics -nowarn  test/files/pos/inductive-implicits.scala
test/files/pos/inductive-implicits.scala:583: error: Inductive implicit expansion for type shapeless.ToTraversable[shapeless.HNil,List] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[List[Nothing],Nothing,List[Nothing]]
  val ttnil = ToTraversable[HNil, List]
                           ^
test/files/pos/inductive-implicits.scala:586: error: Inductive implicit expansion for type shapeless.ToTraversable.Aux[shapeless.HNil,List,Int] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[List[Int],Int,List[Int]]
  implicitly[ToTraversable.Aux[HNil, List, Int]]
            ^
test/files/pos/inductive-implicits.scala:588: error: Inductive implicit expansion for type shapeless.ToTraversable[shapeless.::[Int,shapeless.HNil],List] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[Nothing,Int,List[Int]]
  val tti = ToTraversable[Int :: HNil, List]
                         ^
test/files/pos/inductive-implicits.scala:591: error: Inductive implicit expansion for type shapeless.ToTraversable[shapeless.::[Test.Apple,shapeless.::[Test.Pear,shapeless.HNil]],List] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[Nothing,Test.Pear,List[Test.Pear]]
  val ttap = ToTraversable[Apple :: Pear :: HNil, List]
                          ^
four errors found

real	0m31.445s
user	0m44.614s
sys	0m0.633s

I was hoping to produce and diff logs of the old and new tree of implicit searches to help get my intuition for how the trick works.

Member

retronym commented Jan 19, 2017

I'm having some trouble compiling the pos test with and without the new option:

⚡ time  qscalac -nowarn  test/files/pos/inductive-implicits.scala
test/files/pos/inductive-implicits.scala:583: error: could not find implicit value for parameter toTraversable: shapeless.ToTraversable[shapeless.HNil,List]
  val ttnil = ToTraversable[HNil, List]
                           ^
test/files/pos/inductive-implicits.scala:586: error: could not find implicit value for parameter e: shapeless.ToTraversable.Aux[shapeless.HNil,List,Int]
  implicitly[ToTraversable.Aux[HNil, List, Int]]
            ^
test/files/pos/inductive-implicits.scala:588: error: could not find implicit value for parameter toTraversable: shapeless.ToTraversable[shapeless.::[Int,shapeless.HNil],List]
  val tti = ToTraversable[Int :: HNil, List]
                         ^
test/files/pos/inductive-implicits.scala:591: error: could not find implicit value for parameter toTraversable: shapeless.ToTraversable[shapeless.::[Test.Apple,shapeless.::[Test.Pear,shapeless.HNil]],List]
  val ttap = ToTraversable[Apple :: Pear :: HNil, List]
                          ^
four errors found

real	0m26.175s
user	0m45.837s
sys	0m0.796s

~/code/scala on topic/inductive-implicits-2.12.x*
⚡ time  qscalac -Yinduction-heuristics -nowarn  test/files/pos/inductive-implicits.scala
test/files/pos/inductive-implicits.scala:583: error: Inductive implicit expansion for type shapeless.ToTraversable[shapeless.HNil,List] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[List[Nothing],Nothing,List[Nothing]]
  val ttnil = ToTraversable[HNil, List]
                           ^
test/files/pos/inductive-implicits.scala:586: error: Inductive implicit expansion for type shapeless.ToTraversable.Aux[shapeless.HNil,List,Int] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[List[Int],Int,List[Int]]
  implicitly[ToTraversable.Aux[HNil, List, Int]]
            ^
test/files/pos/inductive-implicits.scala:588: error: Inductive implicit expansion for type shapeless.ToTraversable[shapeless.::[Int,shapeless.HNil],List] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[Nothing,Int,List[Int]]
  val tti = ToTraversable[Int :: HNil, List]
                         ^
test/files/pos/inductive-implicits.scala:591: error: Inductive implicit expansion for type shapeless.ToTraversable[shapeless.::[Test.Apple,shapeless.::[Test.Pear,shapeless.HNil]],List] failed due to missing auxilliary implicit scala.collection.generic.CanBuildFrom[Nothing,Test.Pear,List[Test.Pear]]
  val ttap = ToTraversable[Apple :: Pear :: HNil, List]
                          ^
four errors found

real	0m31.445s
user	0m44.614s
sys	0m0.633s

I was hoping to produce and diff logs of the old and new tree of implicit searches to help get my intuition for how the trick works.

@dwijnand

This comment has been minimized.

Show comment
Hide comment
@dwijnand

dwijnand Jan 19, 2017

Member

@retronym Maybe just needs a clean rebuild of the compiler (for whatever reason)?

11:23:02 $ time qscalac -nowarn test/files/pos/inductive-implicits.scala

real	0m21.548s
user	0m43.258s
sys	0m0.970s

11:23:35 $

11:26:43 $ rm -r *.class shapeless/

11:26:50 $ time qscalac -nowarn -Yinduction-heuristics test/files/pos/inductive-implicits.scala

real	0m31.359s
user	0m50.577s
sys	0m1.076s
Member

dwijnand commented Jan 19, 2017

@retronym Maybe just needs a clean rebuild of the compiler (for whatever reason)?

11:23:02 $ time qscalac -nowarn test/files/pos/inductive-implicits.scala

real	0m21.548s
user	0m43.258s
sys	0m0.970s

11:23:35 $

11:26:43 $ rm -r *.class shapeless/

11:26:50 $ time qscalac -nowarn -Yinduction-heuristics test/files/pos/inductive-implicits.scala

real	0m31.359s
user	0m50.577s
sys	0m1.076s
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jan 19, 2017

Contributor

@retronym I'm not able to reproduce that failure running partest test/files/pos/inductive-implicits.scala from the SBT prompt. What does your qscala wrapper look like?

Contributor

milessabin commented Jan 19, 2017

@retronym I'm not able to reproduce that failure running partest test/files/pos/inductive-implicits.scala from the SBT prompt. What does your qscala wrapper look like?

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jan 20, 2017

Member

Things are working after a clean build and rm *.class.

Next question: I'm seeing that compilation is slower for pos/inductive-implicits.scala with -Yinduction-heuristics enabled.

% sbt setupPublishCore dist/mkQuick && time qscalac -nowarn test/files/pos/inductive-implicits.scala && time qscalac -nowarn -Yinduction-heuristics test/files/pos/inductive-implicits.scala
...
real	0m25.708s
user	0m54.443s
sys	0m1.089s

real	0m34.328s
user	0m55.813s
sys	0m0.823s
Member

retronym commented Jan 20, 2017

Things are working after a clean build and rm *.class.

Next question: I'm seeing that compilation is slower for pos/inductive-implicits.scala with -Yinduction-heuristics enabled.

% sbt setupPublishCore dist/mkQuick && time qscalac -nowarn test/files/pos/inductive-implicits.scala && time qscalac -nowarn -Yinduction-heuristics test/files/pos/inductive-implicits.scala
...
real	0m25.708s
user	0m54.443s
sys	0m1.089s

real	0m34.328s
user	0m55.813s
sys	0m0.823s
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Jan 21, 2017

Contributor

@retronym there is an overhead due to performing the test that the inductive heuristic is applicable. The breakeven point would be for inductions which are larger than the ones in pos/inductive-implicits.scala ... see the benchmark in test/files/induction for an example which shows how times scale with induction size for compilations with and without -Yinduction-heurisitics. The large sizes seen there are not atypical for people attempting to use shapeless for type class derivation for medium to large application data models.

I think there is probably scope for improving performance even for small instances, but the big wins are the ones we have here for medium to large instances.

Contributor

milessabin commented Jan 21, 2017

@retronym there is an overhead due to performing the test that the inductive heuristic is applicable. The breakeven point would be for inductions which are larger than the ones in pos/inductive-implicits.scala ... see the benchmark in test/files/induction for an example which shows how times scale with induction size for compilations with and without -Yinduction-heurisitics. The large sizes seen there are not atypical for people attempting to use shapeless for type class derivation for medium to large application data models.

I think there is probably scope for improving performance even for small instances, but the big wins are the ones we have here for medium to large instances.

@clhodapp

This comment has been minimized.

Show comment
Hide comment
@clhodapp

clhodapp Jan 21, 2017

Contributor

I'll chime in and note that in its current state, this patch cuts the compile time for my inductive-implicit-heavy work project by slightly more than half (258 seconds for a clean compile becomes 121). If @milessabin implements the replacement for shapeless.Lazy described in the description, that should drop even more. I can't share the code though, sorry... I just wanted to note that there definitely do exist real-world projects that benefit a lot from this even without further optimization (not that further optimization couldn't make the picture even better).

Contributor

clhodapp commented Jan 21, 2017

I'll chime in and note that in its current state, this patch cuts the compile time for my inductive-implicit-heavy work project by slightly more than half (258 seconds for a clean compile becomes 121). If @milessabin implements the replacement for shapeless.Lazy described in the description, that should drop even more. I can't share the code though, sorry... I just wanted to note that there definitely do exist real-world projects that benefit a lot from this even without further optimization (not that further optimization couldn't make the picture even better).

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Jan 21, 2017

Contributor

ensime is heavy on typeclass derivation, any volunteers willing to take that on? I spent today getting a 2.12 release out..

Contributor

fommil commented Jan 21, 2017

ensime is heavy on typeclass derivation, any volunteers willing to take that on? I spent today getting a 2.12 release out..

@milessabin milessabin referenced this pull request in typelevel/scala Jan 25, 2017

Closed

Prepare 2.12.1 release #129

@adriaanm adriaanm added the WIP label Feb 8, 2017

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Feb 13, 2017

Contributor

FYI: this has been merged in Typelevel Scala 2.12.1.

Contributor

milessabin commented Feb 13, 2017

FYI: this has been merged in Typelevel Scala 2.12.1.

@dotta

This comment has been minimized.

Show comment
Hide comment
@dotta

dotta Feb 13, 2017

Contributor

I've cherry-picked the commits in this PR on top of the v2.12.1 tag to check if a codebase (can't reveal more) that extensively uses shapeless would benefit from the new inductive heuristics. Our (non-extensive) tests showed that on the specific codebase there was a significant (almost 30%) compile time degradation with -Yinduction-heuristics enabled. The Typelevel Scala 2.12.1 release was also tested, and it yields similar results. I'm sharing this just as a data point.

Contributor

dotta commented Feb 13, 2017

I've cherry-picked the commits in this PR on top of the v2.12.1 tag to check if a codebase (can't reveal more) that extensively uses shapeless would benefit from the new inductive heuristics. Our (non-extensive) tests showed that on the specific codebase there was a significant (almost 30%) compile time degradation with -Yinduction-heuristics enabled. The Typelevel Scala 2.12.1 release was also tested, and it yields similar results. I'm sharing this just as a data point.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Feb 13, 2017

Contributor

@dotta Are you able to give an indication of the sizes of the inductions you're seeing in that codebase?

Also, have you verified that the heuristic is actually being applied? You can check that by adding the @inductive annotation to the type class trait. If that results in a warning the the instances aren't being solved for inductively then you'll be experiencing all the overhead of the mechanism without gaining any of the benefits.

Contributor

milessabin commented Feb 13, 2017

@dotta Are you able to give an indication of the sizes of the inductions you're seeing in that codebase?

Also, have you verified that the heuristic is actually being applied? You can check that by adding the @inductive annotation to the type class trait. If that results in a warning the the instances aren't being solved for inductively then you'll be experiencing all the overhead of the mechanism without gaining any of the benefits.

@dotta

This comment has been minimized.

Show comment
Hide comment
@dotta

dotta Feb 13, 2017

Contributor

@milessabin Unfortunately, I can't. All I've tried was turning on the flag and check if compilation time would drop. No further investigation was done.

Contributor

dotta commented Feb 13, 2017

@milessabin Unfortunately, I can't. All I've tried was turning on the flag and check if compilation time would drop. No further investigation was done.

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Feb 13, 2017

Contributor

that flag sounds very useful. A no-op annotation for 2.10/2.11 would be very useful to use it in cross builds. I may do that (but I still await your Lazy fix...)

Contributor

fommil commented Feb 13, 2017

that flag sounds very useful. A no-op annotation for 2.10/2.11 would be very useful to use it in cross builds. I may do that (but I still await your Lazy fix...)

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Feb 13, 2017

Contributor

@dotta I'll ping you offline and we'll see if we can get a bit more information out of this.

Contributor

milessabin commented Feb 13, 2017

@dotta I'll ping you offline and we'll see if we can get a bit more information out of this.

@adriaanm adriaanm modified the milestones: 2.12.2, WIP Feb 16, 2017

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Feb 20, 2017

Contributor

I've followed up with @dotta and it appears that the induction heuristics weren't applicable (due to use of shapeless's Lazy as discussed at the top of this thread) so all he's seeing is the overhead of the applicability test.

There's quite a bit of scope for improving the performance of the applicability test (paging @rorygraves). I was hoping for some feedback on correctness before digging in to tuning ... @adriaanm, @retronym if you're happy on that score I'll starting looking at reducing the cost of the test.

Contributor

milessabin commented Feb 20, 2017

I've followed up with @dotta and it appears that the induction heuristics weren't applicable (due to use of shapeless's Lazy as discussed at the top of this thread) so all he's seeing is the overhead of the applicability test.

There's quite a bit of scope for improving the performance of the applicability test (paging @rorygraves). I was hoping for some feedback on correctness before digging in to tuning ... @adriaanm, @retronym if you're happy on that score I'll starting looking at reducing the cost of the test.

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Feb 20, 2017

Contributor

is there any hope at all of a version of this that works with current usages of Lazy? I know the new language feature you were discussing is far superior... but it doesn't help with existing codebases and will introduce both source and binary incompatible changes to libraries like spray-json-shapeless and circe.

Contributor

fommil commented Feb 20, 2017

is there any hope at all of a version of this that works with current usages of Lazy? I know the new language feature you were discussing is far superior... but it doesn't help with existing codebases and will introduce both source and binary incompatible changes to libraries like spray-json-shapeless and circe.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Feb 20, 2017

Contributor

@fommil that would require explicit knowledge of (though not a dependency on) shapeless in the compiler. If you're willing to lobby for it then I would be more than happy to implement it.

Contributor

milessabin commented Feb 20, 2017

@fommil that would require explicit knowledge of (though not a dependency on) shapeless in the compiler. If you're willing to lobby for it then I would be more than happy to implement it.

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Feb 20, 2017

Contributor

@milessabin just point my cage where it needs to point, and tell me when to start speaking.

Contributor

fommil commented Feb 20, 2017

@milessabin just point my cage where it needs to point, and tell me when to start speaking.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Feb 20, 2017

Contributor

@fommil here is fine :-)

Contributor

milessabin commented Feb 20, 2017

@fommil here is fine :-)

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Feb 21, 2017

Contributor

In that case I'll motivate why it would be so useful.

Miles' work here is most beneficial for speeding up compilation when there are complex implicit derivations matching a particular pattern, such as typeclass derivation. There is a problem in that resolution diverges and fails for recursive types (I assume you all know what that means) and there is a hack in shapeless which introduces Lazy and Strict containers to give hints such that the developer can trick the compiler into implicit derivation of recursive types (caveat emptor).

But unfortunately, Miles' improvements in this PR do not work for Lazy - he says it needs the compiler to be shapeless-aware. This is even more unfortunate, because many typeclass derivation libraries such as spray-json-shapeless, circe, scodec, etc etc, are all designed to handle recursive types even if the developer doesn't have any. Therefore, none of this great work is being passed on to users of those libraries.

So, please let Miles add the hack! There is a real fix coming soon with an associated language change (for by-name implicits), but that is source incompatible with existing code that will likely require making all typeclass derivation libraries 2.12.x+

Today, we actually need something that is source compatible all the way back to 2.10 - library authors still has to cross compile for 2.10, because sbt.

Please somebody ask Eugene very nicely to release sbt 0.13.13 built with scala 2.12 and call it 0.14.0 😃

Contributor

fommil commented Feb 21, 2017

In that case I'll motivate why it would be so useful.

Miles' work here is most beneficial for speeding up compilation when there are complex implicit derivations matching a particular pattern, such as typeclass derivation. There is a problem in that resolution diverges and fails for recursive types (I assume you all know what that means) and there is a hack in shapeless which introduces Lazy and Strict containers to give hints such that the developer can trick the compiler into implicit derivation of recursive types (caveat emptor).

But unfortunately, Miles' improvements in this PR do not work for Lazy - he says it needs the compiler to be shapeless-aware. This is even more unfortunate, because many typeclass derivation libraries such as spray-json-shapeless, circe, scodec, etc etc, are all designed to handle recursive types even if the developer doesn't have any. Therefore, none of this great work is being passed on to users of those libraries.

So, please let Miles add the hack! There is a real fix coming soon with an associated language change (for by-name implicits), but that is source incompatible with existing code that will likely require making all typeclass derivation libraries 2.12.x+

Today, we actually need something that is source compatible all the way back to 2.10 - library authors still has to cross compile for 2.10, because sbt.

Please somebody ask Eugene very nicely to release sbt 0.13.13 built with scala 2.12 and call it 0.14.0 😃

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Mar 29, 2017

Contributor

Ambiguous implicits now handled correctly. Squashed.

Contributor

milessabin commented Mar 29, 2017

Ambiguous implicits now handled correctly. Squashed.

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Mar 29, 2017

Contributor

Any news on Lazy?

Contributor

fommil commented Mar 29, 2017

Any news on Lazy?

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Mar 29, 2017

Contributor

That's not part of this PR.

Contributor

milessabin commented Mar 29, 2017

That's not part of this PR.

@Atry

This comment has been minimized.

Show comment
Hide comment
@Atry

Atry Apr 20, 2017

Contributor

Does this PR perform the same algorithm as lampepfl/dotty#1998 ?

Contributor

Atry commented Apr 20, 2017

Does this PR perform the same algorithm as lampepfl/dotty#1998 ?

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Apr 21, 2017

Contributor

@Atry that's what @fommil was referring to earlier. I'm working on that as a separate PR.

Contributor

milessabin commented Apr 21, 2017

@Atry that's what @fommil was referring to earlier. I'm working on that as a separate PR.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jun 1, 2017

Member

There are a few pending TODO's in the review comments left by Eugene. The key to making this PR reviewable is more docs: the commit message should motivate the change and explain how it works in some detail, and the code also needs more comments and examples. Accepting this PR means taking on responsibility for maintaining this code in the future, which crucially relies on a design doc / spec.

Member

adriaanm commented Jun 1, 2017

There are a few pending TODO's in the review comments left by Eugene. The key to making this PR reviewable is more docs: the commit message should motivate the change and explain how it works in some detail, and the code also needs more comments and examples. Accepting this PR means taking on responsibility for maintaining this code in the future, which crucially relies on a design doc / spec.

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Aug 3, 2017

Member

I've just discovered a funny behaviour with implicits. In this inductive example, turning the type parameter L contravariant slightly speeds up implicit lookup.

These are the results I get for N=30 (EDIT: N=300). I've reproduced for smaller N's too, and I've also made sure to warm up the compiler before getting these behaviours. They're all captured from within sbt.

1. Waiting for source changes... (press enter to interrupt)
[info] Packaging /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/scalac-profiling_2.12-0.1-SNAPSHOT.jar ...
[info] Done packaging.
[info] Compiling 2 Scala sources to /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/test-classes...
[success] Total time: 125 s, completed Aug 3, 2017 1:29:56 PM
2. Waiting for source changes... (press enter to interrupt)
[info] Packaging /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/scalac-profiling_2.12-0.1-SNAPSHOT.jar ...
[info] Done packaging.
[info] Compiling 2 Scala sources to /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/test-classes...
[success] Total time: 108 s, completed Aug 3, 2017 1:31:55 PM
3. Waiting for source changes... (press enter to interrupt)

I've tried this with Typelevel Scala too and there's no observable difference between the two, both compile in 6 seconds (!).

I considered important to let everyone in this ticket know about the results of my experiment. Is this a well-known behaviour? It's the first time I see it, and it doesn't make sense that turning L contravariant speeds up implicit search -- should be the other way around. I wonder, is there something in the implicit search algorithm that we're not considering?

Member

jvican commented Aug 3, 2017

I've just discovered a funny behaviour with implicits. In this inductive example, turning the type parameter L contravariant slightly speeds up implicit lookup.

These are the results I get for N=30 (EDIT: N=300). I've reproduced for smaller N's too, and I've also made sure to warm up the compiler before getting these behaviours. They're all captured from within sbt.

1. Waiting for source changes... (press enter to interrupt)
[info] Packaging /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/scalac-profiling_2.12-0.1-SNAPSHOT.jar ...
[info] Done packaging.
[info] Compiling 2 Scala sources to /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/test-classes...
[success] Total time: 125 s, completed Aug 3, 2017 1:29:56 PM
2. Waiting for source changes... (press enter to interrupt)
[info] Packaging /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/scalac-profiling_2.12-0.1-SNAPSHOT.jar ...
[info] Done packaging.
[info] Compiling 2 Scala sources to /data/rw/code/scala/scala-profiling/plugin/target/scala-2.12/test-classes...
[success] Total time: 108 s, completed Aug 3, 2017 1:31:55 PM
3. Waiting for source changes... (press enter to interrupt)

I've tried this with Typelevel Scala too and there's no observable difference between the two, both compile in 6 seconds (!).

I considered important to let everyone in this ticket know about the results of my experiment. Is this a well-known behaviour? It's the first time I see it, and it doesn't make sense that turning L contravariant speeds up implicit search -- should be the other way around. I wonder, is there something in the implicit search algorithm that we're not considering?

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 3, 2017

Contributor

N=30? Your numbers don't seem quite right to me ... for Lightbend Scala (ie. without -Yinductive-implicits) I don't see compile times like that until about N=300.

Ahh ... except that my numbers are from compilng with bare scalac ... is it possible that you're seeing some SBT related overhead?

Contributor

milessabin commented Aug 3, 2017

N=30? Your numbers don't seem quite right to me ... for Lightbend Scala (ie. without -Yinductive-implicits) I don't see compile times like that until about N=300.

Ahh ... except that my numbers are from compilng with bare scalac ... is it possible that you're seeing some SBT related overhead?

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Aug 3, 2017

Member

Oh, no, sorry -- I meant N=300. I was only counting from 10 to 10.

Member

jvican commented Aug 3, 2017

Oh, no, sorry -- I meant N=300. I was only counting from 10 to 10.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 3, 2017

Contributor

That's more like it :-)

Contributor

milessabin commented Aug 3, 2017

That's more like it :-)

@fommil

This comment has been minimized.

Show comment
Hide comment
@fommil

fommil Aug 16, 2017

Contributor

@milessabin did this separate PR ever happen? Or has this effort been shelved for now?

Contributor

fommil commented Aug 16, 2017

@milessabin did this separate PR ever happen? Or has this effort been shelved for now?

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 16, 2017

Contributor

I'm working on it now.

Contributor

milessabin commented Aug 16, 2017

I'm working on it now.

@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Aug 31, 2017

Contributor

Rebased ...

Contributor

milessabin commented Aug 31, 2017

Rebased ...

@baank

This comment has been minimized.

Show comment
Hide comment
@baank

baank Jan 21, 2018

What's the latest on this ? .. Seems like such an amazing feature to have landed.

baank commented Jan 21, 2018

What's the latest on this ? .. Seems like such an amazing feature to have landed.

milessabin added a commit to milessabin/scala that referenced this pull request Mar 6, 2018

Implementation of byname implicits with recursive dictionaries
This is an implementation of byname implicit parameters with recursive
dictionaries, intended as a language-level replacement for shapeless's
Lazy type. As of this commit, implicit arguments can be marked as byname
and at call sites will be eligible as parameters in recursively nested
positions within their own implicit expansions.

Consider the following example,

  trait Foo { def next: Foo }
  object Foo {
    implicit def foo(implicit rec: => Foo): Foo = new Foo { def next = rec }
  }
  val foo = implicitly[Foo]
  assert(foo eq foo.next)

In current Scala this would diverge due to the recursive implicit
argument rec of method foo. Under the scheme implemented in this commit
recursive occurrences of this sort are extracted out as lazy val
members of a local synthetic object as follows,

  val foo: Foo = scala.Predef.implicitly[Foo](
    {
      object LazyDefns$1 {
        lazy val rec$1: Foo = Foo.foo(rec$1) // recursive knot tied here
      }
      LazyDefns$1.rec$1
    }
  )
  assert(foo eq foo.next)

and the example compiles with the assertion successful.

This general pattern is essential to the derivation of type class
instances for recursive data types, one of shapeless's most common
applications. Byname implicits have a number of benefits over the macro
implementation of Lazy in shapeless,

+ the implementation of Lazy in shapeless is extremely delicate, relying
  on non-portable compiler internals. By contrast there is already an
  initial implementation of byname implicits in Dotty:
  lampepfl/dotty#1998.
+ the shapeless implementation is unable to modify divergence checking,
  so to solve recursive instances it effectively disables divergence
  checking altogether ... this means that incautious use of Lazy can cause
  the typechecker to loop indefinitely. The byname implicits
  implementation is able to both solve recursive occurrences and check for
  divergence.
+ the implementation of Lazy interferes with the heuristics for solving
  inductive implicits in #5649 because the latter depends on being able to
  verify that induction steps strictly reduce the size of the types being
  solved for; the additional Lazy type constructors make the type appear
  be non-decreasing in size. Whilst this could be special-cased, doing so
  would require some knowledge of shapeless to be incorporated into the
  compiler. Being a language-level feature, byname implicits can be
  accommodated directly in the induction heuristics.
+ in common cases more implicit arguments would have to be marked as
  Lazy than would have to be marked as byname under this PR due to
  restrictions on what the Lazy macro is able to do. Given that there is a
  runtime cost associated with capturing the thunks required for both Lazy
  and byname arguments, any reduction in the number is beneficial.

milessabin added a commit to milessabin/scala that referenced this pull request Mar 7, 2018

Implementation of byname implicits with recursive dictionaries
This is an implementation of byname implicit parameters with recursive
dictionaries, intended as a language-level replacement for shapeless's
Lazy type. As of this commit, implicit arguments can be marked as byname
and at call sites will be eligible as parameters in recursively nested
positions within their own implicit expansions.

Consider the following example,

  trait Foo { def next: Foo }
  object Foo {
    implicit def foo(implicit rec: => Foo): Foo = new Foo { def next = rec }
  }
  val foo = implicitly[Foo]
  assert(foo eq foo.next)

In current Scala this would diverge due to the recursive implicit
argument rec of method foo. Under the scheme implemented in this commit
recursive occurrences of this sort are extracted out as lazy val
members of a local synthetic object as follows,

  val foo: Foo = scala.Predef.implicitly[Foo](
    {
      object LazyDefns$1 {
        lazy val rec$1: Foo = Foo.foo(rec$1) // recursive knot tied here
      }
      LazyDefns$1.rec$1
    }
  )
  assert(foo eq foo.next)

and the example compiles with the assertion successful.

This general pattern is essential to the derivation of type class
instances for recursive data types, one of shapeless's most common
applications. Byname implicits have a number of benefits over the macro
implementation of Lazy in shapeless,

+ the implementation of Lazy in shapeless is extremely delicate, relying
  on non-portable compiler internals. By contrast there is already an
  initial implementation of byname implicits in Dotty:
  lampepfl/dotty#1998.
+ the shapeless implementation is unable to modify divergence checking,
  so to solve recursive instances it effectively disables divergence
  checking altogether ... this means that incautious use of Lazy can cause
  the typechecker to loop indefinitely. The byname implicits
  implementation is able to both solve recursive occurrences and check for
  divergence.
+ the implementation of Lazy interferes with the heuristics for solving
  inductive implicits in #5649 because the latter depends on being able to
  verify that induction steps strictly reduce the size of the types being
  solved for; the additional Lazy type constructors make the type appear
  be non-decreasing in size. Whilst this could be special-cased, doing so
  would require some knowledge of shapeless to be incorporated into the
  compiler. Being a language-level feature, byname implicits can be
  accommodated directly in the induction heuristics.
+ in common cases more implicit arguments would have to be marked as
  Lazy than would have to be marked as byname under this PR due to
  restrictions on what the Lazy macro is able to do. Given that there is a
  runtime cost associated with capturing the thunks required for both Lazy
  and byname arguments, any reduction in the number is beneficial.
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Mar 29, 2018

Contributor

@adriaanm I'm going to rebase on top of #6050 so that I can make some progress on this.

Contributor

milessabin commented Mar 29, 2018

@adriaanm I'm going to rebase on top of #6050 so that I can make some progress on this.

milessabin added a commit to milessabin/scala that referenced this pull request Mar 29, 2018

Implementation of byname implicits with recursive dictionaries
This is an implementation of byname implicit parameters with recursive
dictionaries, intended as a language-level replacement for shapeless's
Lazy type. As of this commit, implicit arguments can be marked as byname
and at call sites will be eligible as parameters in recursively nested
positions within their own implicit expansions.

Consider the following example,

  trait Foo { def next: Foo }
  object Foo {
    implicit def foo(implicit rec: => Foo): Foo = new Foo { def next = rec }
  }
  val foo = implicitly[Foo]
  assert(foo eq foo.next)

In current Scala this would diverge due to the recursive implicit
argument rec of method foo. Under the scheme implemented in this commit
recursive occurrences of this sort are extracted out as lazy val
members of a local synthetic object as follows,

  val foo: Foo = scala.Predef.implicitly[Foo](
    {
      object LazyDefns$1 {
        lazy val rec$1: Foo = Foo.foo(rec$1) // recursive knot tied here
      }
      LazyDefns$1.rec$1
    }
  )
  assert(foo eq foo.next)

and the example compiles with the assertion successful.

This general pattern is essential to the derivation of type class
instances for recursive data types, one of shapeless's most common
applications. Byname implicits have a number of benefits over the macro
implementation of Lazy in shapeless,

+ the implementation of Lazy in shapeless is extremely delicate, relying
  on non-portable compiler internals. By contrast there is already an
  initial implementation of byname implicits in Dotty:
  lampepfl/dotty#1998.
+ the shapeless implementation is unable to modify divergence checking,
  so to solve recursive instances it effectively disables divergence
  checking altogether ... this means that incautious use of Lazy can cause
  the typechecker to loop indefinitely. The byname implicits
  implementation is able to both solve recursive occurrences and check for
  divergence.
+ the implementation of Lazy interferes with the heuristics for solving
  inductive implicits in #5649 because the latter depends on being able to
  verify that induction steps strictly reduce the size of the types being
  solved for; the additional Lazy type constructors make the type appear
  be non-decreasing in size. Whilst this could be special-cased, doing so
  would require some knowledge of shapeless to be incorporated into the
  compiler. Being a language-level feature, byname implicits can be
  accommodated directly in the induction heuristics.
+ in common cases more implicit arguments would have to be marked as
  Lazy than would have to be marked as byname under this PR due to
  restrictions on what the Lazy macro is able to do. Given that there is a
  runtime cost associated with capturing the thunks required for both Lazy
  and byname arguments, any reduction in the number is beneficial.
@milessabin

This comment has been minimized.

Show comment
Hide comment
@milessabin

milessabin Mar 29, 2018

Contributor

Moved to #6481.

Contributor

milessabin commented Mar 29, 2018

Moved to #6481.

@milessabin milessabin closed this Mar 29, 2018

@SethTisue SethTisue removed this from the WIP milestone Mar 29, 2018

milessabin added a commit to milessabin/scala that referenced this pull request Apr 3, 2018

Implementation of byname implicits with recursive dictionaries
This is an implementation of byname implicit parameters with recursive
dictionaries, intended as a language-level replacement for shapeless's
Lazy type. As of this commit, implicit arguments can be marked as byname
and at call sites will be eligible as parameters in recursively nested
positions within their own implicit expansions.

Consider the following example,

  trait Foo { def next: Foo }
  object Foo {
    implicit def foo(implicit rec: => Foo): Foo = new Foo { def next = rec }
  }
  val foo = implicitly[Foo]
  assert(foo eq foo.next)

In current Scala this would diverge due to the recursive implicit
argument rec of method foo. Under the scheme implemented in this commit
recursive occurrences of this sort are extracted out as lazy val
members of a local synthetic object as follows,

  val foo: Foo = scala.Predef.implicitly[Foo](
    {
      object LazyDefns$1 {
        lazy val rec$1: Foo = Foo.foo(rec$1) // recursive knot tied here
      }
      LazyDefns$1.rec$1
    }
  )
  assert(foo eq foo.next)

and the example compiles with the assertion successful.

This general pattern is essential to the derivation of type class
instances for recursive data types, one of shapeless's most common
applications. Byname implicits have a number of benefits over the macro
implementation of Lazy in shapeless,

+ the implementation of Lazy in shapeless is extremely delicate, relying
  on non-portable compiler internals. By contrast there is already an
  initial implementation of byname implicits in Dotty:
  lampepfl/dotty#1998.
+ the shapeless implementation is unable to modify divergence checking,
  so to solve recursive instances it effectively disables divergence
  checking altogether ... this means that incautious use of Lazy can cause
  the typechecker to loop indefinitely. The byname implicits
  implementation is able to both solve recursive occurrences and check for
  divergence.
+ the implementation of Lazy interferes with the heuristics for solving
  inductive implicits in #5649 because the latter depends on being able to
  verify that induction steps strictly reduce the size of the types being
  solved for; the additional Lazy type constructors make the type appear
  be non-decreasing in size. Whilst this could be special-cased, doing so
  would require some knowledge of shapeless to be incorporated into the
  compiler. Being a language-level feature, byname implicits can be
  accommodated directly in the induction heuristics.
+ in common cases more implicit arguments would have to be marked as
  Lazy than would have to be marked as byname under this PR due to
  restrictions on what the Lazy macro is able to do. Given that there is a
  runtime cost associated with capturing the thunks required for both Lazy
  and byname arguments, any reduction in the number is beneficial.

@julienrf julienrf referenced this pull request in jvican/jvican.github.io May 30, 2018

Merged

Move file to allow review #1

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