Is std::ref really necessary when passing references to threads?

Question

I'm reading through C++ Concurrency in Action and in Chapter 2 I'm led to believe that even if a function prototype, e.g:

void MagicFunc(Data& myData);

is intended to be used like the following:

Data dataExample;
thread t(MagicFunc,dataExample);

I should really do this

Data dataExample
thread t(MagicFunc,std::ref(dataExample));

or otherwise the changes I expect to have happened to "dataExample" won't have take place. Specifically it states something like:

Although MagicFunc expects the second parameter to be passed by reference, the std::thread constructor t doesn’t know that; it’s oblivious to the types of the arguments expected by the function and blindly copies the supplied values. When it calls Magicfunc, it will end up passing a reference to the internal copy of data and not a reference to data itself. Consequently, when the thread finishes, these updates will be discarded as the internal copies of the supplied arguments are destroyed, and process_widget_data will be passed an unchanged Data myData rather than a correctly updated version.

However, testing this out with the following program

#include <iostream>
#include <thread>
#include <vector>
#include <chrono>
#include <assert.h>
using namespace std;
using namespace std::chrono;

const int NUM_VALS = 50000000;

#define _MULTICORE 

void AddValuesToSlots(vector<int>& vecVals,vector<int>::iterator& begin,
                      int num,int startNum){
    int i = startNum;
    auto end = begin + num;
    for (auto itr = begin; itr < end; ++itr){
        *itr = i++;
    }
}

int main()
{
    vector<int> vecVals;
    vecVals.resize(NUM_VALS);

    //get number of cores and divide up the workload
    unsigned int numCores = thread::hardware_concurrency();
    unsigned int slotsPerThread = NUM_VALS / numCores;

    //for timing
    high_resolution_clock::time_point t1 = high_resolution_clock::now();


    thread* t = new thread[numCores];

    //get the iterator to the beginning
    auto begin = vecVals.begin();

#ifdef _MULTICORE
    for (int core = 0; core < numCores; ++core){
        t[core] = thread(AddValuesToSlots, vecVals, begin + core*slotsPerThread,
            slotsPerThread, core*slotsPerThread);
    }

    for (int core = 0; core < numCores; ++core){
        t[core].join();
    }
#else
    AddValuesToSlots(vecVals, begin, NUM_VALS, 0);
#endif


    delete[] t;

    //how long did it take?
    high_resolution_clock::time_point t2 = high_resolution_clock::now();
    cout << duration_cast<milliseconds>(t2-t1).count() << endl;

#ifdef _DEBUG
    //test that the values are correct
    for (int slot = 0; slot < NUM_VALS; ++slot)
        assert(vecVals[slot] == slot);
#endif

    return 0;
}

I've tried encasing vecVals in a std::ref and without, both times it executes without problem. Is the std::ref then really necessary and the information provided erroneous?

Thanks

Could you please remove all the irrelevant code (most of what you posted) and post an MCVE? — juanchopanza, Mar 25 '15 at 15:17

score 5 · Accepted Answer · answered Mar 25 '15 at 15:19

5

You're not altering vecVals directly. Iterators are working because copying iterators is ok, it still points to the same memory address

answered Mar 25 '15 at 15:19

Severin Pappadeux

18,636
3
38
64

It seems so obvious now. Brilliant. Thank you! – Lloyd Crawley Mar 25 '15 at 15:45

score 4 · Answer 2 · edited May 23 '17 at 11:52

4

The code you posted is actually illegal under the standard. std::thread should be calling AddValuesToSlots with rvalue copies of your arguments.

Some C++ compilers get this wrong, and instead call it with lvalue copies of your arguments.

live example

An easy way to test if your compiler is breaking the rules is:

void func1(int&&) { std::cout << "func1\n"; }
void func2(int&) { std::cout << "func1\n"; }

int main() {
  int x;
  std::thread t1(func1, x);
  t1.join();
  std::thread t2(func2, x);
  t2.join();
}

if t1 ctor is accepted and t2 rejected, your compiler is compliant.

If t2 ctor is accepted and t1 rejected, your compiler has a violation of the standard.

See here for more about this MSVC compiler bug

edited May 23 '17 at 11:52

Community

1
1

answered Mar 25 '15 at 19:38

Yakk - Adam Nevraumont

262,606
27
330
524

t2 is accepted and t1 is in fact rejected: Error 1 error C2664: 'void (int &&)' : cannot convert argument 1 from 'int' to 'int &&' So it looks like Visual Studio is guilty on both counts - for my "illegal" code and also for the example you posted. Are you really sure? – Lloyd Crawley Mar 25 '15 at 20:19
@LloydCrawley both are rejected? That is surprising! What compiler and version and flags? – Yakk - Adam Nevraumont Mar 25 '15 at 20:21
Sorry, I submitted too early by accident, edited ^ BTW Visual Studio Community 2013 vers. 12.0.31101.00 update 4 – Lloyd Crawley Mar 25 '15 at 20:22
@LloydCrawley I knew MSVC got it backwards: I was surprised that there was a compiler that rejected *both*! The `std` C++11 library in MSVC has a number of such issues, and as it is (as far as I know) closed source, they are harder to fix than in open source libraries. It is a subtle quirk in the implementation: basically because you are only going to call the function *once*, moving the contents of your stored pack of arguments into the call is the right thing to do (and, as a bonus, demanded by the standard). The demand is a bit obtuse, I will admit. – Yakk - Adam Nevraumont Mar 25 '15 at 20:26
@LloydCrawley http://stackoverflow.com/a/28330984/1774667 for more discussion of this bug – Yakk - Adam Nevraumont Mar 26 '15 at 16:16

Is std::ref really necessary when passing references to threads?

2 Answers2