Multithread for loop function not working correctly

Question

I tried to implement simple for loop running on custom number of threads like this:

#include <thread>

template<typename func, typename... args>
void threadFor(int numOfThreads, int repeats, func f, args... a)
{
    auto forPart = [&](int repeats){
        for (int i = 0; i < repeats; ++i) f(i, a...);
    };
    
    int part      = repeats / numOfThreads;
    int remainder = repeats % numOfThreads;

    std::thread threads[numOfThreads];

    for (int i = 0; i < numOfThreads; ++i)
    {
        threads[i] = std::thread(forPart,part + (i < remainder));
    }

    for (int i = 0; i < numOfThreads; ++i) threads[i].join();
}

I am testing it in the following way:

#include <iostream>

void increment(int i, int* j){++*j;}

int main()
{
    int j = 0;
    threadFor(8,1000000, increment, &j);

    std::cout << j;
}

When I execute the code, j is less than 1,000,000. Also, the code sometimes crushes because of double deletion or segmentation error. What am I doing wrong?

Edit your question to quote that error and identify where it came from, please. And if your code crashes, then you can use a debugger to trap those errors and figure out their source. — underscore_d
– underscore_d, Commented Aug 28, 2020 at 16:21
std::thread threads[numOfThreads]; is not valid c++. Use a vector instead. — cigien
– cigien, Commented Aug 28, 2020 at 16:30

Vlad Feinstein · Accepted Answer · 2020-08-28 16:42:01Z

1

You need to synchronize your threads' access to the shared variable.

std::atomic?

This gets 1,000,000:

#include <thread>
#include <vector>
#include <atomic>
#include <iostream>

template<typename func, typename... args>
void threadFor(int numOfThreads, int repeats, func f, args... a)
{
  auto forPart = [&](int repeats) {
    for (int i = 0; i < repeats; ++i) f(i, a...);
  };

  int part = repeats / numOfThreads;
  int remainder = repeats % numOfThreads;

  std::vector<std::thread> threads;// [numOfThreads];
  threads.reserve(numOfThreads);

  for (int i = 0; i < numOfThreads; ++i)
  {
    threads.push_back(std::thread(forPart, part + (i < remainder)));
  }

  for (int i = 0; i < numOfThreads; ++i) threads[i].join();
}


void increment(int i, std::atomic<int>* j) { ++*j; }

int main()
{
  std::atomic<int> j = 0;
  threadFor(8, 1000000, increment, &j);

  std::cout << j;
}

edited Aug 28, 2020 at 16:42

answered Aug 28, 2020 at 16:21

Vlad Feinstein

11.3k1 gold badge14 silver badges32 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Martin Vít Vavřík Over a year ago

Why is it better to use vector than c-style array?

Vlad Feinstein Over a year ago

@MartinVítVavřík - C++ standard doesn't allow arrays with non-compile-time-const dimension. [ADDED] see cigien's comment to your question

cigien · Accepted Answer · 2020-08-28 16:14:33Z

1

Your increment function never actually modifies the j argument:

void increment(int i, int* j) { 
    *j++; 
}

In this definition, you increment j (which is a copy of the original pointer) to an invalid location, and dereference it, invoking undefined behavior.

Instead, you need to do:

void increment(int i, int* j) { 
    ++*j;
}

which dereferences j, giving an l-value that is incremented.

answered Aug 28, 2020 at 16:14

cigien

61.2k11 gold badges86 silver badges124 bronze badges

1 Comment

Martin Vít Vavřík Over a year ago

Thanks, but it does not seem to be the only error, in my debug script I have rewritten it as *j += 1 (and know also as (*j) += 0) and I am still getting double free error.

Collectives™ on Stack Overflow

Multithread for loop function not working correctly

2 Answers 2

2 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related