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

Improve performance of bfs functions #250

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

Tortar
Copy link
Contributor

@Tortar Tortar commented Apr 27, 2023

I added an optimization to these bfs functions which I benchmarked with

using Graphs, BenchmarkTools

gs = [path_graph(500),
      path_graph(5000),
      path_graph(50000),
      erdos_renyi(50, 0.5, seed=123),
      erdos_renyi(500, 0.5, seed=123),
      erdos_renyi(5000, 0.5, seed=123),
      complete_graph(50),
      complete_graph(500),
      complete_graph(5000)]

for g in gs
    n = nv(g)
    u = Int(n/5)
    dists = vec(fill(typemax(Int), (1, n)))
    t_1 = @belapsed gdistances!($g, $u, $dists)
    t_2 = @belapsed bfs_parents($g, $u)
    println("$t_1 $t_2;")
end

and then I calculated the relative speed-up for the graphs in gs before and after this pr:

 gdistances! bfs_parents
     0.99      0.99            --> path_graph(500)
     0.98      0.99            --> path_graph(5000)
     0.99      1.00            --> path_graph(50000)
     3.18      3.14            --> erdos_renyi(50, 0.5, seed=123)
    16.91      15.42           --> erdos_renyi(500, 0.5, seed=123)
   126.52      120.08          --> erdos_renyi(5000, 0.5, seed=123)
     5.40      5.19            --> complete_graph(50)
    46.14      40.11           --> complete_graph(500)
   512.26      475.42          --> complete_graph(5000)

The Path Graph is kind of the worst case scenario, and the time is approximately equal to the algorithm before this pr, but, as you can see, with a well connected Erdos Renyi the speed-up becomes very good, and finally I choose to benchmark a Complete Graph since it should be the best case scenario.

I also ran the tests locally and everything should still work!

@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #250 (1991e25) into master (53cb581) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
- Coverage   97.23%   97.22%   -0.02%     
==========================================
  Files         114      114              
  Lines        6585     6595      +10     
==========================================
+ Hits         6403     6412       +9     
- Misses        182      183       +1     

@gdalle gdalle added the enhancement New feature or request label Jun 16, 2023
@Tortar
Copy link
Contributor Author

Tortar commented Aug 31, 2023

@gdalle If I can ask, what do you think about this PR? Since to me it seems a pretty good speed-up to have :-)

Copy link
Member

@etiennedeg etiennedeg left a comment

Choose a reason for hiding this comment

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

Looks good to me

@etiennedeg etiennedeg merged commit aeb7e30 into JuliaGraphs:master Aug 31, 2023
@Tortar Tortar deleted the patch-1 branch August 31, 2023 21:28
@simonschoelly
Copy link
Contributor

This PR adds a heuristic that detects early that every vertex has been finished - it makes sense that it increases the speed in case of graphs with low diameter.

On the other hand, it adds some unnecessary if statements and a variable named k that could easily have been called something like num_visited. I think we should address such things in a code review instead of just merging them.

@etiennedeg
Copy link
Member

The if statements are necessary, if some vertices are duplicated in source then k will be incremented too much.

@simonschoelly
Copy link
Contributor

The if statements are necessary, if some vertices are duplicated in source then k will be incremented too much.

Ok thats a fair point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants