Optimize: Update transitive closure algorithm#920
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #920 +/- ##
==========================================
+ Coverage 77.14% 77.18% +0.04%
==========================================
Files 130 130
Lines 11868 11883 +15
Branches 1458 1462 +4
==========================================
+ Hits 9155 9172 +17
+ Misses 2331 2330 -1
+ Partials 382 381 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous implementation scanned all edges on every iteration of a fixed-point loop, resulting in O(depth * edges) complexity. For deep dependency chains this is catastrophic - a 5000-node chain required 5000 full passes over all edges. Replace with a standard Breadth-First Search using an adjacency list built once upfront, reducing complexity to O(vertices + edges). Benchmarks on large graphs show 22x-1333x speedups depending on graph shape, with identical results verified against the original algorithm.
86d36d8 to
56fb085
Compare
|
If you haven't already, testing this with Gecko would be a good additional sanity check (eg: that it produces identical graphs, no exceptions, etc.). |
I have tested this against gecko and found no diffs on the generated graphs. |
bhearsum
left a comment
There was a problem hiding this comment.
I'm -0 on this...it adds complexity for no real world win. As long as it doesn't hurt perf in our real world cases I guess it's fine to take.
ahal
left a comment
There was a problem hiding this comment.
Seems fine to me. It's a few more lines of code, but I can't really say it's much more complex than the old algorithm
Merging this PR will improve performance by ×3.3
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_transitive_closure[fan] |
881.2 µs | 2,056.4 µs | -57.15% |
| ⚡ | test_transitive_closure[linear] |
196.9 ms | 2.3 ms | ×87 |
| ⚡ | test_transitive_closure[diamond] |
390 ms | 133.9 ms | ×2.9 |
| ⚡ | test_transform_sequence |
6.9 ms | 6.1 ms | +12.43% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing hneiva/transitive-closure (1cee94b) with main (81f3b38)
The previous implementation scanned all edges on every iteration of a fixed-point loop, resulting in O(depth * edges) complexity. For deep dependency chains this is catastrophic - a 5000-node chain required 5000 full passes over all edges.
Replace with a standard Breadth-First Search using an adjacency list built once upfront, reducing complexity to O(vertices + edges). Benchmarks on large graphs show 22x-1333x speedups depending on graph shape, with identical results verified against the original algorithm.