Implicit scope caching: bug fixes and performance improvements #1288
Conversation
The issue is subtle: the `tp` in scope in `def ofTypeImplicits` is the `tp` passed to the top-level `implicitScope` method, not the `tp` passed to the recursively called `iscope`, this means that before this commit, all intermediate `OfTypeImplicit` scopes cached while computing an implicit scope had their `tp` field incorrectly set, which means that we could miss implicits in later implicit searches. Note that the `implicit_cache.scala` test worked before this commit because of the restrictions on caching that exist since b8b0f38, it is included anyway because our caching strategy might change in the future.
This did not work before because we incorrectly looked for their value in the prefix of the type instead of the type itself.
According to SLS § 7.2, self types are not a named part of a type, so they're not part of the implicit scope. Before this commit, this was usually the case because we normally refer to a class using a TypeRef, but in some cases a class might appear as a ThisType, and ThisType#underlying returns the self type, we now use ThisType#tref instead which just returns a TypeRef corresponding to the class.
Many intermediate `OfTypeImplicits` are created during a call to `implicitScope`, but they won't all be used so there is no need to compute `OfTypeImplicits#refs` unless it's actually used.
This reduces the number of implicit scopes we cache.
smarter
force-pushed
the
fix/implicit-caching-2
branch
from
1c0d8e6 to
7a060ba
Compare
| isApplicable(alt, targs, args, resultType) | ||
| ) | ||
| else | ||
| alts2 |
There was a problem hiding this comment.
That changes the logic of overloading resolution. With the change, we prefer methods that are applicable without implicit conversions to other methods. That's not as specced I think. What is the motivation for the change?
There was a problem hiding this comment.
From the commit message of 7a060ba:
If a directly applicable alternative exists, there is no need to try
every alternative (which would require searching for implicit
conversions for every argument that doesn't directly match). This should
not affect semantics since applicable overloads that require some
implicit conversions are dismissed innarrowMostSpecific
The motivation for this change is that I observed that many of the implicit searches when compiling dotty were caused by overloading resolution (e.g., every call to Int#+ will check if the argument can be converted to Char,Short,Double, etc even if the argument is an Int) It's possible that I missed a case where this does change the semantics, if so could you give an example?
|
@smarter I was thinking something along these lines: trait A; trait B Under the old rules this is ambiguous. Under your new rules, C1#f would win. Which is reasonable, but not the way I remember the spec. On the other hand, |
|
Everything else LGTM |
Not really, it seemed to shave at least a second on |
|
The spec can be changed quite easily. And it's true that there are cases On Fri, Jun 3, 2016 at 6:52 PM, Guillaume Martres notifications@github.com
Prof. Martin Odersky |
|
OK, I'll add your example as a pos test to the commit that does the change and merge. Looking at the spec it seems we would just need to define what "directly applicable" means and use that in the overloading resolution secton |
If directly applicable alternatives exists, do not try other alternatives. The original motivation for this change was to reduce the number of searches for implicit views we do since some overloaded methods like `Int#+` are used a lot, but it turns out that this also makes more code compile (see `overload_directly_applicable.scala` for an example), this change does not seem to match what the specification says (it does not define a notion of "directly applicable") but it does match the behavior of scalac, and it seems useful in general.
smarter
force-pushed
the
fix/implicit-caching-2
branch
from
7a060ba to
295c981
Compare
Review by @odersky