Return to Answer

• It seems that you forgot to include the header <cmath> for std::sqrt. Your implementation may include it from another header, but that's not guaranteed by the standard. My implementation produced a compile-time error because of the lack of <cmath>.

• threads.push_back(thread([&]{ /* ... */ })); is redundant: we already know that threads is a std::vector<std::thread>, so let's write threads.emplace_back([&]{ /* ... */ }); instead.

• You have problems due to paralellism. When I try to run your program, I get an std::out_of_range exception thrown which says I tried to access the nth element of a std::vector of size n. I did not understand all the details, but it comes fromm this loop:

   for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&]{ prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  In this loop, the lambda captures all the variables by reference and that's a bad idea: once the thread is spawn, i might be incremented before inputVector.at(i) is called. To avoid this problem, you should take i by value in the capture instead of taking it by reference:

   threads.emplace_back([&, i]{ /* ... */ }); 

  Capturing i by value ensures that it won't be modified by another thread. In multithreaded code, it is really important to think about what should be taken by value and what should be taken by reference.

• Moreover, on the same piece of code, you have concurrent stores on primes through the method push_back. While I didn't have any error - it seems that you didn't either -, this may be a data race. Therefore, you should use a std::mutex to lock primes before pushing values to it (the same applies for result1):

   std::mutex primes_mutex; for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&, i]{ std::lock_guard<std::mutex> lock(primes_mutex); prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  Note that I used a, std::lock_guard to guard the std::mutex. It avoids to forget a potential call to unlock since it invokes the unocking function when its destructor is called.

• I love to focus on small pieces of code, so there we go: the loop above can still be improved thanks to the C++11 range-based for loop:

   std::mutex primes_mutex; for (long long val: inputVector) { PrimeNumber prime; prime.setInitValue(val); threads.emplace_back([&, val]{ std::lock_guard<std::mutex> lock(primes_mutex); prime.setVector(result1 = getPrimes(val, temp1)); primes.push_back(prime); }); } 

  This iteration style allows to avoid the double concurrent lookup of inputVector at the same position. While it was already data-race free (the standard guarantees that concurrent loads do not induce data races), the less variables to read and write, the less there is to think.

• Also (yeah, still the same loop), I don't understand why you left the initialization of prime out of the thread: there is no point in leaving it out, and it forces the lambda to capture yet another parameter by reference, which would have been avoided had you declared prime directly into the new thread.

• It seems that you forgot to include the header <cmath> for std::sqrt. Your implementation may include it from another header, but that's not guaranteed by the standard. My implementation produced a compile-time error because of the lack of <cmath>.

• threads.push_back(thread([&]{ /* ... */ })); is redundant: we already know that threads is a std::vector<std::thread>, so let's write threads.emplace_back([&]{ /* ... */ }); instead.

• You have problems due to paralellism. When I try to run your program, I get an std::out_of_range exception thrown which says that I tried to access the nth element of a std::vector of size n. I did not understand all the details, but it comes from this loop:

   for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&]{ prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  In this loop, the lambda captures all the variables by reference and that's a bad idea: once the thread is spawn, i might be incremented before inputVector.at(i) is called. To avoid this problem, you should take i by value in the capture instead of taking it by reference:

   threads.emplace_back([&, i]{ /* ... */ }); 

  Capturing i by value ensures that it won't be modified by another thread meanwhile. In multithreaded code, it is really important to think about what should be taken by value and what should be taken by reference.

• Moreover, on the same piece of code, you have concurrent stores on primes through the method push_back. While I didn't get any error - it seems that you didn't either -, this may be a data race. Therefore, you should use an std::mutex to lock primes before pushing values to it (that comment is also valid for result1):

   std::mutex primes_mutex; for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&, i]{ std::lock_guard<std::mutex> lock(primes_mutex); prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  Note that I used a, std::lock_guard to guard the std::mutex. It avoids to forget a potential call to unlock since it invokes the unlocking function when its destructor is called.

• I love to focus on small pieces of code, so there we go: the loop above can still be improved thanks to the C++11 range-based for loop:

   std::mutex primes_mutex; for (long long val: inputVector) { PrimeNumber prime; prime.setInitValue(val); threads.emplace_back([&, val]{ std::lock_guard<std::mutex> lock(primes_mutex); prime.setVector(result1 = getPrimes(val, temp1)); primes.push_back(prime); }); } 

  This iteration style allows to avoid the double concurrent lookup of inputVector at the same position. While it was already data-race free (the standard guarantees that concurrent loads do not induce data races), the less variables to read and write, the less there is to think about.

• Also (yeah, still the same loop), I don't understand why you left the initialization of prime out of the thread: there is no point in leaving it out, and it forces the lambda to capture yet another parameter by reference, which would have been avoided had you declared prime directly into the new thread.

• It seems that you forgot to include the header <cmath> for std::sqrt. Your implementation may include it from another header, but that's not guaranteed by the standard. My implementation produced a compile-time error because of the lack of <cmath>.

• threads.push_back(thread([&]{ /* ... */ })); is redundant: we already know that threads is a std::vector<std::thread>, so let's write threads.emplace_back([&]{ /* ... */ }); instead.

• You have problems due to paralellism. When I try to run your program, I get an std::out_of_range exception thrown which says I tried to access the nth element of a std::vector of size n. I did not understand all the details, but it comes fromm this loop:

   for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&]{ prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  In this loop, the lambda captures all the variables by reference and that's a bad idea: once the thread is spawn, i might be incremented before inputVector.at(i) is called. To avoid this problem, you should take i by value in the capture instead of taking it by reference:

   threads.emplace_back([&, i]{ /* ... */ }); 

  Capturing i by value ensures that it won't be modified by another thread. In multithreaded code, it is really important to think about what should be taken by value and what should be taken by reference.

• Moreover, on the same piece of code, you have concurrent stores on primes through the method push_back. While I didn't have any error - it seems that you didn't either -, this may be a data race. Therefore, you should use a std::mutex to lock primes before pushing values to it:

   std::mutex primes_mutex; for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&]{ prime.setVector(result1 = getPrimes(inputVector.at(i), temp1));   std::lock_guard<std::mutex> lock(primes_mutex); primes.push_back(prime); }); } 

  Note that I used a, std::lock_guard to guard the std::mutex. It avoids to forget a potential call to unlock since it invokes the unocking function when its destructor is called.

• It seems that you forgot to include the header <cmath> for std::sqrt. Your implementation may include it from another header, but that's not guaranteed by the standard. My implementation produced a compile-time error because of the lack of <cmath>.

• threads.push_back(thread([&]{ /* ... */ })); is redundant: we already know that threads is a std::vector<std::thread>, so let's write threads.emplace_back([&]{ /* ... */ }); instead.

• You have problems due to paralellism. When I try to run your program, I get an std::out_of_range exception thrown which says I tried to access the nth element of a std::vector of size n. I did not understand all the details, but it comes fromm this loop:

   for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&]{ prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  In this loop, the lambda captures all the variables by reference and that's a bad idea: once the thread is spawn, i might be incremented before inputVector.at(i) is called. To avoid this problem, you should take i by value in the capture instead of taking it by reference:

   threads.emplace_back([&, i]{ /* ... */ }); 

  Capturing i by value ensures that it won't be modified by another thread. In multithreaded code, it is really important to think about what should be taken by value and what should be taken by reference.

• Moreover, on the same piece of code, you have concurrent stores on primes through the method push_back. While I didn't have any error - it seems that you didn't either -, this may be a data race. Therefore, you should use a std::mutex to lock primes before pushing values to it (the same applies for result1):

   std::mutex primes_mutex; for (int i = 0; i < inputVector.size(); i++) { PrimeNumber prime; prime.setInitValue(inputVector.at(i)); threads.emplace_back([&, i]{ std::lock_guard<std::mutex> lock(primes_mutex); prime.setVector(result1 = getPrimes(inputVector.at(i), temp1)); primes.push_back(prime); }); } 

  Note that I used a, std::lock_guard to guard the std::mutex. It avoids to forget a potential call to unlock since it invokes the unocking function when its destructor is called.

• I love to focus on small pieces of code, so there we go: the loop above can still be improved thanks to the C++11 range-based for loop:

   std::mutex primes_mutex; for (long long val: inputVector) { PrimeNumber prime; prime.setInitValue(val); threads.emplace_back([&, val]{ std::lock_guard<std::mutex> lock(primes_mutex); prime.setVector(result1 = getPrimes(val, temp1)); primes.push_back(prime); }); } 

  This iteration style allows to avoid the double concurrent lookup of inputVector at the same position. While it was already data-race free (the standard guarantees that concurrent loads do not induce data races), the less variables to read and write, the less there is to think.

• Also (yeah, still the same loop), I don't understand why you left the initialization of prime out of the thread: there is no point in leaving it out, and it forces the lambda to capture yet another parameter by reference, which would have been avoided had you declared prime directly into the new thread.

Obviously, the best way to speed up your code woud be to change it to use a best algorithms. However, the other posts already do a great job covering that part. Therefore, I will do a review of the other things you could have done better:

Obviously, the best way to speed up your code woud be use another algorithm. However, the other posts already do a great job covering that part. Therefore, I will review the other things in your code that could have been done better: