• Advertisement
Sign in to follow this  

vector push_back creates error when multi threading

This topic is 932 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

void doit(std::vector<int>& d){
	std::mutex m;
	{
		std::lock_guard<std::mutex> guard(m);
		for (int i = 0; i < 10000;i++)
			d.push_back(15);
	}
	
	
}

If I execute that function on 2 threads, with a vector from the main app thread as a reference, then I get all kinds of heap & memory errors.

But why?  The vector d is locked when that push_back takes place. 

 

Share this post


Link to post
Share on other sites
Advertisement
No. Every time you call doit, the function creates its own mutex. As written, it is completely useless and just wastes CPU cycles. No blocking will ever happen.

You need to do something like this:

std::mutex my_global_doit_mutex;

void doit(std::vector<int>& d)
{
   std::lock_guard<std::mutex> guard(my_global_doit_mutex);
   for (int i = 0; i < 10000;i++)
      d.push_back(15);
}
This is the simplest possible example.

Share this post


Link to post
Share on other sites

I'm not too much of an expert on threading, but essencially it should be better in your above example to handle the lock like this:



void doit(std::vector<int>& d){
    static std::mutex m;
    {
        std::vector<int> dTemp;
        for (int i = 0; i < 10000;i++)
            dTemp.push_back(15);
            
        std::lock_guard<std::mutex> guard(m);
        d += dTemp;
    }
}

Side note on this - if you're using a MS compiler older than 2015, or any other not-C++11-conforming compiler this will not work as the initialization of the mutex will not be correctly handled if two threads call the function for the first time at the same time.

(Look for your compiler's implementation of "Magic Statics")

On non-conforming compilers you'll want to move the variable out of the function entirely.

Share this post


Link to post
Share on other sites

And ideally, you pass the mutex as a parameter.

 

Statics/globals are the enemy of multi-threading, and having that singular mutex pretty much ensures that you are not, in fact parallelising much of anything.

Share this post


Link to post
Share on other sites

having that singular mutex pretty much ensures that you are not, in fact parallelising much of anything


It would all depend on how much contention there is for the doit function in real code. The threads could be doing a billion other things not controlled by that (or any) lock.

Share this post


Link to post
Share on other sites


It would all depend on how much contention there is for the doit function in real code. The threads could be doing a billion other things not controlled by that (or any) lock.

Yes, but the point is that the caller won't know whether or not anyone else is calling (because they can't control who has a reference to the mutex). And even beyond that, your minimum unit of parallelisation is now 1.

 

If you really have a solitary resource (and very few resources are actually singletons in practice), then a more nuanced approach would be to provide a queue or requests, and a worker thread executing on the queue, so that at least you can let N threads past the call site (where N is the maximum length of the queue).

Share this post


Link to post
Share on other sites

And ideally, you pass the mutex as a parameter.

 

Statics/globals are the enemy of multi-threading, and having that singular mutex pretty much ensures that you are not, in fact parallelising much of anything.

Or the better option would be for this doit function to be a member function instead, and the mutex is a member variable.

That way the caller can't pass in the wrong mutex.

Share this post


Link to post
Share on other sites

Or the better option would be for this doit function to be a member function instead, and the mutex is a member variable.
That way the caller can't pass in the wrong mutex.

(we're pretty far off-topic at this point, and I should probably create a new thread, but with apologies to the OP...)

You've now gone the classic OOP route, and encapsulated the mutex such that the caller can never know who else has access to it. This might be fine for an inner class, or a class which is used by a single consumer. For any shared object though, this is about as bad as the global mutex.

Data is important, people. Always know where your data is, who has access to it, what operations they are performing on it. Hiding it all away inside a shared object is like sticking your head in the sand.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement