[ConstraintSystem] Add a new stat to be used for expression type chec… #19930
Conversation
…ker performance testing. This counts the number of leaf scopes we reach while solving the constraint sytem, and is a much better measure of the growth of unnecessary work than the total number of scopes opened. There were two tests where I had a difficult time getting scale-test to fit the curve even after adjusting some of the parameters, so I've left those to use the old stat for now.
|
@swift-ci Please smoke test |
lib/Sema/ConstraintSystem.h
Outdated
| @@ -1439,6 +1433,10 @@ class ConstraintSystem { | |||
|
|
|||
| void incrementScopeCounter(); | |||
|
|
|||
| void numLeafScopes(unsigned adjustment); | |||
| void incrementLeafScopes(unsigned increment =1); | |||
lib/Sema/CSStep.cpp
Outdated
| // not been split, so allow each additional component after the | ||
| // first to have a free leaf scope that doesn't count against the | ||
| // total for the system. | ||
| CS.decrementLeafScopes(Components.size() - 1); |
There was a problem hiding this comment.
As far as I understand the behavior, we can only reach "leaf" scope when:
- None of the previous components failed but current did (and components which follow it up are not going to allocate scopes);
- Disjunction choice has been "attempted" without problems - choice constraint has been simplified, but then failed to attempt any type variables;
- Last successful type variable in the path has been bound (because all type variables attempted before it would create intermediate scopes).
So the question is - if we always decrement by the number of components here but not all of the components resulted in at least one recorded "leaf" scope (e.g. one failed so all the rest had to be aborted), wouldn't it mean that this counter would go negative and/or overflow?
There was a problem hiding this comment.
We count something as a leaf scope any time it creates no new scopes. That could be because we found a successful solution or because a constraint failed. So we should always increment this at least once per component.
My hope here is that this counter will provide the same value whether we split a system or not. i.e., if we take a system and solve it without splitting it and hit 10 leafs, I would like to ensure that if we took the same system and split it into three we would also end up with a count of 10 leafs (rather than 10 plus two extra ones for the two successful endpoints in the two new components).
Does that make sense and if so are there still any concerns?
I can look at adding asserts to ensure we never roll over or go negative.
There was a problem hiding this comment.
Take a look at the ComponentStep::take, we don’t allocate scopes if previous components failed...
There was a problem hiding this comment.
In general: very happy to see new instrumentation for a more-accurate count of work the typechecker does! In specific though, a few concerns:
-
If you want this on in the field (or graphed/tracked in CI perf testing) you need to use the UnifiedStatsReporter's AlwaysOnFrontendCounters fields (defined by entries in
include/swift/Basic/Statistics.def, not llvm:Statistic; the latter is compiled-out in release builds. It's also atomic ops and therefore a fair bit more expensive when on. -
The stats-profiling machinery will get confused if counters decrement along the way. They should be monotonically increasing if at all possible (I mean it's not the end of the world if you can't, but it'll make any profiles misleading and wrong).
-
As @xedin mentions I'm a little concerned that the increments and decrements won't balance out, and would be cautious about being 100% sure they do if you're going to leave it as non-monotonic.
|
@graydon I'll take another look at the always-on counters. It wasn't clear to me if these got reported out when doing scale-test, so I did the lazy-programmer approach of copying what was already happening for the total scope count. |
|
@graydon Oh yes, and on decreasing counts - are these being sampled within a constraint system? If not, then I don't think there should be a problem here since they should always be increasing from system to system. |
|
@rudkx All stats get sampled on every construction/destruction of any FrontendStatsTracer. |
In the process, remove the old incrementScopeCounter SWIFT_FUNC_STAT.
|
@xedin @graydon Okay, I reworked this to only use the always-on stats and updated the tests to use that stat instead. The counter only increments, and it accurately counts the number of leaf scopes we hit, but the counter will vary with the number of connected components we find which is unfortunate but not the end of the world. |
|
@swift-ci Please smoke test |
|
@swift-ci please smoke test Linux platform |
…ker performance testing.
This counts the number of leaf scopes we reach while solving the
constraint sytem, and is a much better measure of the growth of
unnecessary work than the total number of scopes opened.
There were two tests where I had a difficult time getting scale-test
to fit the curve even after adjusting some of the parameters, so I've
left those to use the old stat for now.