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 multithreading to BinaryArithmetic to increase speed. #29

Closed
wants to merge 3 commits into from

Conversation

KingAkeem
Copy link
Contributor

This PR references issue #21 I've added multithreading code for concurrently running four strip_leading_zeroes functions. This has reduced the test time of the BinaryArithemetic tests by 3x as much. I've attached some photos of comparison below:
Before multithreading:
screen shot 2018-05-10 at 6 20 28 am
After multithreading:
screen shot 2018-05-10 at 7 38 33 am

@faheel
Copy link
Owner

faheel commented May 10, 2018

This is great! Can you also take a look at other places where multithreading can be used to speed up things? For example, calculating prod_high, prod_mid and prod_low on different threads may lead to a significant speedup.

@KingAkeem
Copy link
Contributor Author

Thank you! I definitely was planning on it. I just wanted to get a review on what I had already done to show the improvement and let you know that I was working on it.

@KingAkeem
Copy link
Contributor Author

I attempted to add more threads for calculating prod variables and it seems that there is a bottleneck when it comes to spawning threads. After the first 4, the overhead cost of spawning threads causes a decrease in speed.

Copy link
Owner

@faheel faheel left a comment

Choose a reason for hiding this comment

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

Just a small change and then it'll be good to merge.

std::thread t1(strip_leading_zeroes, std::ref(num1_high.value));
std::thread t2(strip_leading_zeroes, std::ref(num1_low.value));
std::thread t3(strip_leading_zeroes, std::ref(num2_high.value));
std::thread t4(strip_leading_zeroes, std::ref(num2_low.value));
Copy link
Owner

Choose a reason for hiding this comment

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

The threads t1, t2, ... aren't very descriptive and should be renamed to thread1, thread2, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the fix :)

@faheel faheel force-pushed the master branch 2 times, most recently from 6bf4812 to 69dd72f Compare May 14, 2018 13:38
@faheel
Copy link
Owner

faheel commented May 14, 2018

@KingAkeem I've updated the testing and build mechanism recently, so can you merge the recent changes to your branch and push it again to that we can if the build still fails (only 1 build, that uses GCC 8 and does coverage analysis should fail, the rest should pass).

@faheel faheel added enhancement New feature or request optimisation Performance optimisation labels May 18, 2018
@faheel
Copy link
Owner

faheel commented May 18, 2018

@KingAkeem Please merge with latest upstream (for the reason mentioned above)

@KingAkeem
Copy link
Contributor Author

@faheel I've merged the recent changes and pushed the branch. Sorry about taking so long, I just saw your comment.

@faheel
Copy link
Owner

faheel commented May 19, 2018

Looks like on Linux we'll need to add -pthread when compiling to link the pthread library. It's not that big of a deal, but then the compilation step will require special attention from the user which impacts the main features of this library, which are simplicity and ease of use. So I'll need to think about this and see what's the best thing to do here.

What are your thoughts on this @KingAkeem?

@KingAkeem
Copy link
Contributor Author

@faheel Is there no way to add the -pthread library in the release script? But we're going to have to have to use some library to provide us with the ability to use threads in cpp.

@KingAkeem KingAkeem closed this Feb 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request optimisation Performance optimisation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants