Up-to-date version of PR #32 "Performance improvements and bugfix to Tarjan's algorithm"

[simsurace]

This replaces #32.

I ran into long runtimes with strongly_connected_components myself and then saw that there was this unmerged PR. I brushed it up a little and added the stress tests that have been requested. The performance claims made in the PR still hold against today's master branch:

Graph	nv / ne	master	this PR	speedup	memory (master → PR)
out-star	10⁴ / 10⁴	27.4 ms	146 µs	188×	1.28 → 0.94 MiB
out-star	10⁵ / 10⁵	2.72 s	1.75 ms	1550×	11.9 → 8.8 MiB
bidirectional star	10⁵ / 2·10⁵	2.79 s	1.34 ms	2090×	6.0 → 4.4 MiB
dense random (deg ≈ 790)	2·10³ / 1.6·10⁶	1.87 ms	1.54 ms	1.2×	175 → 148 KiB
tournament	2·10³ / 2.0·10⁶	2.02 ms	1.91 ms	1.1×	175 → 160 KiB
random (k ≈ 10)	5·10⁴ / 5·10⁵	5.68 ms	4.09 ms	1.4×	3.9 → 3.1 MiB
sparse (k ≈ 2)	10⁵ / 2·10⁵	8.18 ms	5.20 ms	1.6×	8.9 → 7.0 MiB
big cycle	2·10⁵ / 2·10⁵	4.94 ms	4.19 ms	1.2×	20.9 → 17.9 MiB
path (DAG)	2·10⁵ / 2·10⁵	7.00 ms	4.99 ms	1.4×	36.1 → 24.7 MiB

The asymptotic quadratic→linear win on star / high-out-degree graphs is confirmed (three orders of magnitude at 10⁵ vertices), and the new implementation is faster with lower memory on every other case as well. The small-margin rows (1.1×–1.6×) carry run-to-run noise but were consistently ≥ master.

[codecov]

Codecov Report

❌ Patch coverage is 96.77419% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.45%. Comparing base (9feb61a) to head (5ed31bf).

Files with missing lines	Patch %	Lines
src/connectivity.jl	96.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
- Coverage   97.47%   97.45%   -0.02%     
==========================================
  Files         128      128              
  Lines        7811     7830      +19     
==========================================
+ Hits         7614     7631      +17     
- Misses        197      199       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gdalle]

Hi @simsurace,
Sorry but I don't have the bandwidth to review this! Good luck

[simsurace]

No worries, I'll try to find someone else who participated in #32.

[KristofferC]

In my benchmarks, this cutoff is not great:

  - 1,024 vertices: it is 3.52× slower than ours (separate implementation).
  - 1,025 vertices: it is 14% faster.

[KristofferC]

FWIW, I've also encountered this (Ferrite-FEM/Ferrite.jl#1539). This PR makes us more or less able to drop the local implementation (the hard coded 1024 is not great for us though).

[simsurace]

That's oddly sensitive. Do you have a small reproducer? I will look into it later.

[KristofferC]

Why is it "oddly sensitive", it changes the alg used. Here is a benchmark:

using Graphs, BenchmarkTools

function out_star(n)
    g = SimpleDiGraph(n)
    for v in 2:n
        add_edge!(g, 1, v)
    end
    return g
end

for n in (1024, 1025)
    g = out_star(n)
    t = @belapsed strongly_connected_components_tarjan($g)
    tk = @belapsed strongly_connected_components_kosaraju($g)
    println("n = $n (center degree $(n - 1)): tarjan ", round(t * 1e6; digits=1), " μs, kosaraju ", round(tk * 1e6; digits=1), " μs")
end

n = 1024 (center degree 1023): tarjan 147.6 μs, kosaraju 99.5 μs
n = 1025 (center degree 1024): tarjan  11.5 μs, kosaraju 100.1 μs

graph	1024	64	16	1
star, deg 1023	147 μs	11.7 μs	11.6 μs	11.8 μs
star, deg 500	39.9 μs	6.0 μs	5.8 μs	5.8 μs
random k≈2, 10⁵	7.47 ms	6.58 ms	7.61 ms	7.84 ms
random k≈10, 5·10⁴	4.90 ms	5.13 ms	5.22 ms	5.50 ms
dense, 2·10³	799 μs	591 μs	591 μs	590 μs
cycle, 2·10⁵	4.75 ms	4.74 ms	4.43 ms	4.51 ms
path, 2·10⁵	4.68 ms	4.73 ms	4.73 ms	4.84 ms

Conclusion: Just remove the cutoff and use it unconditionally.