Optimize lub algorithm #20034
Merged
Optimize lub algorithm #20034
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Simplify had some elaborate condition that prevented hard union types to be recomputed with a lub. I am not sure why that was. In the concrete scenario of i10693.scala, we had an explicitly union result type `B | A` where `A` and `B` are type parameters. So that is a hard union type. Then `A` was instantiated by `Int | String` and `B` was instantiated by `String | Int`. Re-forming the lub of that union would have eliminated one pair, but since the union type was hard tyat was not done. On the other hand I see no reason why hard unions should not be re-lubbed. Hard unions are about preventing the widening of or types with a join. I don't see a connection with avoiding re-lubbing. Fixes scala#10693
Replace mergeIfSuper by a different algorithm that is more efficient. We drop or-summands in both arguments of a lub that are subsumed by the other. This avoids expesnive recusive calls to lub or expensive comparisons with union types on the right.
|
Based on #20027. Last commit is new. |
LucySMartin
reviewed
Mar 27, 2024
| @@ -157,15 +157,8 @@ object TypeOps: | |||
| tp.derivedAlias(simplify(tp.alias, theMap)) | |||
| case AndType(l, r) if !ctx.mode.is(Mode.Type) => | |||
| simplify(l, theMap) & simplify(r, theMap) | |||
There was a problem hiding this comment.
Should this be calling into TypeComparer.glb It just seems odd to be altering the Or case, but not the And case.
There was a problem hiding this comment.
& and glb are in fact the same. It's just for lub that we need the additional isSoft parameter.
noti0na1
reviewed
Mar 27, 2024
| case _ => | ||
| NoType | ||
| } | ||
| private def dropIfSub(tp: Type, sup: Type, canConstrain: Boolean): Type = |
There was a problem hiding this comment.
The new operations are much clearer than the original mergeIfs. 👍🏼
|
test performance please |
|
performance test scheduled: 96 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/20034/ to see the changes. Benchmarks is based on merging with main (43ed9fd) |
noti0na1
approved these changes
Mar 28, 2024
| case _ => | ||
| NoType | ||
| } | ||
| private def dropIfSub(tp: Type, sup: Type, canConstrain: Boolean): Type = |
There was a problem hiding this comment.
Add a comment:
/** If some (|-operand of) `tp` is a subtype of `sup` replace it with `NoType`. */
odersky
commented
Mar 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace mergeIfSuper by a different algorithm that is more efficient.
We drop or-summands in both arguments of a lub that are subsumed by the other.
This avoids expensive recursive calls to lub or expensive comparisons
with union types on the right.
I tested the previous performance regression #19907 with the new algorithm, and without the changes in #19995 that avoid a slow lub. Where previously it took minutes it now compiles fast.
Specifically, we get for i19907_slow_1000_3.scala: 2.9s with the optimizations in #19995, 3.3s with just this PR. And for i19907_slow_1000_4.scala: 3.9s with the optimizations in #19995, 4.5s with just this PR. So the optimizations in #19995 are much less critical now since lubs are much faster. Still, it's probably worthwhile to leave them in in case there is a humongous program that stresses lubs even more.