perf(scc): traverse iteratively and look each successor up once - #799
Merged
Merged
Conversation
`recurse_onto` descends one stack frame per node, so a graph only has to be deep to bring the process down. A chain of 200 000 nodes overflows an 8 MiB stack, and a random graph of 60 000 nodes with average degree 4 is already enough. Walk an explicit stack instead. While the traversal is being rewritten, three things it repeated go away: - `scca` and `preorders` answered two halves of the same question about a successor, costing two hash lookups per edge. A node whose component has been emitted is now marked in `preorders` itself, leaving one. - `p` held cloned nodes and looked their preorder numbers back up on every pop. It now holds the preorder numbers directly, so the loop that unwinds it does no hashing and no cloning at all. - `strongly_connected_components` picked its next unvisited node with `preorders.keys().find(..)` over a map it was emptying as it went, rescanning the vacated buckets each time. It now walks the caller's slice, which also makes the order of the returned components deterministic rather than dependent on hash iteration order. Nodes are still assigned to exactly one component each, which is checked against mutual reachability over 100 random graphs. About 64% off a 60 000 node graph of degree 4.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #795, which covers the same defect in
topological_sortas well; that half is #800, and the issue can be closed once both have landed.recurse_ontodescended one stack frame per node, so the recursion depth was the depth of the graph. A 200 000 node chain overflows an 8 MiB stack, and a random graph of 60 000 nodes at average degree 4 is enough on its own. This replaces the recursion with an explicit stack.While the traversal was being rewritten, three things it repeated go away:
sccaandpreordersanswered two halves of the same question about each successor, costing two hash lookups per edge. A node whose component has been emitted is now marked inpreordersitself, leaving one.pheld cloned nodes and looked their preorder numbers back up on every pop. It now holds the preorder numbers directly, so unwinding it does no hashing and no cloning.strongly_connected_componentspicked its next unvisited node withpreorders.keys().find(..)over a map it was emptying as it went, rescanning the vacated buckets each time. It now walks the caller's slice.Worth reviewing
The order of the returned components now follows the order of the
nodesargument rather than hash iteration order. It was never specified, but this is a visible change for anyone who happened to depend on it — it is at least deterministic now.Checking
Correctness is checked against mutual reachability, for every pair of nodes, over 100 random graphs. A regression test runs a 200 000 node chain on a deliberately small 1 MiB stack, so a return to a recursive traversal fails loudly rather than silently.
About 64% off a 60 000 node graph of degree 4, measured against this branch's parent.
All timings measured on Windows 11, Intel Core Ultra 7 265K, 64 GB RAM.