[Jacob Kaplan (qwertey6)]

Problem

Graph.SubGraphFrom clones the whole graph, sorts it topologically, then removes every node that doesn't depend on the target. Each removal updates the transitive closure over the whole graph, so one call costs about V² per removed edge.

Two hot paths go through it:

• MarkReachability, which viam-server's remote refresh (updateRemoteResourceNames) calls on every pass, even when the remote is already reachable.
• markChildrenForUpdate, whenever a resource is rebuilt.

On a Raspberry Pi CM5 running v1.11.3:

• With one remote, updateRemoteResourceNames → MarkReachability → subGraphFromWithMutex was 5.8% of a core, 16% of viam-server's CPU (pprof).
• On a machine without remotes where one resource failed to build every 5 s, markChildrenForUpdate → SubGraphFrom was about half of viam-server's idle CPU.

Change

• The dependents come straight from the transitive closure. They're the nodes n with closure[n][target] != 0, the same test the pruning loop used.
• The sub-graph is built from just 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 reads the dependents directly, without building a sub-graph, and marks the node and the remote resources among them (the behavior from APP-18274 - Report resource reachability in machine status #6653).

Benchmark

The first commit adds BenchmarkGraphMarkReachability and BenchmarkGraphSubGraphFrom on a robot-sized graph: a remote with 50 resources plus 60 local components and services, ~110 nodes. Measured on a CM5, one core (taskset), 3 runs each, which agree within 1%. These runs predate merging #6653, whose filter adds one name check per dependent to MarkReachability:

	Before	After
MarkReachability	1,343 ms/op, 3.27 MB, 2,220 allocs	71 µs/op, 24 KB, 11 allocs
SubGraphFrom	1,580 ms/op, 3.27 MB, 2,222 allocs	0.88 ms/op, 336 KB, 551 allocs

Tests

• TestResourceGraphSubGraph now also asserts the sub-graph's edges and transitive closure. The sub-graph from C keeps only the edges among C, D and F, not D's edge to B or F's edge to A. Before this, every test in resource/ passed with those outside edges left in.
• TestStatusReachability (from APP-18274 - Report resource reachability in machine status #6653) asserts exactly which nodes MarkReachability marks.
• Each fails on a deliberately broken version: parents not filtered, children dropped, or closure rows not restricted for SubGraphFrom. For MarkReachability: every node marked, every dependent marked, only the node marked, or the node itself skipped.
• Runs:
  • go test ./resource/... passes.
  • 35 robot/impl tests covering remotes, offline remotes, reconfiguration, dependents and optional dependencies pass, including TestReconfigure, TestRemoteRobotsUpdate, TestOfflineRemoteResources, TestRemovingOfflineRemotes, TestDependentAndOrphanedResources and TestModularOptionalDependencyOnRemote.
  • After merging main, those six and the robot/impl tests APP-18274 - Report resource reachability in machine status #6653 changed (TestMachineStatus*, TestRemoteRobotsGold, TestInferRemoteRobotDependency*) pass: 16 tests.
  • golangci-lint v2.13.2 reports no issues, with no format diff.

🤖 Generated with Claude Code

[CLAassistant]

All committers have signed the CLA.

[viam-overwatch]

Hey Jacob Kaplan (@qwertey6) — CI is green and no reviewer is assigned yet. Could you request one when you have a chance?

Auto-comment from overwatch. Will not re-nudge for 7 days.

[Jacob Kaplan (qwertey6)]

Dan Gottlieb (@dgottlieb) could you review this when you have a chance? I can't request reviewers from a fork. You reviewed #5959's graph locking, and this is the next step for the same code path: MarkReachability and SubGraphFrom build the sub-graph from a node's dependents instead of cloning the whole graph and pruning it. On a CM5 with a ~110-node graph that's 1.34 s → 71 µs per MarkReachability call. That call runs on every remote refresh, and it was most of the 5.8 points of a core that viam-server spent on the resource graph in our kiosk profiles.

[Dan Gottlieb (dgottlieb)]

I can see the utility of comparing to the pruning result to prove equivalence. Are there tests here that are actually exercising more cases than existing tests? If not, can just delete this.

But if there are cases worth covering, we should remove the subGraphByPruning and instead hard code the answers/assertions. That way a bug introduced in, say ReverseTopologicalSort has a predictable diagnosis rather than leaving us guess which way of generating a subgraph has a bug.

[Jacob Kaplan (qwertey6)]

Yes. They checked the sub-graph's edges and closure, which no existing test did. With the outside parents left in (D's edge to B, F's edge to A), every test in resource/ still passed. Node identity was already covered by TestResourceGraphMarkForRemoval.

So I dropped subGraphByPruning and the sweep in 749c542. TestResourceGraphSubGraph now hard-codes the sub-graph from C: children, parents (only C), and the closure among C, D and F. TestResourceGraphMarkReachability had the same problem, because it computed its expected marks from the closure. It now asserts the exact set: marking B marks B, C, D, E and F. Each assertion fails on a deliberately broken version: parents unfiltered, children dropped, closure rows not restricted, or every node marked.

[Dan Gottlieb (dgottlieb)]

What is edgeSets doing here? Getting subgraphs clones (ultimately) a map of keys, but the terminal values are the same pointer values. Do these assertions really fail without edgeSets?

Assuming we need to actually keep some part of this test that is.

[Jacob Kaplan (qwertey6)]

No, they pass without it. Both graphs hold the same &GraphNode{} edge values, so ShouldResemble on children and parents directly passes. It's gone along with the rest of that test.

[Dan Gottlieb (dgottlieb)]

I really appreciate these. Very compelling results!

[github-actions]

For security reasons, this PR must be labeled with safe to test in order for tests to run.

[Jacob Kaplan (qwertey6)]

Dan Gottlieb (@dgottlieb) thanks! A change after your approval: #6653 landed and conflicted on MarkReachability, so I merged main in ad89503. MarkReachability now reads the dependents from the closure and keeps #6653's filter: the node plus the remote resources among them, with local dependents left alone. I dropped TestResourceGraphMarkReachability, which asserted the old mark-everything behavior. #6653's TestStatusReachability now asserts exactly which nodes get marked, and it fails on every breakage I tried against this version.

The push removed the safe to test label. Could you add it back so CI runs?

[Dan Gottlieb (dgottlieb)]

CI ran -- everything still looks good. Didn't look too closely at the post-merge conflict code. Push back if I ought to be more discerning.

[Jacob Kaplan (qwertey6)]

I don't think it needs a close look. The resolution is #6653's filter, applied to the dependents map instead of the sub-graph's nodes:

for name, gNode := range dependents {
	if name == node || name.Remote != "" {
		gNode.markReachability(reachable)
	}
}

The rest of the merge is main's code, unchanged. TestStatusReachability covers this loop: it fails if the filter is dropped, if every node is marked, or if the node itself isn't marked.