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

✨ Added multi-threading support for QuickSim #128

Merged
merged 10 commits into from
Feb 23, 2023

Conversation

Drewniok
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #128 (d5240bd) into main (3ce5bf0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   93.87%   93.88%   +0.01%     
==========================================
  Files          75       75              
  Lines        6936     6955      +19     
==========================================
+ Hits         6511     6530      +19     
  Misses        425      425              
Impacted Files Coverage Δ
...ion/algorithms/simulation/sidb/is_ground_state.hpp 100.00% <ø> (ø)
...on/algorithms/simulation/sidb/time_to_solution.hpp 96.87% <ø> (ø)
...de/fiction/algorithms/simulation/sidb/quicksim.hpp 98.00% <100.00%> (+0.94%) ⬆️
...fiction/technology/charge_distribution_surface.hpp 94.62% <100.00%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc56a83...d5240bd. Read the comment docs.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@marcelwa
Copy link
Collaborator

Why are you using std::thread instead of std::execution_policy?

@Drewniok
Copy link
Collaborator Author

Actually, my first step was to use std::execution_policy since you mentioned this once. However, I was not able to make it work. As far as I can see, Apple Clang does not yet support parallel algorithms and execution policies.

@marcelwa
Copy link
Collaborator

I can't say for sure and your milage may vary, but I had some luck writing a thin abstraction layer on top of std::execution_policy that makes the general concept available error-free on all platforms we're continuously testing on. That is, if policies are supported, they are used, if not, not. This might still not be the solution you were hoping for and it might not even work on Apple Silicon chips. That requires further testing. If you want to try it at least, please check out the execution policy macros on the placer branch.

@Drewniok
Copy link
Collaborator Author

Drewniok commented Feb 22, 2023

Sounds really interesting! Maybe we can keep this in mind. For now, I would suggest using the current implementation since the performance increases as expected and I have already tested/analyzed all the different benchmarks. What is your opinion? I mean, you have more experience with multithreading than I do. If you think there is a benefit, I will have a look.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@marcelwa
Copy link
Collaborator

I agree with you.

@marcelwa
Copy link
Collaborator

Could you please add a few tests for varying thread counts? After that, this PR is ready to be merged.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@marcelwa marcelwa added enhancement New feature or request performance Performance improvements labels Feb 23, 2023
@marcelwa marcelwa changed the title ✨ multithreading for QuickSim ✨ Added multi-threading support for QuickSim Feb 23, 2023
@Drewniok Drewniok merged commit 26bb1a8 into cda-tum:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Performance improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants