Skip to content

Fix #9504: Cache typeSize and covering set #9559

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

Merged
merged 9 commits into from
Aug 14, 2020

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 13, 2020

The problem with #9504 is that types in the search history grow larger over time, since they contain type variables
that get instantiated with new types containing new type variables and so on. That's why divergence checking is fooled
into believing that the newly searched type is always smaller than the history.

To fix this, we need to take a snapshot of type size and covering set at the time the search history is
created.

This is much easier to do with the performance refactorings for search histories I did recently in #9409. As a first step, this commit contains those refactorings.

The overwhelming majority of TermRefSets in implicit search has
a single prefix per symbol (in the case of typer.scala: 100%).
Optimize for this case.
Avoid a separate list of candidate/type pairs. Instead integrate them as
fields of the search history itself.
Move core operations from SearchHistory to ImplicitSearch. This gives more
opportunities for sharing and optimizations.
No need to query recursiveRef twice
@odersky
Copy link
Contributor Author

odersky commented Aug 13, 2020

test performance please

@dottybot
Copy link
Member

performance test scheduled: 4 job(s) in queue, 1 running.

The problem with scala#9504 is that types in the search history grow larger over time, since they contain type variables
that get instantiated with new types containing new type variables and so on. That's why divergence checking is fooled
into believing that the newly searched type is always smaller than the history.

To fix this, we need to take a snapshot of type size and covering set at the time the search history is
created.
@odersky
Copy link
Contributor Author

odersky commented Aug 13, 2020

test performance please

@dottybot
Copy link
Member

performance test scheduled: 5 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9559/ to see the changes.

Benchmarks is based on merging with master (550e969)

1 similar comment
@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9559/ to see the changes.

Benchmarks is based on merging with master (550e969)

@@ -239,23 +239,20 @@ object Implicits:
Nil
else
val nestedCtx = ctx.fresh.addMode(Mode.TypevarsMissContext)
val candidates = new mutable.ListBuffer[Candidate]
var extensionOnly = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that using a var here means that the tryCandidate closure can be reused, but the trade-off is that this var will become a BooleanRef and thus be allocated too, so I don't think we're gaining anything. Maybe instead extensionOnly and tryCandidate could be put in an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I think it's fine to use two closures instead.

@smarter smarter assigned odersky and unassigned smarter Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants