-
Notifications
You must be signed in to change notification settings - Fork 93
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
Speed up has_path
by avoiding popfirst!
and push!
#406
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #406 +/- ##
=======================================
Coverage 97.30% 97.30%
=======================================
Files 117 117
Lines 6948 6951 +3
=======================================
+ Hits 6761 6764 +3
Misses 187 187 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
May I ask what Julia version you used for these benchmarks? In the past this kind of optimization would often lead to significant speedups but from Julia I also think that perhaps the graphs you choose for benchmarking are kind of very small - for this kind of function we can also work with graphs with millions of vertices. |
This was on 1.10.5, so indeed before the The original version improves on 1.11 - but so does this revision (but the gap probably narrows a bit overall): julia> g = path_graph(4)
julia> @b has_path($g, 1, 3) # true
50.041 ns (5 allocs: 224 bytes)
julia> @b new_has_path($g, 1, 3) # true
42.559 ns (5 allocs: 192 bytes)
julia> g = path_graph(10)
julia> @b has_path($g, 1, 10) # true
94.484 ns (5 allocs: 240 bytes)
julia> @b new_has_path($g, 1, 10) # true
77.722 ns (5 allocs: 256 bytes)
julia> g = path_graph(10)
julia> @b has_path($g, 1, 10) # true
100.112 ns (5 allocs: 240 bytes)
julia> @b new_has_path($g, 1, 10) # true
79.012 ns (5 allocs: 256 bytes)
julia> pp = SimpleGraph(30, vcat([p.fadjlist, [v .+ (15) for v in p.fadjlist]]...))
julia> @b has_path($pp, 1, 16) # false
203.519 ns (6 allocs: 624 bytes)
julia> @b new_has_path($pp, 1, 16) # false
148.489 ns (5 allocs: 432 bytes)
julia> g = path_graph(1000)
julia> @b has_path($g, 1, 1000)
5.148 μs (5 allocs: 1.250 KiB)
julia> @b new_has_path($g, 1, 1000)
5.498 μs (6 allocs: 9.000 KiB)
julia> @b has_path($g, 1, 3)
101.500 ns (5 allocs: 1.250 KiB)
julia> @b new_has_path($g, 1, 3)
202.632 ns (6 allocs: 9.000 KiB)
Sure, that is an undeniable tradeoff. To me though, we are already instantiating an
Also fair; I just don't know that many canonical/conventional graph kinds, so I just picked a few of the ones provided by Graphs.jl. Open to suggestions. |
@thchr Can you rebase your PR on the lastest master? I think the nightly tests might succeed then. |
Thanks for this suggestion, but I would rather weigh in against greedy allocation of a huge vector that we may never need. I think @simonschoelly is right and relying on Julia's compiler to get better is the right call |
We could use an actual queue from DataStructures.jl instead of a vector though |
Okay, no worries; feel free to close.
I think that is likely to be a good bit slower for small graphs, probably big ones too; the |
This speeds up
has_path
considerably by allocating thenext
buffer up front, avoiding the need topush!
andpopfirst!
(and hence resize / reallocate) during the search.Examples below (
has_path
= old implementation;new_has_path
= implementation in this PR):It can occasionally be slower if the graph is very large and the vertices are very "close", e.g.:
For the above example, the new implementation is faster for all vertices separated by more than 14 steps.
Overall, I think this should be a very worthwhile trade-off for most applications.