Skip to content

Compilation is very slow on some files due to implicit search repeatedly diverging #14333

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

Closed
kubukoz opened this issue Jan 24, 2022 · 6 comments · Fixed by #14353
Closed

Compilation is very slow on some files due to implicit search repeatedly diverging #14333

kubukoz opened this issue Jan 24, 2022 · 6 comments · Fixed by #14353
Labels
area:implicits related to implicits itype:bug itype:performance stat:needs minimization Needs a self contained minimization

Comments

@kubukoz
Copy link
Contributor

kubukoz commented Jan 24, 2022

Compiler version

3.1.0 / 3.1.1

Minimized code

// using "org.typelevel" %% "cats-effect-kernel" % "3.3.4"
import cats.effect.kernel.Resource

import cats.syntax.apply.catsSyntaxApply
import cats.Monad

object middlewares {

  def withToken[F[_]: Monad](getToken: F[Unit]): Resource[F, Unit] =
    Resource.eval(getToken) *> Resource.eval(getToken)

}

Expectation

Sub-second recompilation when there are no changes in the file except whitespace.

Reality

It takes 7-8 seconds to compile this file on a fresh JVM. Recompilation also takes nearly as much, and the presentation compiler is affected as well (which is visible when you e.g. attempt to get completions in Metals).

Notes

I think the *> call is the culprit here. Removing it fixes the issue.

Scala 2.13.8 compiles this in 2 seconds with sub-second incremental recompilations.

@kubukoz kubukoz changed the title Incremental compilation is very slow on some files Compilation is very slow on some files Jan 24, 2022
@smarter smarter added area:implicits related to implicits itype:bug labels Jan 24, 2022
@smarter
Copy link
Member

smarter commented Jan 24, 2022

It looks like the compiler is spending a ton of time doing a bunch of implicit searches which all diverge:

...
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForIorT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForWriterT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForReaderWriterStateT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForIorT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForWriterT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForReaderWriterStateT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForWriterT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForReaderWriterStateT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForEitherT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForOptionT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForOptionT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForKleisli),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForEitherT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForStateT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForOptionT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForKleisli),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForEitherT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForStateT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForIorT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForWriterT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForReaderWriterStateT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForWriterT),1,0)
diverged: Candidate(TermRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class effect)),object kernel),object Sync),method syncForReaderWriterStateT),1,0)
... continues for thousands lines

I have no idea what these implicits do but they look like:

implicit def syncForOptionT[F[_]](implicit F0: Sync[F]): Sync[OptionT[F, *]] =

So I guess whenever the compiler tries one, to see if it can be applied, it ends up doing a recursive implicit search where all these implicits are again possible candidates, and so we get an exponential blow-up. I think we'll need someone to produce a minimization to figure out if this is a bug in scala 3 or an intentional difference in implicit resolution. See #13838 for a similar case, and in particular in #13838 (comment) @joroKr21 suggested there might be situations where we do implicit search that cannot possibly succeed, I don't know if this is the same issue.

@smarter smarter added the stat:needs minimization Needs a self contained minimization label Jan 24, 2022
@smarter smarter changed the title Compilation is very slow on some files Compilation is very slow on some files due to implicit search repeatedly divering Jan 24, 2022
@smarter smarter changed the title Compilation is very slow on some files due to implicit search repeatedly divering Compilation is very slow on some files due to implicit search repeatedly diverging Jan 24, 2022
@kubukoz
Copy link
Contributor Author

kubukoz commented Jan 24, 2022

Noteworthy: Resource.eval[F, Unit](getToken) is all right. The method doesn't take any implicits... so I assume type inference of eval's type parameters doesn't happen fully before *>'s type parameters are inferred?

smarter added a commit to dotty-staging/dotty that referenced this issue Jan 25, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.
smarter added a commit to dotty-staging/dotty that referenced this issue Jan 25, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.
smarter added a commit to dotty-staging/dotty that referenced this issue Jan 25, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.
smarter added a commit to dotty-staging/dotty that referenced this issue Jan 25, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.
@smarter smarter linked a pull request Jan 25, 2022 that will close this issue
@smarter
Copy link
Member

smarter commented Jan 25, 2022

@dwijnand removed the itype:bug label

IMO this is a severe enough regression that this qualifies as a bug. Luckily I think I have a fix: #14353

smarter added a commit to dotty-staging/dotty that referenced this issue Jan 25, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.

Fixes scala#14333.
@kubukoz
Copy link
Contributor Author

kubukoz commented Jan 26, 2022

Your fix works :) tried with a publishLocal and this compiles much faster now.

@dwijnand
Copy link
Member

IMO this is a severe enough regression that this qualifies as a bug.

I consider performance bugs as bugs. But I guess you could argue there are performance enhancements that aren't bugs? Also, I saw that the templates marks crashes as bugs, which seemed redundant to me. Surely all crashes are bugs?!

@smarter
Copy link
Member

smarter commented Jan 26, 2022

Yeah our categorization system isn't great.

Xavientois pushed a commit to Xavientois/dotty that referenced this issue Feb 2, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.

Fixes scala#14333.
olsdavis pushed a commit to olsdavis/dotty that referenced this issue Apr 4, 2022
In some situations we only call `constrainResult` for its side-effects
and ignore its result, but `constrainResult` calls
`necessarilyCompatible` which will call `viewExists` as a last-try.
Since `viewExists` doesn't have side-effects we might as well skip it,
and it turns out that using `NoViewsAllowed.constrainResult` does
exactly that.

This leads to a significant speed-up (from ~8s to sub-second with a hot
compiler) with the test case from scala#14333 (most likely this is because at
the point where we call `constrainResult` we haven't accumulated any
constraint from the arguments of the application, so implicit search is
free to go on a wild goose chase).

I also removed obsolete comments in this method.

Fixes scala#14333.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:implicits related to implicits itype:bug itype:performance stat:needs minimization Needs a self contained minimization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants