Optimize interpolateTypeVars #6331
Conversation
There seemed to have been a large performance regression due to its recent changes. Let's find out whether that's really the case.
|
test performance please |
|
performance test scheduled: 4 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6331/ to see the changes. Benchmarks is based on merging with master (f09550d) |
|
I wonder what's going on with the benchmarks? We see a lot more volatility than wee used to. This PR seems to improve the situation somewhat for Dotty compilation, but we still have no clue what caused the dramatic degradation in #6301 - it does not look that the changes in this commit were to blame alone. EDIT: #6333 demonstrated that the changes in this commit were indeed to blame alone. The benchmarks were accurate. |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
- drop -- since it is not used anymore. - optimize ++ by special casing empty sets and large, array-backed sets.
|
I think the bugfix is revealing some underlying bug, when compiling Dotty there's only a few cases where the old check returned false and the new returned true, but a couple of those have a crazy amount of type variables: state: TS[7389, 7388, 7387]
state.ownedVars: 28
locked: 28
state: TS[7389, 7388, 7387]
state.ownedVars: 344
locked: 344
state: TS[7389, 7388, 7387]
state.ownedVars: 344
locked: 344
state: TS[7389, 7388, 7387]
state.ownedVars: 344
locked: 345 |
|
Would be interesting to find out for what inputs these large sets are created. |
|
It looks like all these type variables are created using |
They get added to typerState.ownedVars and subsequently |
To compare with scala#6331.
Yes, but what is the source file that causes so many dependent TypeVars to be created? I verified that when compiling all files in typer the maximum ownedVars set size is 9. So it must be some particular circumstance in the source that causes the set to explode. |
|
Most seem to be either in |
|
That makes sense. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6331/ to see the changes. Benchmarks is based on merging with master (2eebb70) |
Bring back -- but optimize it for the case where the sets are similar.
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/6331/ to see the changes. Benchmarks is based on merging with master (832b140) |
|
OK, we are back to normal. We could still check whether we have to create so many ownedVars in the first place. |
|
I propose to merge this now to get back a stable baseline for other performance tests. |
| (this /: that.toList)(_ - _) | ||
| if (this.size == 0) that | ||
| else if (that.size == 0) this | ||
| else concat(that) |
There was a problem hiding this comment.
Why have both concat and ++, with some extra checks in ++ ?
There was a problem hiding this comment.
I originally thought of putting concat in Set 1-3, so wanted to factor put the common behavior. But the way it is now a single method is better.
| var j = searchStart // search thatElems in round robin fashion, starting one after latest hit | ||
| var missing = false | ||
| while (!missing && elem != thatElems(j)) { | ||
| j = (j + 1) % thatSize |
There was a problem hiding this comment.
% is fairly expensive in cpu cycles, I would do:
j += 1
if (j == thatSize) j = 0| val elem = this.xs(i) | ||
| var j = searchStart // search thatElems in round robin fashion, starting one after latest hit | ||
| var missing = false | ||
| while (!missing && elem != thatElems(j)) { |
There was a problem hiding this comment.
| while (!missing && elem != thatElems(j)) { | |
| while (!missing && elem `ne` thatElems(j)) { |
Yes, I saw that. I don't think we need to optimize it since it's in the outer loop. |
There seemed to have been a large performance regression due to its recent changes. Let's find
out whether that's really the case.