Fix inefficieny in the presence of aliasing. #984
Conversation
| secondTry(tp1, tp2) | ||
| implicit val ctx: Context = this.ctx | ||
| tp2.info match { | ||
| case info2: TypeAlias => firstTry(tp1, info2.alias) |
There was a problem hiding this comment.
It seems like you could avoid a bunch of recursive calls to firstTry by using a utility function like:
def dealiasTypeRef(tp: TypeRef): Type =
if (tp.symbol.isClass) tp
else tp.info match {
case TypeAlias(alias: TypeRef) => dealiasTypeRef(alias)
case TypeAlias(alias) => alias
case _ => tp
}
|
/rebuild |
|
Yes, but would this be faster? I noticed we had some deteriorations in On Sun, Nov 29, 2015 at 5:47 PM, Guillaume Martres <notifications@github.com
Martin Odersky |
|
It should be as fast, but less recursive calls mean less risk of deep subtype errors |
@SethTisue @adriaanm : Help 😦 |
|
Note that the two calls in question are tailcalls anyway. |
Yes, but TypeComparer.scala:235: could not optimize @tailrec annotated method firstTry: it contains a recursive call not in tail position
secondTry(tp1, tp2)
^ |
|
@smarter Yes, you are right. It's not a recognized as a tailcall because it One thing we could try is simply call firstTry with dealiased types. That On Sun, Nov 29, 2015 at 6:13 PM, Guillaume Martres <notifications@github.com
Martin Odersky |
|
I just tried it and it seems to be about as fast as your patch, I'll make a PR |
|
@odersky : I made a commit to avoid the recursive calls to |
| @@ -417,29 +414,11 @@ class TypeApplications(val self: Type) extends AnyVal { | |||
|
|
|||
| /** If this is the image of a type argument to type parameter `tparam`, | |||
There was a problem hiding this comment.
This still refers to tparam even though tparam is not a parameter of this method anymore. I think the comment could be simplified to just If this is the image of a type argument for a type parameter, recover the type argument, otherwise NoType., although I'd appreciate it if you could explain the meaning of image in the documentation.
|
Otherwise LGTM. |
|
Could you do the change and commit please? It's currently hard for me to On Mon, Nov 30, 2015 at 3:47 PM, Guillaume Martres <notifications@github.com
Martin Odersky |
|
Sure! |
|
/rebuild |
2 similar comments
|
/rebuild |
|
/rebuild |
…pilation compileStdLib went from 45 sec to 230 sec. The problem were many redundant tests when every member of an alias chain was compared to every other. The new scheme follows alias chains to their end before doing anything else.
I noted a slowdown of about 25% (66sec -> 81sec) when compiling dotty even after the subtype optimization (before it was 117sec). I tracked it down to the traceIndented fix which avoided questions to be evaluated twice. But it also caused the question to be evaluated Making the val lazy fixed the problem.
It turns out it's not needed because now all type arguments are expressed as aliases. Interestingly dropping this feature shaved 20% off the time off junit tests. Which seems to indicate that the handling of type application is really performance critical.
|
I will go fix the disk space problem now. |
smarter
force-pushed
the
optimize-subtyping
branch
from
5339faf to
9e7e40b
Compare
|
@SethTisue : Hurray, thanks :) |
|
/rebuild |
Fix inefficieny in the presence of aliasing.
The change to do compareAlias early caused a dramatic slowdown of compilation
compileStdLib went from 45 sec to 230 sec. The problem were many redundant tests
when every member of an alias chain was compared to every other.
The new scheme follows alias chains to their end before doing anything else. Review by @smarter