Skip to content

perf(topological_sort): traverse iteratively and drop the redundant sets - #800

Merged
samueltardieu merged 1 commit into
evenfurther:mainfrom
tachsin:fix/topological-sort-stack-overflow
Sep 9, 2026
Merged

perf(topological_sort): traverse iteratively and drop the redundant sets#800
samueltardieu merged 1 commit into
evenfurther:mainfrom
tachsin:fix/topological-sort-stack-overflow

Conversation

@tachsin

@tachsin tachsin commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Part of #795, which covers the same defect in strongly_connected_components as well; that half is #799, and the issue can be closed once both have landed.

visit recursed once per node, so a deep graph overflowed the stack: a chain of 200 000 nodes is enough, and so is a random DAG of 60 000. This replaces the recursion with an explicit stack.

The bookkeeping around it was doing the same work more than once:

  • marked and temp are nested — a node is in temp from the moment it is reached and in marked once it is finished, so marked is a subset of temp and every node was hashed into both. One map from node to "finished?" answers both questions in a single lookup.
  • The outer loop repeatedly took an arbitrary key out of a HashSet of roots that visit was emptying, so each call rescanned the buckets left behind by the ones before it. Walking the caller's slice needs no set at all, and with it goes a clone of every root.
  • Both sets used the default hasher, unlike the rest of the crate.

topological_sort_into_groups keeps its structure and only changes hasher.

Checking

successors is still called exactly once per node, which the existing complexity test covers. Orders are checked edge by edge against 200 random DAGs, and a regression test runs a 200 000 node chain on a deliberately small 1 MiB stack.

On 60 000 nodes: about 76% off topological_sort, about 33% off topological_sort_into_groups.

All timings measured on Windows 11, Intel Core Ultra 7 265K, 64 GB RAM.

`visit` recurses once per node, so a deep graph overflows the stack: a chain of
200 000 nodes is enough, and so is a random DAG of 60 000. Walk an explicit
stack instead.

The bookkeeping around it was doing the same work several times over:

- `marked` and `temp` are nested: a node is in `temp` from the moment it is
  reached and in `marked` once it is finished, so `marked` is a subset of
  `temp` and every node was hashed into both. One map from node to "finished?"
  answers both questions in a single lookup.
- The outer loop repeatedly took an arbitrary key out of a `HashSet` of roots
  that `visit` was emptying as it went, so each call rescanned the buckets left
  behind by the ones before it. Walking the caller's slice needs no set at all,
  and with it goes a clone of every root.
- Both sets used the default hasher, unlike the rest of the crate.

`topological_sort_into_groups` keeps its structure and just moves to the same
hasher.

`successors` is still called exactly once per node, which the existing
`complexity` test checks. Orders are checked for validity against 200 random
DAGs.

On 60 000 nodes: about 76% off `topological_sort`, and about 33% off
`topological_sort_into_groups`.
@samueltardieu
samueltardieu added this pull request to the merge queue Sep 9, 2026
Merged via the queue into evenfurther:main with commit b6e418f Sep 9, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants