Threadpool support for matrix multiplication #124
Conversation
youben11
added
the
Type: Improvement 📈
bcebere
added
the
Type: Refactor 🔨
| if (tenseal_context->get_concurrency() == 1) | ||
| return worker_func(0, vector_size); | ||
|
|
||
| size_t thread_cnt = tenseal_context->get_concurrency(); | ||
| std::vector<std::future<Ciphertext>> future_results; | ||
| size_t batch_size = (vector_size + thread_cnt - 1) / thread_cnt; | ||
|
|
||
| for (size_t i = 0; i < thread_cnt; i++) { | ||
| future_results.push_back(tenseal_context->dispatcher()->enqueue_task( | ||
| worker_func, i * batch_size, | ||
| std::min((i + 1) * batch_size, vector_size))); | ||
| } | ||
|
|
||
| for (size_t i = 0; i < tenseal_context->get_concurrency(); i++) { | ||
| tenseal_context->evaluator->add_inplace(result, | ||
| future_results[i].get()); | ||
| } |
There was a problem hiding this comment.
I like this new design without threads synchronization! But I'm wondering about the distribution of ranges, my thoughts about whether specify the range in advance vs. let them compute collectively till the end is that the former doesn't include synchronization but works well in case where threads might not run in an equitable manner (suppose one thread doesn't run like the other, so they will still wait for it to finish its range when other threads might have finished it), while the latter include synchronization but make the threads work hard till the end. All this is just me with my imagination, I don't know how this translate in practice, do you have any thoughts about this? But of course, when we don't have synchronization, we can go up to many threads without the fear of threads' synchronization becoming a bottleneck.
There was a problem hiding this comment.
So, you're saying that this loop might get stuck at 0, let's say, waiting for a slow thread, while the others finished the job, right?
Windows has a mechanism for waiting for multiple objects efficiently, unfortunately I cannot find something similar in standard C++ library, but I'll research it. I was looking for a solution too here, it's not ideal to loop over the futures and do a blocking wait.
I could experiment with more designs. One other idea was to add the results to a blocking queue, and notify the main thread using a condition variable. That might schedule the operations more efficiently.
There was a problem hiding this comment.
The current design is great, I don't think we should jump directly into a different one, I just wanted to give it some thoughts. But this is definitely something I would merge
There was a problem hiding this comment.
I'll make a test with a condition variable&mutex, to see how the benchmarks look that way.
There was a problem hiding this comment.
I did a test using a mutex and a condition variable to notify the main thread(instead of waiting for a std::future), but the benchmarks look pretty similar.
I think it needs better load testing here, I am not sure how to simulate the scenario.
Maybe a random sleep in each worker might show the performance differences(cond variable vs future.get() )
I have the git stash anyway, I can try to add a test in another PR.
Co-authored-by: Ayoub Benaissa <ayouben9@gmail.com>
There was a problem hiding this comment.
LGTM! I just think the API for matmul with n_jobs would be great
vec.matmul(matrix, n_jobs=j) with:
- 0 being the default (which uses the current number of threads in the threadpool)
- j > 0, first verify that the current number of threads is greater or equal than j, and run only j jobs
* add benchmarks for mamul_plain operation * add threadpool implementation Co-authored-by: Ayoub Benaissa <ayouben9@gmail.com>
Description
The current flow spawns a number of threads on every multiplication.This can lead to denial of service, we should have a controlled way of creating new threads.
For that, this PR extends the current approach with:
fixes #118
Checklist