From 8bbafc0b622a9f8232015436b04907b7c6c262e9 Mon Sep 17 00:00:00 2001 From: jacob Date: Tue, 13 Oct 2026 10:53:00 -0400 Subject: [PATCH 1/3] resource: benchmark sub-graphs and reachability on a robot-sized graph A graph shaped like a robot with one remote: the remote and 50 resources on it, plus 60 local components and services. MarkReachability runs on every remote refresh pass, and SubGraphFrom whenever a resource's dependents are marked for update. Co-Authored-By: Claude Opus 5.5 --- resource/resource_graph_bench_test.go | 82 +++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 resource/resource_graph_bench_test.go diff --git a/resource/resource_graph_bench_test.go b/resource/resource_graph_bench_test.go new file mode 100644 index 00000000000..dc6c9c88f80 --- /dev/null +++ b/resource/resource_graph_bench_test.go @@ -0,0 +1,82 @@ +package resource + +import ( + "fmt" + "testing" + + "go.viam.com/test" + + "go.viam.com/rdk/logging" +) + +// robotSizedGraph builds a graph shaped like a robot with one remote: the +// remote and 50 resources on it, plus 60 local resources in three layers +// (components, services on components and remote resources, and services on +// services). It returns the graph, the remote's name and a component with a +// dozen dependents. +func robotSizedGraph(tb testing.TB) (*Graph, Name, Name) { + tb.Helper() + g := NewGraph(logging.NewTestLogger(tb)) + add := func(name Name, deps ...Name) { + test.That(tb, g.AddNode(name, &GraphNode{}), test.ShouldBeNil) + for _, dep := range deps { + test.That(tb, g.AddChild(name, dep), test.ShouldBeNil) + } + } + + remote := NewName(APINamespace("rdk").WithType("component").WithSubtype("remote"), "remote1") + add(remote) + remoteRes := make([]Name, 50) + for i := range remoteRes { + remoteRes[i] = NewName(apiA, fmt.Sprintf("r%d", i)).PrependRemote(remote.Name) + add(remoteRes[i], remote) + } + + components := make([]Name, 20) + for i := range components { + components[i] = NewName(apiA, fmt.Sprintf("c%d", i)) + if i < 5 { + add(components[i]) + } else { + add(components[i], components[i%5]) + } + } + services := make([]Name, 25) + for i := range services { + services[i] = NewName(apiA, fmt.Sprintf("s%d", i)) + deps := []Name{components[i%20], components[(i*7+1)%20], components[(i*13+2)%20]} + if i%2 == 0 { + deps = append(deps, remoteRes[i%50], remoteRes[(i*3+1)%50]) + } + add(services[i], deps...) + } + for i := 0; i < 15; i++ { + add(NewName(apiA, fmt.Sprintf("h%d", i)), services[i%25], services[(i*7+3)%25], components[(i*11)%20]) + } + return g, remote, components[3] +} + +// The remote refresh marks the remote and everything depending on it +// reachable on every pass. +func BenchmarkGraphMarkReachability(b *testing.B) { + g, remote, _ := robotSizedGraph(b) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if err := g.MarkReachability(remote, true); err != nil { + b.Fatal(err) + } + } +} + +// Marking a resource's dependents for update starts from its sub-graph. +func BenchmarkGraphSubGraphFrom(b *testing.B) { + g, _, component := robotSizedGraph(b) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := g.SubGraphFrom(component); err != nil { + b.Fatal(err) + } + } +} From c3c03a7e50ba2ff9a844cf24d3ac1745d66395c4 Mon Sep 17 00:00:00 2001 From: jacob Date: Tue, 13 Oct 2026 10:57:18 -0400 Subject: [PATCH 2/3] resource: build sub-graphs from a node's dependents instead of pruning a clone SubGraphFrom cloned the whole graph, sorted it topologically, and removed every node that doesn't depend on the target. Each removal updates the transitive closure across the whole graph, so one call costs about V^2 per removed edge. MarkReachability went through the same path, and viam-server's remote refresh calls it on every pass, as does markChildrenForUpdate when a resource is rebuilt. On a Raspberry Pi CM5 with one remote this was about 6% of a core at idle. The target's dependents can be read straight from the transitive closure: the nodes n with closure[n][target] != 0, the same test the pruning loop used. The sub-graph is now built from those nodes: the same *GraphNode values, the edges among them, and the closure restricted to them. Every path between two dependents runs through dependents, so the closure among them is unchanged. MarkReachability marks the dependents directly, without building a sub-graph. A new test compares the result with the pruning approach (nodes, node pointers, children, parents, closure) on the existing test graph and on the robot-sized benchmark graph, and another checks MarkReachability marks exactly the node and its dependents. Co-Authored-By: Claude Opus 5.5 --- resource/resource_graph.go | 67 +++++++++++++++++++++---- resource/resource_graph_test.go | 89 +++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 10 deletions(-) diff --git a/resource/resource_graph.go b/resource/resource_graph.go index a2380e8b776..9fa4510fb99 100644 --- a/resource/resource_graph.go +++ b/resource/resource_graph.go @@ -1087,19 +1087,66 @@ func (g *Graph) SubGraphFrom(node Name) (*Graph, error) { return g.subGraphFromWithMutex(node) } +// dependentsWithMutex returns node and every node that depends on it, directly or +// transitively, read from the transitive closure. This method is NOT threadsafe: A client +// must read-lock [Graph.mu] while calling this method. +func (g *Graph) dependentsWithMutex(node Name) (map[Name]*GraphNode, error) { + if _, ok := g.nodes.Get(node); !ok { + return nil, errors.Errorf("cannot create sub-graph from non existing node %v ", node.Name) + } + dependents := map[Name]*GraphNode{} + for name, gNode := range g.nodes.All() { + if g.transitiveClosureMatrix[name][node] != 0 { + dependents[name] = gNode + } + } + return dependents, nil +} + // subGraphFrom returns a Sub-Graph containing all linked dependencies starting with node // [Name]. This method is NOT threadsafe: A client must read-lock [Graph.mu] while calling // this method. +// +// The sub-graph holds node and its dependents, the same *GraphNode values as g, and the +// edges and transitive closure among them. Every path between two of those nodes stays +// among them, so the closure is the same as g's. It is built from just those nodes rather +// than by removing every other node from a clone of g, which costs a transitive closure +// update per removed edge. func (g *Graph) subGraphFromWithMutex(node Name) (*Graph, error) { - if _, ok := g.nodes.Get(node); !ok { - return nil, errors.Errorf("cannot create sub-graph from non existing node %v ", node.Name) + dependents, err := g.dependentsWithMutex(node) + if err != nil { + return nil, err + } + subGraph := &Graph{ + children: resourceDependencies{}, + nodes: g.nodes.Copy(), + parents: resourceDependencies{}, + transitiveClosureMatrix: make(transitiveClosureMatrix, len(dependents)), + logicalClock: g.logicalClock, + } + for name := range g.nodes.Keys() { + if _, ok := dependents[name]; !ok { + subGraph.nodes.Delete(name) + } } - subGraph := g.clone() - sorted := subGraph.ReverseTopologicalSort() - for _, n := range sorted { - if !subGraph.isNodeDependingOn(node, n) { - subGraph.remove(n) + for name := range dependents { + // A dependent's children depend on it, so they are all dependents too. + if children, ok := g.children[name]; ok { + subGraph.children[name] = copyNodes(children) + } + for parent, edge := range g.parents[name] { + if _, ok := dependents[parent]; ok { + if subGraph.parents[name] == nil { + subGraph.parents[name] = graphNodes{} + } + subGraph.parents[name][parent] = edge + } + } + row := make(map[Name]int, len(dependents)) + for other := range dependents { + row[other] = g.transitiveClosureMatrix[name][other] } + subGraph.transitiveClosureMatrix[name] = row } return subGraph, nil } @@ -1109,12 +1156,12 @@ func (g *Graph) MarkReachability(node Name, reachable bool) error { g.mu.RLock() defer g.mu.RUnlock() - subGraph, err := g.subGraphFromWithMutex(node) + dependents, err := g.dependentsWithMutex(node) if err != nil { return err } - for node := range subGraph.nodes.Values() { - node.markReachability(reachable) + for _, gNode := range dependents { + gNode.markReachability(reachable) } return nil } diff --git a/resource/resource_graph_test.go b/resource/resource_graph_test.go index 30ef09cbfae..963641deaed 100644 --- a/resource/resource_graph_test.go +++ b/resource/resource_graph_test.go @@ -267,6 +267,95 @@ func TestResourceGraphSubGraph(t *testing.T) { }...)) } +// subGraphByPruning is how SubGraphFrom used to build a sub-graph: clone the graph, then +// remove every node that doesn't depend on node. +func subGraphByPruning(g *Graph, node Name) *Graph { + sub := g.clone() + for _, n := range sub.ReverseTopologicalSort() { + if !sub.isNodeDependingOn(node, n) { + sub.remove(n) + } + } + return sub +} + +func edgeSets(rd resourceDependencies) map[Name]map[Name]bool { + out := map[Name]map[Name]bool{} + for k, nodes := range rd { + out[k] = map[Name]bool{} + for n := range nodes { + out[k][n] = true + } + } + return out +} + +// The sub-graph must hold the same nodes (the same *GraphNode values), edges and +// transitive closure as pruning a clone of the graph. +func TestResourceGraphSubGraphMatchesPruning(t *testing.T) { + logger := logging.NewTestLogger(t) + small := NewGraph(logger) + for _, component := range commonCfg { + test.That(t, small.AddNode(component.Name, &GraphNode{}), test.ShouldBeNil) + for _, dep := range component.DependsOn { + test.That(t, small.AddChild(component.Name, dep), test.ShouldBeNil) + } + } + robot, remote, component := robotSizedGraph(t) + + check := func(g *Graph, node Name) { + t.Helper() + got, err := g.SubGraphFrom(node) + test.That(t, err, test.ShouldBeNil) + want := subGraphByPruning(g, node) + + test.That(t, len(got.nodes.nodes), test.ShouldEqual, len(want.nodes.nodes)) + for name, wantNode := range want.nodes.All() { + gotNode, ok := got.nodes.Get(name) + test.That(t, ok, test.ShouldBeTrue) + test.That(t, gotNode == wantNode, test.ShouldBeTrue) + } + test.That(t, edgeSets(got.children), test.ShouldResemble, edgeSets(want.children)) + test.That(t, edgeSets(got.parents), test.ShouldResemble, edgeSets(want.parents)) + for u := range want.nodes.Keys() { + for v := range want.nodes.Keys() { + test.That(t, got.transitiveClosureMatrix[u][v], test.ShouldEqual, want.transitiveClosureMatrix[u][v]) + } + } + test.That(t, got.TopologicalSort(), test.ShouldHaveLength, len(want.nodes.nodes)) + } + + for name := range small.nodes.Keys() { + check(small, name) + } + // Pruning the robot-sized graph takes about half a second per node, so check a spread: + // the remote, a remote resource, components with and without dependencies, services. + for _, name := range []Name{ + remote, + NewName(apiA, "r7").PrependRemote(remote.Name), + component, + NewName(apiA, "c0"), + NewName(apiA, "c12"), + NewName(apiA, "s4"), + NewName(apiA, "h2"), + } { + check(robot, name) + } +} + +func TestResourceGraphMarkReachability(t *testing.T) { + g, remote, _ := robotSizedGraph(t) + test.That(t, g.MarkReachability(remote, false), test.ShouldBeNil) + for name, gNode := range g.nodes.All() { + test.That(t, gNode.IsReachable(), test.ShouldEqual, !g.isNodeDependingOn(remote, name)) + } + test.That(t, g.MarkReachability(remote, true), test.ShouldBeNil) + for gNode := range g.nodes.Values() { + test.That(t, gNode.IsReachable(), test.ShouldBeTrue) + } + test.That(t, g.MarkReachability(NewName(apiA, "missing"), false), test.ShouldNotBeNil) +} + func TestResourceGraphDepTree(t *testing.T) { cfg := []fakeComponent{ { From 749c5423b86c189ce501eb0b733934507d2ff1ff Mon Sep 17 00:00:00 2001 From: jacob Date: Mon, 19 Oct 2026 14:20:40 -0400 Subject: [PATCH 3/3] resource: hard-code sub-graph and reachability test expectations Drop the comparison against the old pruning approach. TestResourceGraphSubGraph now asserts the sub-graph's edges and transitive closure directly, and TestResourceGraphMarkReachability asserts the exact set of nodes it marks, so a failure points at the code under test rather than at one of two ways of building a sub-graph. Co-Authored-By: Claude Opus 5.5 --- resource/resource_graph_test.go | 112 ++++++++++---------------------- 1 file changed, 35 insertions(+), 77 deletions(-) diff --git a/resource/resource_graph_test.go b/resource/resource_graph_test.go index 963641deaed..e476a4c1119 100644 --- a/resource/resource_graph_test.go +++ b/resource/resource_graph_test.go @@ -265,95 +265,53 @@ func TestResourceGraphSubGraph(t *testing.T) { NewName(apiA, "F"), NewName(apiA, "C"), }...)) -} - -// subGraphByPruning is how SubGraphFrom used to build a sub-graph: clone the graph, then -// remove every node that doesn't depend on node. -func subGraphByPruning(g *Graph, node Name) *Graph { - sub := g.clone() - for _, n := range sub.ReverseTopologicalSort() { - if !sub.isNodeDependingOn(node, n) { - sub.remove(n) - } - } - return sub -} -func edgeSets(rd resourceDependencies) map[Name]map[Name]bool { - out := map[Name]map[Name]bool{} - for k, nodes := range rd { - out[k] = map[Name]bool{} - for n := range nodes { - out[k][n] = true - } - } - return out + // D and F also depend on B and A, which are not in the sub-graph, so those edges are + // left out of it. + c, d, f := NewName(apiA, "C"), NewName(apiA, "D"), NewName(apiA, "F") + test.That(t, newResourceNameSet(sg.GetAllChildrenOf(c)...), test.ShouldResemble, newResourceNameSet(d, f)) + test.That(t, sg.GetAllChildrenOf(d), test.ShouldBeEmpty) + test.That(t, sg.GetAllChildrenOf(f), test.ShouldBeEmpty) + test.That(t, sg.GetAllParentsOf(c), test.ShouldBeEmpty) + test.That(t, sg.GetAllParentsOf(d), test.ShouldResemble, []Name{c}) + test.That(t, sg.GetAllParentsOf(f), test.ShouldResemble, []Name{c}) + test.That(t, sg.transitiveClosureMatrix, test.ShouldResemble, transitiveClosureMatrix{ + c: {c: 1, d: 0, f: 0}, + d: {c: 1, d: 1, f: 0}, + f: {c: 1, d: 0, f: 1}, + }) } -// The sub-graph must hold the same nodes (the same *GraphNode values), edges and -// transitive closure as pruning a clone of the graph. -func TestResourceGraphSubGraphMatchesPruning(t *testing.T) { +func TestResourceGraphMarkReachability(t *testing.T) { logger := logging.NewTestLogger(t) - small := NewGraph(logger) + g := NewGraph(logger) for _, component := range commonCfg { - test.That(t, small.AddNode(component.Name, &GraphNode{}), test.ShouldBeNil) + test.That(t, g.AddNode(component.Name, &GraphNode{}), test.ShouldBeNil) for _, dep := range component.DependsOn { - test.That(t, small.AddChild(component.Name, dep), test.ShouldBeNil) + test.That(t, g.AddChild(component.Name, dep), test.ShouldBeNil) } } - robot, remote, component := robotSizedGraph(t) - - check := func(g *Graph, node Name) { - t.Helper() - got, err := g.SubGraphFrom(node) - test.That(t, err, test.ShouldBeNil) - want := subGraphByPruning(g, node) - - test.That(t, len(got.nodes.nodes), test.ShouldEqual, len(want.nodes.nodes)) - for name, wantNode := range want.nodes.All() { - gotNode, ok := got.nodes.Get(name) - test.That(t, ok, test.ShouldBeTrue) - test.That(t, gotNode == wantNode, test.ShouldBeTrue) - } - test.That(t, edgeSets(got.children), test.ShouldResemble, edgeSets(want.children)) - test.That(t, edgeSets(got.parents), test.ShouldResemble, edgeSets(want.parents)) - for u := range want.nodes.Keys() { - for v := range want.nodes.Keys() { - test.That(t, got.transitiveClosureMatrix[u][v], test.ShouldEqual, want.transitiveClosureMatrix[u][v]) + unreachable := func() map[Name]*GraphNode { + var names []Name + for name, gNode := range g.nodes.All() { + if !gNode.IsReachable() { + names = append(names, name) } } - test.That(t, got.TopologicalSort(), test.ShouldHaveLength, len(want.nodes.nodes)) - } - - for name := range small.nodes.Keys() { - check(small, name) - } - // Pruning the robot-sized graph takes about half a second per node, so check a spread: - // the remote, a remote resource, components with and without dependencies, services. - for _, name := range []Name{ - remote, - NewName(apiA, "r7").PrependRemote(remote.Name), - component, - NewName(apiA, "c0"), - NewName(apiA, "c12"), - NewName(apiA, "s4"), - NewName(apiA, "h2"), - } { - check(robot, name) + return newResourceNameSet(names...) } -} -func TestResourceGraphMarkReachability(t *testing.T) { - g, remote, _ := robotSizedGraph(t) - test.That(t, g.MarkReachability(remote, false), test.ShouldBeNil) - for name, gNode := range g.nodes.All() { - test.That(t, gNode.IsReachable(), test.ShouldEqual, !g.isNodeDependingOn(remote, name)) - } - test.That(t, g.MarkReachability(remote, true), test.ShouldBeNil) - for gNode := range g.nodes.Values() { - test.That(t, gNode.IsReachable(), test.ShouldBeTrue) - } - test.That(t, g.MarkReachability(NewName(apiA, "missing"), false), test.ShouldNotBeNil) + test.That(t, g.MarkReachability(NewName(apiA, "B"), false), test.ShouldBeNil) + test.That(t, unreachable(), test.ShouldResemble, newResourceNameSet([]Name{ + NewName(apiA, "B"), + NewName(apiA, "C"), + NewName(apiA, "D"), + NewName(apiA, "E"), + NewName(apiA, "F"), + }...)) + test.That(t, g.MarkReachability(NewName(apiA, "B"), true), test.ShouldBeNil) + test.That(t, unreachable(), test.ShouldBeEmpty) + test.That(t, g.MarkReachability(NewName(apiA, "Z"), false), test.ShouldNotBeNil) } func TestResourceGraphDepTree(t *testing.T) {