Skip to content

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