Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified Tarjan SCC to reduce memory usage. #413

Merged
merged 8 commits into from
Apr 26, 2021
Merged

Conversation

saolof
Copy link
Contributor

@saolof saolof commented Apr 16, 2021

Reduced heap memory usage, improved cache locality, and made the code somewhat cleaner for a possible future rewrite to an imperative implementation. The changes to a more modern version of Tarjan's algorithm were directly based on algorithm 3 in "A Space-Efficient Algorithm for Finding Strongly Connected Components" by David J. Pearce, while also aiming to avoid changing the structure of the code too much.

In addition to saving memory, this version has the property that it builds a lookup
table for the components at the same time as it outputs them. If v belongs to the kth output component, then
self.nodes[g.to_index(v)] == Some(k) . This could be used to save work in
functions that may want to call it such as condensation (where the work needed to make the node map can be completely avoided, while also creating a condensation that is topologically sorted).

Reduced heap memory usage to a single vec allocation, improved cache locality,
and made the code cleaner for a possible future rewrite to a more
optimized imperative version.

In addition, the new version has the property that it builds a lookup
table for the components. If v belongs to the kth output component, then
self.nodes[g.to_index(v)] == Some(k) . This can be used to save work in
functions that may want to call it such as condensation.
@saolof
Copy link
Contributor Author

saolof commented Apr 24, 2021

Another quick comment: because of the way it builds up a component lookup table, this version would also enable a sort_scc_stable function (returning a list of SCCs where the nodes appear in the order given by the original graph or some iterator over its nodes), which would be very useful in some situations.

Basically, since a node will be assigned to its proper SCC when visit returns from the top level loop, that means you can bucket sort nodes by their rindex value in the top level loop instead of when you pop them from the stack.

Copy link
Member

@ABorgna ABorgna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice !

In addition to the comments, it would be nice to have some benchmarks to compare the changes.

src/algo/mod.rs Show resolved Hide resolved
src/algo/mod.rs Outdated Show resolved Hide resolved
src/algo/mod.rs Outdated Show resolved Hide resolved
@ABorgna
Copy link
Member

ABorgna commented Apr 26, 2021

I ran #421's benchmarks, and I'm seeing some noticeable performance regressions (I include Kosaraju's for reference):

 name                        master ns/iter  new_tarjan ns/iter  diff ns/iter  diff %  speedup
 bigger_kosaraju_sccs        6,000           6,047                         47   0.78%   x 0.99
 bigger_tarjan_sccs          3,299           3,879                        580  17.58%   x 0.85
 full_kosaraju_sccs          1,047           1,047                          0   0.00%   x 1.00
 full_tarjan_sccs            538             658                          120  22.30%   x 0.82
 sccs_kosaraju_graph         2,687           2,607                        -80  -2.98%   x 1.03
 sccs_kosaraju_stable_graph  2,691           2,669                        -22  -0.82%   x 1.01
 sccs_tarjan_graph           1,809           2,134                        325  17.97%   x 0.85
 sccs_tarjan_stable_graph    1,755           2,087                        332  18.92%   x 0.84

I'll try to do some profiling.

Tests the visited flag before doing the recursive call.
@ABorgna
Copy link
Member

ABorgna commented Apr 26, 2021

I pushed a small change to your branch, it checks if the node has been visited before calling visit and hence avoids the recursive calls.
Now the benchmarks are much nicer :)

 name                        master ns/iter  new_tarjan_check ns/iter  diff ns/iter   diff %  speedup
 bigger_kosaraju_sccs        5,791           5,402                             -389   -6.72%   x 1.07
 bigger_tarjan_sccs          3,002           2,817                             -185   -6.16%   x 1.07
 full_kosaraju_sccs          1,156           976                               -180  -15.57%   x 1.18
 full_tarjan_sccs            487             438                                -49  -10.06%   x 1.11
 sccs_kosaraju_graph         2,405           2,367                              -38   -1.58%   x 1.02
 sccs_kosaraju_stable_graph  2,263           2,261                               -2   -0.09%   x 1.00
 sccs_tarjan_graph           1,460           1,249                             -211  -14.45%   x 1.17
 sccs_tarjan_stable_graph    1,478           1,223                             -255  -17.25%   x 1.21

@ABorgna
Copy link
Member

ABorgna commented Apr 26, 2021

Uhm, I'm getting pretty noisy benchmarks (full_kosaraju_sccs there got -15% with no code changes).
But the Tarjan implementations is consistently faster with your implementation.

If you are OK with this I think the PR is ready to merge.

@saolof
Copy link
Contributor Author

saolof commented Apr 26, 2021

Sounds great! When I have more time I'll work on an imperative version that can't stack overflow and microoptimize it in a followup PR. There's also a few other functions that depend on the SCC's such as condensation that could be rewritten to take advantage of the new node_component_index method in order to avoid work.

@ABorgna ABorgna merged commit 5e3a5b5 into petgraph:master Apr 26, 2021
teuron pushed a commit to teuron/petgraph that referenced this pull request Oct 9, 2022
* Modified Tarjan SCC to reduce memory usage.

Reduced heap memory usage to a single vec allocation, improved cache locality,
and made the code cleaner for a possible future rewrite to a more
optimized imperative version.

In addition, the new version has the property that it builds a lookup
table for the components. If v belongs to the kth output component, then
self.nodes[g.to_index(v)] == Some(k) . This can be used to save work in
functions that may want to call it such as condensation.

* added std:: in front of std::usize::MAX

* Rustfmt-ed the code.

* Added node_component_index, adopted NonZeroUsize, reversed counters.

* Minor change to node_component_index.

* Docs update. Added citation to paper with link.

* Improve tarjan scc performance

Tests the visited flag before doing the recursive call.

Co-authored-by: Agustin Borgna <agustinborgna@gmail.com>
teuron pushed a commit to teuron/petgraph that referenced this pull request Oct 9, 2022
* Modified Tarjan SCC to reduce memory usage.

Reduced heap memory usage to a single vec allocation, improved cache locality,
and made the code cleaner for a possible future rewrite to a more
optimized imperative version.

In addition, the new version has the property that it builds a lookup
table for the components. If v belongs to the kth output component, then
self.nodes[g.to_index(v)] == Some(k) . This can be used to save work in
functions that may want to call it such as condensation.

* added std:: in front of std::usize::MAX

* Rustfmt-ed the code.

* Added node_component_index, adopted NonZeroUsize, reversed counters.

* Minor change to node_component_index.

* Docs update. Added citation to paper with link.

* Improve tarjan scc performance

Tests the visited flag before doing the recursive call.

Co-authored-by: Agustin Borgna <agustinborgna@gmail.com>
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.

None yet

2 participants