go/types, types2: detect instantiation cycles in linear time
The monomorphization check looks for a positive-weight cycle in a
weighted digraph of type parameter dependencies. It previously used a
Bellman-Ford variant whose worst case is O(V*E), noticeable on
packages with very many small generic declarations: the 100,000-type
example in the issue takes ~26s to report its error.
Instead, partition the vertices into strongly connected components:
a positive-weight cycle exists if and only if some weight-1 edge has
both endpoints in a single component. Such an edge, together with a
path from its destination back to its source, forms a cycle of weight
at least 1; conversely, the vertices of any cycle are mutually
reachable, and a cycle of 0/1-weight edges can only have positive
weight if it contains a weight-1 edge.
The components are computed with the same algorithm already used by
cmd/compile/internal/ir to group mutually recursive functions
(Sedgewick, Algorithms, Second Edition, p. 482), and detection is a
single scan of the edges, so the check now runs in O(V+E). The
100,000-type example reports its error in about a second, almost all
of it parsing.
Diagnostics are unchanged: the cycle is reconstructed by
breadth-first search through the offending edge, so a shortest cycle
through that edge is reported, and (resolving an old TODO) it is
pivoted to start at its earliest-declared vertex. This reproduces the
previous output on all existing tests and on multi-vertex cycles
checked by hand.
Fixes #80378
diff --git a/src/cmd/compile/internal/types2/mono.go b/src/cmd/compile/internal/types2/mono.go
index 1263507..ac7c4ca 100644
--- a/src/cmd/compile/internal/types2/mono.go
+++ b/src/cmd/compile/internal/types2/mono.go
@@ -62,10 +62,6 @@
}
type monoVertex struct {
- weight int // weight of heaviest known path to this vertex
- pre int // previous edge (if any) in the above path
- len int // length of the above path
-
// obj is the defined type or type parameter represented by this
// vertex.
obj *TypeName
@@ -80,70 +76,149 @@
}
func (check *Checker) monomorph() {
- // We detect unbounded instantiation cycles using a variant of
- // Bellman-Ford's algorithm. Namely, instead of always running |V|
- // iterations, we run until we either reach a fixed point or we've
- // found a path of length |V|. This allows us to terminate earlier
- // when there are no cycles, which should be the common case.
+ // A positive-weight cycle exists if and only if some weight-1 edge
+ // starts and ends within a single strongly connected component: such
+ // an edge together with a path from its destination back to its
+ // source forms a cycle, of weight at least 1; and conversely, the
+ // vertices of any cycle are mutually reachable, while a cycle's
+ // weight can only be positive if the cycle contains a weight-1 edge.
- again := true
- for again {
- again = false
+ // out[v] lists the edges leaving vertex v.
+ out := make([][]int, len(check.mono.vertices))
+ for i, edge := range check.mono.edges {
+ out[edge.src] = append(out[edge.src], i)
+ }
- for i, edge := range check.mono.edges {
- src := &check.mono.vertices[edge.src]
- dst := &check.mono.vertices[edge.dst]
-
- // N.B., we're looking for the greatest weight paths, unlike
- // typical Bellman-Ford.
- w := src.weight + edge.weight
- if w <= dst.weight {
- continue
- }
-
- dst.pre = i
- dst.len = src.len + 1
- if dst.len == len(check.mono.vertices) {
- check.reportInstanceLoop(edge.dst)
- return
- }
-
- dst.weight = w
- again = true
+ scc := check.mono.sccs(out)
+ for i, edge := range check.mono.edges {
+ if edge.weight > 0 && scc[edge.src] == scc[edge.dst] {
+ check.reportInstanceLoop(out, i)
+ return
}
}
}
-func (check *Checker) reportInstanceLoop(v int) {
+// sccs partitions the graph's vertices into strongly connected
+// components and returns a slice that maps each vertex to a component
+// number. Two vertices are assigned the same component number if and
+// only if each is reachable from the other.
+//
+// It runs in time linear in the size of the graph, using the algorithm
+// from Sedgewick, Algorithms, Second Edition, p. 482: depth-first
+// search numbers vertices in preorder and keeps each visited vertex on
+// a stack until every vertex reachable from it has been visited. A
+// vertex that cannot reach any vertex numbered before it closes a
+// component, consisting of itself and the vertices above it on the
+// stack.
+func (w *monoGraph) sccs(out [][]int) []int {
+ scc := make([]int, len(w.vertices))
+ num := make([]int, len(w.vertices)) // preorder numbering; 0 means unvisited
var stack []int
- seen := make([]bool, len(check.mono.vertices))
+ nvisited, ncomponents := 0, 0
- // We have a path that contains a cycle and ends at v, but v may
- // only be reachable from the cycle, not on the cycle itself. We
- // start by walking backwards along the path until we find a vertex
- // that appears twice.
- for !seen[v] {
+ // done marks vertices whose component is complete. It compares
+ // larger than every preorder number, so a finished component never
+ // lowers the minimum of vertices still being visited.
+ done := len(w.vertices) + 1
+
+ var visit func(v int) int
+ visit = func(v int) int {
+ if num[v] != 0 {
+ return num[v]
+ }
+ nvisited++
+ num[v] = nvisited
stack = append(stack, v)
- seen[v] = true
- v = check.mono.edges[check.mono.vertices[v].pre].src
+
+ low := num[v] // lowest preorder number reachable from v
+ for _, e := range out[v] {
+ if n := visit(w.edges[e].dst); n < low {
+ low = n
+ }
+ }
+
+ if low == num[v] {
+ // v cannot reach any vertex visited before it, so it is the
+ // root of a component: v and the vertices above it on the
+ // stack.
+ ncomponents++
+ for {
+ x := stack[len(stack)-1]
+ stack = stack[:len(stack)-1]
+ num[x] = done
+ scc[x] = ncomponents
+ if x == v {
+ break
+ }
+ }
+ }
+ return low
+ }
+ for v := range w.vertices {
+ visit(v)
+ }
+ return scc
+}
+
+// reportInstanceLoop reports an instantiation cycle through edge loop,
+// whose endpoints must lie within a single strongly connected
+// component.
+func (check *Checker) reportInstanceLoop(out [][]int, loop int) {
+ edges := check.mono.edges
+ src, dst := edges[loop].src, edges[loop].dst
+
+ // Reconstruct a cycle: edge loop, followed by a path from dst back
+ // to src, found by breadth-first search so that we report a
+ // shortest cycle through the edge. The path exists because src and
+ // dst are strongly connected; pre records the edge along which the
+ // search first reached each vertex.
+ pre := make([]int, len(check.mono.vertices))
+ for i := range pre {
+ pre[i] = -1
+ }
+ pre[dst] = loop
+ for frontier := []int{dst}; pre[src] < 0; {
+ assert(len(frontier) > 0)
+ var next []int
+ for _, v := range frontier {
+ for _, e := range out[v] {
+ if w := edges[e].dst; pre[w] < 0 {
+ pre[w] = e
+ next = append(next, w)
+ }
+ }
+ }
+ frontier = next
}
- // Trim any vertices we visited before visiting v the first
- // time. Since v is the first vertex we found within the cycle, any
- // vertices we visited earlier cannot be part of the cycle.
- for stack[0] != v {
- stack = stack[1:]
+ // Walk the cycle backwards from dst, collecting its vertices. For
+ // every vertex v on the cycle, pre[v] is the cycle's edge into v.
+ var stack []int
+ for v := dst; ; {
+ stack = append(stack, v)
+ v = edges[pre[v]].src
+ if v == dst {
+ break
+ }
}
- // TODO(mdempsky): Pivot stack so we report the cycle from the top?
+ // Pivot the cycle so the error reads top-down in the source: report
+ // it starting from its earliest-declared vertex.
+ top := 0
+ for i := range stack {
+ if v := check.mono.vertices[stack[i]].obj; cmpPos(v.Pos(), check.mono.vertices[stack[top]].obj.Pos()) < 0 {
+ top = i
+ }
+ }
+ stack = append(stack[top:], stack[:top]...)
err := check.newError(InvalidInstanceCycle)
- obj0 := check.mono.vertices[v].obj
+ obj0 := check.mono.vertices[stack[0]].obj
err.addf(obj0, "instantiation cycle:")
qf := RelativeTo(check.pkg)
for _, v := range stack {
- edge := check.mono.edges[check.mono.vertices[v].pre]
+ edge := edges[pre[v]]
obj := check.mono.vertices[edge.dst].obj
switch obj.Type().(type) {
diff --git a/src/go/types/mono.go b/src/go/types/mono.go
index 65f7aa6..2804ec2 100644
--- a/src/go/types/mono.go
+++ b/src/go/types/mono.go
@@ -66,10 +66,6 @@
}
type monoVertex struct {
- weight int // weight of heaviest known path to this vertex
- pre int // previous edge (if any) in the above path
- len int // length of the above path
-
// obj is the defined type or type parameter represented by this
// vertex.
obj *TypeName
@@ -84,70 +80,149 @@
}
func (check *Checker) monomorph() {
- // We detect unbounded instantiation cycles using a variant of
- // Bellman-Ford's algorithm. Namely, instead of always running |V|
- // iterations, we run until we either reach a fixed point or we've
- // found a path of length |V|. This allows us to terminate earlier
- // when there are no cycles, which should be the common case.
+ // A positive-weight cycle exists if and only if some weight-1 edge
+ // starts and ends within a single strongly connected component: such
+ // an edge together with a path from its destination back to its
+ // source forms a cycle, of weight at least 1; and conversely, the
+ // vertices of any cycle are mutually reachable, while a cycle's
+ // weight can only be positive if the cycle contains a weight-1 edge.
- again := true
- for again {
- again = false
+ // out[v] lists the edges leaving vertex v.
+ out := make([][]int, len(check.mono.vertices))
+ for i, edge := range check.mono.edges {
+ out[edge.src] = append(out[edge.src], i)
+ }
- for i, edge := range check.mono.edges {
- src := &check.mono.vertices[edge.src]
- dst := &check.mono.vertices[edge.dst]
-
- // N.B., we're looking for the greatest weight paths, unlike
- // typical Bellman-Ford.
- w := src.weight + edge.weight
- if w <= dst.weight {
- continue
- }
-
- dst.pre = i
- dst.len = src.len + 1
- if dst.len == len(check.mono.vertices) {
- check.reportInstanceLoop(edge.dst)
- return
- }
-
- dst.weight = w
- again = true
+ scc := check.mono.sccs(out)
+ for i, edge := range check.mono.edges {
+ if edge.weight > 0 && scc[edge.src] == scc[edge.dst] {
+ check.reportInstanceLoop(out, i)
+ return
}
}
}
-func (check *Checker) reportInstanceLoop(v int) {
+// sccs partitions the graph's vertices into strongly connected
+// components and returns a slice that maps each vertex to a component
+// number. Two vertices are assigned the same component number if and
+// only if each is reachable from the other.
+//
+// It runs in time linear in the size of the graph, using the algorithm
+// from Sedgewick, Algorithms, Second Edition, p. 482: depth-first
+// search numbers vertices in preorder and keeps each visited vertex on
+// a stack until every vertex reachable from it has been visited. A
+// vertex that cannot reach any vertex numbered before it closes a
+// component, consisting of itself and the vertices above it on the
+// stack.
+func (w *monoGraph) sccs(out [][]int) []int {
+ scc := make([]int, len(w.vertices))
+ num := make([]int, len(w.vertices)) // preorder numbering; 0 means unvisited
var stack []int
- seen := make([]bool, len(check.mono.vertices))
+ nvisited, ncomponents := 0, 0
- // We have a path that contains a cycle and ends at v, but v may
- // only be reachable from the cycle, not on the cycle itself. We
- // start by walking backwards along the path until we find a vertex
- // that appears twice.
- for !seen[v] {
+ // done marks vertices whose component is complete. It compares
+ // larger than every preorder number, so a finished component never
+ // lowers the minimum of vertices still being visited.
+ done := len(w.vertices) + 1
+
+ var visit func(v int) int
+ visit = func(v int) int {
+ if num[v] != 0 {
+ return num[v]
+ }
+ nvisited++
+ num[v] = nvisited
stack = append(stack, v)
- seen[v] = true
- v = check.mono.edges[check.mono.vertices[v].pre].src
+
+ low := num[v] // lowest preorder number reachable from v
+ for _, e := range out[v] {
+ if n := visit(w.edges[e].dst); n < low {
+ low = n
+ }
+ }
+
+ if low == num[v] {
+ // v cannot reach any vertex visited before it, so it is the
+ // root of a component: v and the vertices above it on the
+ // stack.
+ ncomponents++
+ for {
+ x := stack[len(stack)-1]
+ stack = stack[:len(stack)-1]
+ num[x] = done
+ scc[x] = ncomponents
+ if x == v {
+ break
+ }
+ }
+ }
+ return low
+ }
+ for v := range w.vertices {
+ visit(v)
+ }
+ return scc
+}
+
+// reportInstanceLoop reports an instantiation cycle through edge loop,
+// whose endpoints must lie within a single strongly connected
+// component.
+func (check *Checker) reportInstanceLoop(out [][]int, loop int) {
+ edges := check.mono.edges
+ src, dst := edges[loop].src, edges[loop].dst
+
+ // Reconstruct a cycle: edge loop, followed by a path from dst back
+ // to src, found by breadth-first search so that we report a
+ // shortest cycle through the edge. The path exists because src and
+ // dst are strongly connected; pre records the edge along which the
+ // search first reached each vertex.
+ pre := make([]int, len(check.mono.vertices))
+ for i := range pre {
+ pre[i] = -1
+ }
+ pre[dst] = loop
+ for frontier := []int{dst}; pre[src] < 0; {
+ assert(len(frontier) > 0)
+ var next []int
+ for _, v := range frontier {
+ for _, e := range out[v] {
+ if w := edges[e].dst; pre[w] < 0 {
+ pre[w] = e
+ next = append(next, w)
+ }
+ }
+ }
+ frontier = next
}
- // Trim any vertices we visited before visiting v the first
- // time. Since v is the first vertex we found within the cycle, any
- // vertices we visited earlier cannot be part of the cycle.
- for stack[0] != v {
- stack = stack[1:]
+ // Walk the cycle backwards from dst, collecting its vertices. For
+ // every vertex v on the cycle, pre[v] is the cycle's edge into v.
+ var stack []int
+ for v := dst; ; {
+ stack = append(stack, v)
+ v = edges[pre[v]].src
+ if v == dst {
+ break
+ }
}
- // TODO(mdempsky): Pivot stack so we report the cycle from the top?
+ // Pivot the cycle so the error reads top-down in the source: report
+ // it starting from its earliest-declared vertex.
+ top := 0
+ for i := range stack {
+ if v := check.mono.vertices[stack[i]].obj; cmpPos(v.Pos(), check.mono.vertices[stack[top]].obj.Pos()) < 0 {
+ top = i
+ }
+ }
+ stack = append(stack[top:], stack[:top]...)
err := check.newError(InvalidInstanceCycle)
- obj0 := check.mono.vertices[v].obj
+ obj0 := check.mono.vertices[stack[0]].obj
err.addf(obj0, "instantiation cycle:")
qf := RelativeTo(check.pkg)
for _, v := range stack {
- edge := check.mono.edges[check.mono.vertices[v].pre]
+ edge := edges[pre[v]]
obj := check.mono.vertices[edge.dst].obj
switch obj.Type().(type) {
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Friendly ping - happy to keep iterating on this.
Worth surfacing from the issue, since it's the bar I think this should be
judged against: Keith Randall noted there that he'd be fine with the faster
algorithm "if we can make [it] no worse than the current one" in ease of
understanding and verification. This CL's message leads with the speedup
instead, which is the part he'd already discounted, so:
- Correctness reduces to a single invariant: a positive-weight cycle exists
iff some weight-1 edge has both endpoints in one strongly connected
component. Each direction is a sentence, and both are spelled out in the
commit message. Verifying the Bellman-Ford version instead means reasoning
about why V relaxation rounds suffice.
- The SCC routine is not new to the tree. It is the same algorithm
cmd/compile/internal/ir already uses to group mutually recursive functions,
from the same Sedgewick reference.
- Being honest about the cost: the line count does go up. Most of that is the
doc comment, but the real addition is explicit cycle reconstruction for the
diagnostic. Bellman-Ford gave that away for free through predecessors; SCC
tells you an edge lies on a cycle but you have to walk it. If that is the
part that reads as harder to verify, I am happy to simplify it, or to drop
the "pivot to the earliest-declared vertex" TODO fix, which is optional and
independent of the complexity change.
Diagnostics are unchanged on all existing tests and on multi-vertex cycles
checked by hand.
Also happy to split this into types2 first with the go/types mirror as a
follow-up, if that is easier to review.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adding Keith, since the bar here is his.
The Aug 2 comment answers your point on the issue, cost included. Happy to drop the earliest-vertex pivot, or split types2 from go/types, if that makes it easier to review.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thanks.
Splitting out go/types/mono.go into a separate CL won't make a difference here; we always try to review changes in both checkers simulataneously as they are usually almost identical.
As this is non-trivial, one of us will need to dedicate some time to review this. This is not urgent in any way, I believe.
Finally, there should be benchmarks showing that this is not slower in the common case and clearly faster in the cases where we're currently slow.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benchmarks are in patch set 2, mono_test.go in both trees, over three shapes of instantiation graph. Ten interleaved pairs of old and new test binaries, benchstat.
For the common case I went and measured what it actually is. Across the 704 packages under GOROOT/src, 669 build no instantiation graph at all, the largest is slices at 72 vertices and 101 edges, only 3 packages have any positive weight edge, and none needs more than two Bellman-Ford passes. So "wide" at n=100 is already a bigger graph than anything in the tree, and it doesn't move:
wide/100 843.8µ ± 4% 842.3µ ± 6% ~ (p=0.912 n=10)
wide/1000 10.59m ± 18% 10.39m ± 9% ~ (p=0.631 n=10)
wide/10000 136.2m ± 9% 136.0m ± 10% ~ (p=1.000 n=10)
Where it's slow, at the issue's 100,000 declarations:
wide 0.88s -> 0.86s
chain 26.3s -> 1.59s
cycle 36.9s -> 0.57s
The chain one is worth a look. It's the aside in the issue: one deep instantiation chain, no cycle, and it compiles clean. Bellman-Ford advances one link per pass, so that's 26 seconds on a valid program. The quadratic isn't only reachable through the error.
One thing against it. Building the adjacency list allocates once per vertex, so allocs/op is up 1.0% to 1.75% everywhere. Bytes allocated don't change and neither does time, and at std's sizes it's about 70 allocations, so I kept the simpler slice of slices instead of a flat adjacency. Happy to switch if you'd rather.
And to be straight about what this buys: no real package gets faster. std never needs more than two passes. It removes a worst case, that's all.
All tests pass in both trees.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patch set 3 takes that allocation back out. Two changes.
No positive weight edge means no cycle, so the check returns after one scan and allocates nothing. That's 701 of the 704 packages under GOROOT/src.
For the rest, the adjacency is two flat slices, head and link, instead of a slice per vertex. Built back to front so each list stays in edge order, which is why the reported cycle can't change.
allocs/op geomean went from +1.41% to +0.02%, and it's now a constant 3 to 5 per package whether the graph has 100 vertices or 10,000. Bytes allocated are down at every size. Timings unchanged, chain/10000 and cycle/10000 still 56% and 75% faster.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |