-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Attempt to fix performance regression from #20120 #20200
Conversation
@@ -815,6 +815,7 @@ class OrderingConstraint(private val boundsMap: ParamBounds, | |||
newConstraint(hardVars = this.hardVars + tv) | |||
|
|||
def instType(tvar: TypeVar): Type = entry(tvar.origin) match | |||
case TypeBounds(lo, hi) if lo == hi => lo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very interesting observation.
I encountered a similar problem when working on #19955.
I believe the part of the code attempting to do this logic is
scala3/compiler/src/dotty/tools/dotc/core/ConstraintHandling.scala
Lines 304 to 311 in 63810dc
val bound = legalBound(param, rawBound, isUpper) | |
val oldBounds @ TypeBounds(lo, hi) = constraint.nonParamBounds(param) | |
val equalBounds = (if isUpper then lo else hi) eq bound | |
if equalBounds && !bound.existsPart(_ eq param, StopAt.Static) then | |
// The narrowed bounds are equal and not recursive, | |
// so we can remove `param` from the constraint. | |
constraint = constraint.replace(param, bound) | |
true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that this happens after we search for implicits and, in result, commit the new typerstate, in the version without the intersection type the constraint was consisting of a single type, so it was easily removed. For intersection types, we had that weird TypeBound with equal lower and upper bounds, so the constraint was kept and we later compiled with an unfulfilled type variable, causing the bad performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the TypeBound looking like?
It's generally not recommended to use ==
for types, since it has no good meaning. Types are highly recursive. ==
risks either running into an infinite recursion or missing some equalities. So the only sanctioned equalities are eq
(fast, incomplete) and =:=
(slow, complete, but prone to cycles).
So maybe one can use a custom mixture of structural comparison of AndTypes and eq
comparison for their operands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's TypeBounds(Repro.ObjectId & Endpoints.this.Foo, Repro.ObjectId & Endpoints.this.Foo)
, or TypeBounds(AndType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),module class Repro$)),trait ObjectId),TypeRef(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),module class Repro$)),class Endpoints)),trait Foo)),AndType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),module class Repro$)),trait ObjectId),TypeRef(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),module class Repro$)),class Endpoints)),trait Foo)))
The eq
comparison in place of ==
is a sufficient fix for the reproduction in #20120, but I am wondering where this constraint (TX1 >: Repro.ObjectId & Endpoints.this.Foo <: Repro.ObjectId & Endpoints.this.Foo
) is created (and will try to investigate that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be that one of the sides contained a type variable that got instantiated. But then I think @EugeneFlessele's latest fix would apply. Unless Eugene finds the root cause and fixes it otherwise, we can go with the eq
comparison in instType
. Maybe that turns out to be the simplest solution. Does eq
also suffer from the #6505 failure?
It's just a single test that fails, and looking at the history we never really understood why it works now and failed previously. So it's worth digging some more here. In principle, it would be nice if we could do the improvement in this PR. Or maybe thge weaker form I suggested above:
|
Fixed in #20399 |
Attempts to fix #20120.
I'll try to prepare an explanation later today/tomorrow, for now let's see if it will pass CI (it does fix the performance in the minimization)
Also, there may be better ways of achieving the same effect (like not producing redundant TypeBounds before committing the typerState, I'll look into this later).