Skip to content

More efficient garbage collection of type variables #4447

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 1 commit into from
May 8, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented May 2, 2018

When narrowing a constraint, we check if its bounds are now equal to
remove it from the constraint set. This allows the corresponding type
variable to be instantiated in TyperState#gc instead of accumulating
uninstantiated type variables that we traverse again and again in
Inferencing#interpolateTypeVars.

This reduces the running time of the added testcase (copied from
scala/scala#6580) from ~6 seconds to ~4 seconds.
If we fully uncomment the testcase (and increase the JVM stack size),
the running time goes from > 300 seconds to ~24 seconds. This is not as
good as what scala/scala#6580 achieves but is
already much more reasonable that what we did before.

@smarter
Copy link
Member Author

smarter commented May 2, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented May 2, 2018

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

@smarter smarter requested a review from odersky May 2, 2018 22:08
@smarter
Copy link
Member Author

smarter commented May 2, 2018

Two tests fail due to what looks like underlying issues to me, I'd appreciate some guidance here:

cannot merge Constraint(
 uninstVars = A;
 constrained types = [A, B](elems: (A, B)*): Map[A, B]
 bounds = 
     A >: Char
     B := Boolean
 ordering = 
) with Constraint(
 uninstVars = Boolean;
 constrained types = [A, B](elems: (A, B)*): Map[A, B]
 bounds = 
     A := String
     B <: Boolean
 ordering = 
), mergeEntries( >: Char, String)
        at dotty.tools.dotc.core.OrderingConstraint.mergeEntries$1(OrderingConstraint.scala:532)

The fix in #4276 is not enough anymore since we now try to merge a TypeBounds and a non-TypeBounds. The underlying issue is that we try to commit() a typerstate with errors in https://github.com/lampepfl/dotty/blob/a3c4ec793545fd2b922eb469f7fc4d4a7d26b05f/compiler/src/dotty/tools/dotc/typer/Applications.scala#L732, we should either not do that or handle errors gracefully in OrderingConstraint#&#mergeEntries.

@dottybot
Copy link
Member

dottybot commented May 3, 2018

Performance test finished successfully:

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

Benchmarks is based on merging with master (d0f7846)

@odersky
Copy link
Contributor

odersky commented May 8, 2018

For tests/pos/i1590.scala can we simply not eliminate constraints if they contain wildcard types? Or else do the approximation on elimination as well? We do get these constraints because we approximate type variables in expected implicit types and values with wildcard types in order to get good cache hit rates for implicit eligibility caches.

Fir tests/neg/i4272.scala I agree, we should not commit, but simply flush the reporter.

When narrowing a constraint, we check if its bounds are now equal to
remove it from the constraint set. This allows the corresponding type
variable to be instantiated in TyperState#gc instead of accumulating
uninstantiated type variables that we traverse again and again in
Inferencing#interpolateTypeVars.

This reduces the running time of the added testcase (copied from
scala/scala#6580) from ~6 seconds to ~4 seconds.
If we fully uncomment the testcase (and increase the JVM stack size),
the running time goes from > 300 seconds to ~24 seconds. This is not as
good as what scala/scala#6580 achieves but is
already much more reasonable that what we did before.
@smarter smarter force-pushed the fix/constraint-simplification branch from 4797b8a to eeb5965 Compare May 8, 2018 15:55
@smarter
Copy link
Member Author

smarter commented May 8, 2018

After further discussion, here's the fixes we agreed on and that are now implemented:

  • Don't instantiate a type that contains a wildcard, because we don't know which bound to pick (wildcards in bounds can arise because an expected type can contain a wildcard, they shouldn't arise in implicit search since we always use TypevarsMissContext there)
  • When merging constraints a and b, recover from errors if b is known to be erroneous.

@odersky odersky merged commit 474e79a into scala:master May 8, 2018
@allanrenucci allanrenucci deleted the fix/constraint-simplification branch May 8, 2018 21:45
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.

3 participants