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

add multithread-bfs #252

Merged
merged 1 commit into from
Dec 6, 2022
Merged

add multithread-bfs #252

merged 1 commit into from
Dec 6, 2022

Conversation

suncanghuai
Copy link
Contributor

@suncanghuai suncanghuai commented Dec 6, 2022

  • implement multi-thread bfs
  • add test cases for multi-thread bfs(in BFSTest.cpp)
  • add benchmark for multi-thread bfs(in BFS_BM.cpp)

I actually implement a much more simple multithread-bfs with OpenMP at first, which is very similar to the pseudocode of the paper from the corresponding issue.
But there are two problems with that version of implementation:

  • pseudo-multi-thread (parameter num_threads = 1) has a performance advantage over true multi-thread in each benchmark test, even when a graph is large.
  • it forces users to load the OpenMP package when they compile their projects

Finally, I change the code to a version with std::threads using condition_variable to synchronize threads and do some code optimization. Although the result of the benchmark is still not good as I expected, at least in the CitHepPh graph true multi-thread performs better than pseudo-multi-thread.
3U5MK3ALBi

* add testcases for multi-thread bfs(in BFSTest.cpp)
* add benchmark for multi-thread bfs(in BFS_BM.cpp)
@github-actions github-actions bot added core something about core repo something about repo test Something about test labels Dec 6, 2022
@ghost
Copy link

ghost commented Dec 6, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codecov-commenter
Copy link

Codecov Report

Merging #252 (7a9212d) into master (588f64f) will decrease coverage by 0.11%.
The diff coverage is 97.82%.

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
- Coverage   96.90%   96.79%   -0.12%     
==========================================
  Files          47       47              
  Lines        5500     5740     +240     
==========================================
+ Hits         5330     5556     +226     
- Misses        170      184      +14     
Flag Coverage Δ
unittests 96.79% <97.82%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/Graph/Graph.hpp 93.59% <94.93%> (+0.12%) ⬆️
test/BFSTest.cpp 99.61% <99.33%> (-0.39%) ⬇️
include/Partitioning/EdgeBalancedVertexCut.hpp 74.50% <0.00%> (-5.89%) ⬇️
include/Partitioning/GreedyVertexCut.hpp 84.69% <0.00%> (-5.11%) ⬇️
include/Partitioning/EBV.hpp 83.58% <0.00%> (-4.48%) ⬇️
include/Partitioning/WeightBalancedLibra.hpp 77.77% <0.00%> (-0.80%) ⬇️
include/Partitioning/CoordinatedPartitionState.hpp 75.96% <0.00%> (+1.21%) ⬆️
include/Partitioning/Partitioner.hpp 100.00% <0.00%> (+2.04%) ⬆️
include/Partitioning/HDRF.hpp 81.31% <0.00%> (+2.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ZigRazor
Copy link
Owner

ZigRazor commented Dec 6, 2022

Before I can accept this PR I think you should:

  • Give to this function an ifdef condition ( also in the cmake files ), in this way the library can be used with/without this functionality and OpenMP library;
  • Report the result in the readme, and explain the current problem of performances, in this way the users can choose better and as reminder for next study;
  • If possible correct some issue of the static code analysis ( this is optional )

Thank you in advance @suncanghuai

@ZigRazor ZigRazor self-requested a review December 6, 2022 07:46
@ZigRazor ZigRazor self-assigned this Dec 6, 2022
@suncanghuai
Copy link
Contributor Author

@ZigRazor perhaps I haven't described it clearly. I have changed the way multithread is implemented from “omp parallel” to std::thread and std::condition_variable, the version I apply to be merged is without OpenMP. The reason I talk about "omp parallel" is to explain why I didn't implement multithread-bfs exactly following the pseudocode of the paper. Besides, some benchmark results really should be added in the readme file so that users could know when to use multithread-bfs instead of bfs. And I will look into issues of static analysis, but may not fix them all.

@ZigRazor
Copy link
Owner

ZigRazor commented Dec 6, 2022

@suncanghuai Ok, i misunderstand, now is clear.
Good work and Thank you

@ZigRazor ZigRazor linked an issue Dec 6, 2022 that may be closed by this pull request
@ZigRazor ZigRazor added this to the Algorithm Implementation milestone Dec 6, 2022
@ZigRazor ZigRazor merged commit 5afdc18 into ZigRazor:master Dec 6, 2022
@suncanghuai
Copy link
Contributor Author

@ZigRazor I'm gonna add something to readme file and fix some issus of static analysis in this MR,but you have merged it.So I suppose I have to put them in my next MR.

@ZigRazor
Copy link
Owner

ZigRazor commented Dec 6, 2022

Yes, thank you

@ZigRazor
Copy link
Owner

ZigRazor commented Dec 7, 2022

@suncanghuai given you contributions and your follow to the project I think you can be upgraded as collaborator of the project, what do you think about? You want to become collaborator?

@suncanghuai
Copy link
Contributor Author

@suncanghuai given you contributions and your follow to the project I think you can be upgraded as collaborator of the project, what do you think about? You want to become collaborator?

Sure, I'm glad to work for this project as a collaborator.

ZigRazor added a commit that referenced this pull request Mar 21, 2023
* introduce topological sort for graph (#247)

* implement topological sort based on dfs
* add benchmark for topological sort
* add basic test for topological sort

Co-authored-by: suncanghuai <[email protected]>

* Fix typo in link specification (#248)

This PR removes an additional space character between link text and link URL, which caused faulty formatting of the README.md file.

* introduce WeightBalancedLibra algorithm(vertex-cut graph partition) (#249)

* implement WeightBalancedLibra algorithm based on paper pseudocode
* add weight-related apis for class CoordinatedPartitionState
* adjust set operations of class CoordinatedRecord
* append a testcase in PartitionTest.cpp for WB-Libra

Co-authored-by: suncanghuai <[email protected]>

* Update README.md

* Update README.md

* Install the CodeSee workflow. Learn more at https://docs.codesee.io (#250)

Co-authored-by: codesee-maps[bot] <86324825+codesee-maps[bot]@users.noreply.github.com>

* * implement multi-thread bfs (#252)

* add testcases for multi-thread bfs(in BFSTest.cpp)
* add benchmark for multi-thread bfs(in BFS_BM.cpp)

Co-authored-by: suncanghuai <[email protected]>

* Update Readme Roadmap

* Implement best first search (#254)

* first implementation and tests

* add docs and minor changes

* minor change

* minor change

* Update README.md

* Update README.md

* Include best first search test (#258)

* Update README for best first search algorithm (#257)

* Update README for best first search algorithm

* minor change

* minor change

* minor review changes

* Implement kahn's algorithm for topological sorting (#259)

* Improved return type for Kosaraju's algoritm + tests (#260)

* custom return type for kosaraju()

* Tests for Kosaraju's algorithm

* fixed minor issues

* fixed merge issue

* Update Road Map

* Update README.md

* Reworked Cmake

Signed-off-by: GitHub <[email protected]>

* Add partition Example ( HDRF )

Signed-off-by: GitHub <[email protected]>

* Corrected Partition Class
Fix #263

Signed-off-by: GitHub <[email protected]>

* Update Readme for Roadmap

Signed-off-by: GitHub <[email protected]>

* Corrected Cmake for old Example

Signed-off-by: GitHub <[email protected]>

* Update cmake.yml

* Update benchmark_pr.yml

* Update benchmark.yml

* Update Code_Coverage.yml

* Correction for Graph.hpp

Signed-off-by: GitHub <[email protected]>

* Update Code_Coverage.yml

Parallel compilation

* Update benchmark.yml

parallel compilation

* Update benchmark_pr.yml

parallel compilation

* Update cmake.yml

parallel compilation

* Update codeql-analysis.yml

remove useless steps:
- manually installed google test and benchmark

* Create .github/workflows/super-linter.yml

Added Super-Linter

* Update super-linter.yml

* Create .clang-format

* Reformatted Files
with clang-format with syle "Google"

Signed-off-by: GitHub <[email protected]>

* Delete super-linter.yml

* Create .github/workflows/codeql.yml

* Delete codeql-analysis.yml

* Update README.md

* Create .github/workflows/codacy.yml

* Create .github/workflows/snyk-security.yml

* Update snyk-security.yml

* Delete snyk-security.yml

---------

Signed-off-by: GitHub <[email protected]>
Co-authored-by: ARockHammer <[email protected]>
Co-authored-by: suncanghuai <[email protected]>
Co-authored-by: David Chocholatý <[email protected]>
Co-authored-by: suncanghuai <[email protected]>
Co-authored-by: codesee-maps[bot] <86324825+codesee-maps[bot]@users.noreply.github.com>
Co-authored-by: Pradeep Krishnamurthy <[email protected]>
Co-authored-by: David Sapienza <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core something about core repo something about repo test Something about test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-Thread implementation of BFS
3 participants