[Constraint graph] Eliminate adjacency information in the graph nodes. #26347
Conversation
The constraint graph maintains a fairly heavyweight list of adjacencies that is only used in two operations: connected components and gathering related constraints. Switch connected components over to primarily use the set of constraints (which are necessary for many reasons), reducing the need for the adjacencies list.
…adjacencies list. Use the adjacencies implied by the constraints of a node rather than looking at the "adjacency" list, and try to simplify this code path a bit. The diagnostic change is because we are now uniformly recording the members of the equivalence class.
Each constraint graph node maintained adjacency information that was fairly expensive---a vector of type variables and a dense map of extra adjacency information---and that was continuously maintained. Remove this adjacency information, instead recomputing it by walking the constraints (to get their type variables) and having a separate (smaller) list of type variables that are adjacent due to fixed bindings. This simplifies the constraint graph code and reduces per-node memory overhead.
Simplify the interface to gatherConstraints() by performing the uniquing within the function itself and returning only the resulting (uniqued) vector of constraints.
|
@swift-ci please smoke test |
|
@swift-ci please test compiler performance |
|
Awesome! |
| // If we want all mentions, visit type variables within each of our | ||
| // constraints. | ||
| if (kind == GatheringKind::AllMentions) { | ||
| for (auto adjTypeVar : constraint->getTypeVariables()) { |
There was a problem hiding this comment.
I think the idea behind caching adjacency is based on the observation that we add constraints a lot less often than we gather them (e.g. to determine bindings), this algorithm is going to be less efficient (maybe not really reflecting in the wall time) because it would iterate over the same equivalence class N times based on the number of unique constraints in each node because equivalence class always includes "self".
There was a problem hiding this comment.
On the other hand, DenseMap in particular is quite heavyweight in terms of memory usage, even when empty, IIRC
There was a problem hiding this comment.
I wonder if that size is justified vs. having to allocate/grow and destruct sets of constraints every time we gather which is very often... I'm not saying that I'm against this, just thought that this is worse mentioning.
There was a problem hiding this comment.
I was looking at gatherConstraints, and it was a pretty expensive operation both before and after this change, because it is grabbing constraints from a number of different adjacent type variables and uniquing them along the way. This new implementation is doing basically the same work, but instead of getting the type variables from the stored "adjacencies", we get them from the constraints we are already walking. We do the same uniquing of constraints and of type variables.
This simplification has another purpose, which is to make it easier to ignore specific constraints when performing connected components, so that we can handle one-way constraints specially (#25983).
|
Hm, this is interesting, I wouldn't expect number of scopes to go down with these changes... |
There was one small behavioral change in this rewrite, which is that we weren't looking at constraints for type variables in the equivalence class of adjacent type variables, which could be responsible for the reduction in the number of scores... I think that behavioral change is properly captured by a separate PR I just put up at #26368. |
|
My worry about that is related to how we determine bindings based on these constraints, if we are not getting as many constraints as before, it might mean we’ll not be able to find some solutions we do today. |
|
We are getting more constraints, not fewer, because we were artificially skipping related constraints. |
|
Ah, I misread your previous comment. I see what you mean now, hopefully that doesn't cause any source compatibility regressions. |
| case .init(opt: 0): break // expected-error{{value of optional type 'StaticMembers?' must be unwrapped to a value of type 'StaticMembers'}} | ||
| // expected-note@-1{{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} | ||
| // expected-note@-2{{coalesce using '??' to provide a default when the optional value contains 'nil'}} | ||
| case .init(opt: 0): break // expected-error{{pattern cannot match values of type 'StaticMembers'}} |
There was a problem hiding this comment.
Looks like removing UR_LabelingMismatch and related changes fixed this diagnostic back to diagnose as missing force unwrap.
Each constraint graph node maintained adjacency information that was
fairly expensive---a vector of type variables and a dense map of extra
adjacency information---and that was continuously maintained. Remove
this adjacency information, instead recomputing it by walking the
constraints (to get their type variables) and having a separate
(smaller) list of type variables that are adjacent due to fixed
bindings. This simplifies the constraint graph code and reduces
per-node memory overhead.