Optimize find_shortest_chains #238
Conversation
CodSpeed Instrumentation Performance ReportMerging #238 will degrade performances by 53.36%Comparing Summary
Benchmarks breakdown
|
Peter554
changed the title
find_shortest_chains Performance
Peter554
changed the title
find_shortest_chains Performance
Peter554
force-pushed
the
improve-find-shortest-chains
branch
2 times, most recently
from
dc83c71 to
3a7775c
Compare
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Thanks for looking into this!
It's great to see the improvement for no chains found, but there are several other things which are apparently slower. What are you thoughts? Possibly one for a verbal discussion.
|
|
||
| * Add closed layers to layer contract. | ||
| * Rename default repository branch to 'main'. | ||
| * Optimise `find_shortest_chains` query. |
There was a problem hiding this comment.
Might be worth mentioning that it might lead to slightly different results in some cases? (I agree with you, the results of find_shortest_chains is not formally specified, though I'd like us to get to that point.)
Peter554
force-pushed
the
improve-find-shortest-chains
branch
from
3a7775c to
acb4720
Compare
…s utility This changes the algorithm used by find_shortest_chains. The algorithm now matches that used by find_illegal_dependencies. This algorithm is much more performant (see change in benchmark). Neither the previous or changed algorithm are fully exhaustive, so I don't think this counts as a breaking change. The algorithm used by find_shortest_chains was never formally specified, I believe. All test cases still pass, so the behaviour is practically near-unchanged.
Peter554
force-pushed
the
improve-find-shortest-chains
branch
from
acb4720 to
d57385e
Compare
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Amazing speed up! 👏🏻
I was a bit concerned about the slowdowns in some of the benchmarks, but have run this branch on a very large code base and it doesn't seem to cause a noticeable slowdown - and makes one problematic contract much, much faster.
| // We'll add chains to this set as we discover them. | ||
| let mut excluded_imports = FxHashMap::default(); | ||
|
|
||
| loop { |
There was a problem hiding this comment.
I've noticed that in a long-running call we can't exit using CMD-C or equivalent, might be a nice improvement in another PR to listen for that somehow.
2 participants
This PR improves the performance of the
find_shortest_chainsfunction by refactoring the underlying implementation to use a more efficient algorithm. The original implementation used a cartesian product of all possible module pairs, which resulted in an O(n²) complexity and poor performance on large graphs.Changes:
_find_shortest_chainshelper function that can be reused across different queriesfind_illegal_dependencieswith calls to the new helper functionfind_shortest_chainsto use the same algorithm asfind_illegal_dependencies, resulting in significantly better performance in realistic scenarios.This optimization is particularly beneficial for large dependency graphs where the O(n²) behavior of the previous implementation could cause performance bottlenecks.