-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Faster compilation of inductive implicits. #5649
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
Faster compilation of inductive implicits. #5649
Conversation
| case ite: ImplicitTypeError => false | ||
| case _ => true | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is double L in auxiliary a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
| val result = | ||
| if(solvedContext.isFailure) acc | ||
| else { | ||
| val allUndetparams = (context.undetparams ++ context.outer.undetparams ++ solvedContext.undetparams).distinct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/context.undetparams/savedUndetparams/g?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be 2.12.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
| } | ||
|
|
||
| val tree2 = typed(tree1, EXPRmode, wildPt) | ||
| if(tree2.isErroneous) acc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the if(...) style is much less common in scalac than if (...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I'll fix.
| @@ -0,0 +1,196 @@ | |||
| object shapeless { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's a trifle but you write ifs inconsistently. Personally I would always add one space between if and (.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. My normal style is no space, the scalac style is a space. I try to conform to local customs but sometimes slip.
|
Great work! I have two remarks:
|
|
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) |
|
@fommil yes, to verify both backwards and forwards compatibility actually. |
|
@fommil Also there's the integration build for scala-ide. |
|
@smarter agreed on both points. |
|
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? |
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) |
|
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. |
|
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. |
|
Gah, why do I always seem to get those whitelists wrong :-( |
@mpociecha that build is actually not running on 2.12.x, there's no scala-ide for 2.12 yet. |
|
I'm having some trouble compiling the pos test with and without the new option: 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. |
|
@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 |
|
@retronym I'm not able to reproduce that failure running |
|
Things are working after a clean build and Next question: I'm seeing that compilation is slower for |
|
@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 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. |
|
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 |
|
ensime is heavy on typeclass derivation, any volunteers willing to take that on? I spent today getting a 2.12 release out.. |
|
Ambiguous implicits now handled correctly. Squashed. |
|
Any news on Lazy? |
|
That's not part of this PR. |
|
Does this PR perform the same algorithm as scala/scala3#1998 ? |
|
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. |
3bdd4fd to
d3dd25e
Compare
|
I've just discovered a funny behaviour with implicits. In this inductive example, turning the type parameter 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. 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 |
|
N=30? Your numbers don't seem quite right to me ... for Lightbend Scala (ie. without Ahh ... except that my numbers are from compilng with bare scalac ... is it possible that you're seeing some SBT related overhead? |
|
Oh, no, sorry -- I meant N=300. I was only counting from 10 to 10. |
|
That's more like it :-) |
|
@milessabin did this separate PR ever happen? Or has this effort been shelved for now? |
|
I'm working on it now. |
d3dd25e to
6f23ec4
Compare
|
Rebased ... |
|
What's the latest on this ? .. Seems like such an amazing feature to have landed. |
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:
scala/scala3#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 scala#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.
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:
scala/scala3#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 scala#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.
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:
scala/scala3#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 scala#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.
|
Moved to #6481. |
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:
scala/scala3#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 scala#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.
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-heuristicsscalac 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
@inductiveannotation 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@tailrecis a post hoc check that an expected tail call optimization was performed.The use of
@inductiveallows 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'sMappertype class (which recurses through anHListapplying 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 overallMapperinstance can't be resolved. With this algorithm, and in the presence of the@inductiveannotation, 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. Seetest/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
Lazywill notice is that these will not take advantage of this algorithm. In some cases it may be possible to drop the use ofLazy, though not all. I intend to follow this PR with another which provides an interpretation of byname implicit arguments which is equivalent to shapeless'sLazyand 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/inductionwhich validates my performance claims.