Concurrent write to different buckets in unordered_map (C++)?

Question

C++ newbie here. I'm trying to write to different buckets concurrently in an unordered_map. From what I can tell by searching, it is my understanding that this should be a thread safe operation. My (perhaps incorrect) understanding is based on the answers here and here, as well as the referenced portion of the C++11 standard (particularly item 2 -- emphasis mine):

23.2.2 Container data races [container.requirements.dataraces]

1 For purposes of avoiding data races (17.6.5.9), implementations shall consider the following functions to be const: begin, end, rbegin, rend, front, back, data, find, lower_bound, upper_bound, equal_range, at and, except in associative or unordered associative containers, operator[].

2 Notwithstanding (17.6.5.9), implementations are required to avoid data races when the contents of the contained object in different elements in the same sequence, excepting vector<bool>, are modified concurrently.

3 [ Note: For a vector x with a size greater than one, x[1] = 5 and *x.begin() = 10 can be executed concurrently without a data race, but x[0] = 5 and *x.begin() = 10 executed concurrently may result in a data race. As an exception to the general rule, for a vector < bool > y, y[0] = true may race with y[1] = true. —end note ]

In any case, it seems that writing to different buckets is not thread safe with the standard containers, as demonstrated by the code below. You'll see that I enable a lock corresponding to the bucket being modified before writing, yet sometimes pairs do not get recorded correctly. For what it's worth, if I use a single lock -- e.g., just change auto bkt = mm->bucket(key); to auto bkt=0;, effectively locking the entire unordered_map container -- everything works as expected.

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

#define NUM_LOCKS 409
#define N 100
#define NUM_THREADS 2

using namespace std;


class SpinLock
{
    public:
        void lock()
        {
            while(lck.test_and_set(memory_order_acquire)){}
        }
    void unlock()
        {
            lck.clear(memory_order_release);
        }

    private:
        atomic_flag lck = ATOMIC_FLAG_INIT;
};


vector<SpinLock> spinLocks(NUM_LOCKS);


void add_to_map(unordered_map<int,int> * mm, const int keyStart, const int keyEnd, const int tid){

    for(int key=keyStart;key<keyEnd;++key){
        auto bkt = mm->bucket(key);

        //lock bucket
        spinLocks[bkt].lock();

        //insert pair
        mm->insert({key,tid});

        //unlock bucket
        spinLocks[bkt].unlock();
    }

}


int main() {

    int Nbefore, Nafter;
    thread *t = new thread[NUM_THREADS];

    //create an unordered map, and reserve enough space to avoid a rehash
    unordered_map<int,int> my_map;
    my_map.reserve(2*NUM_THREADS*N);

    //count number of buckets to make sure that a rehash didn't occur
    Nbefore=my_map.bucket_count();


    // Launch NUM_THREADS threads.  Thread k adds keys k*N through (k+1)*N-1 to the hash table, all with associated value = k.

    for(int threadID=0;threadID<NUM_THREADS;++threadID){
        t[threadID]=thread(add_to_map,&my_map,threadID*N,(threadID+1)*N,threadID);
    }

    // Wait for the threads to finish
    for(int threadID=0;threadID<NUM_THREADS;++threadID){
        t[threadID].join();
    }

    //count number of buckets to make sure that a rehash didn't occur
    Nafter=my_map.bucket_count();


    cout << "Number of buckets before adding elements: " << Nbefore <<endl;
    cout << "Number of buckets after  adding elements: " << Nafter  << " <--- same as above, so rehash didn't occur" <<endl;

    //see if any keys are missing
    for(int key=0;key<NUM_THREADS*N;++key){

        if(!my_map.count(key)){

            cout << "key " << key << " not found!" << endl;

        }
    }

    return 0;
}

The program will exit when a key was erroneously not entered. A sample output is:

Number of buckets before adding elements: 401
Number of buckets after  adding elements: 401 <--- same as above, so rehash didn't occur
key 0 not found!
key 91 not found!
key 96 not found!
key 97 not found!
key 101 not found!
key 192 not found!
key 193 not found!
key 195 not found!

So, my question is two-fold:

1. Am I doing something wrong in how I am locking the buckets?
2. If so, is there a better way to lock the map on a bucket-by-bucket basis to enable concurrent writes to different buckets?

Finally, I'll mention that I already tried TBB's concurrent_unordered_map, but it was much slower in my application than simply doing things in serial. The stray errors aside, my bucket-locking approach using std::unordered_map performed significantly better.

Answer 1

The elements of a container are not the buckets, but rather the value_type elements.

Modifying one element in a std container has no concurrency impact on other elements. But modifying one bucket has no such guarantee.

Adding or removing elements in a bucket is a non-const operation on the container that is not from the special list of non-const operations that are safe to use without synchronization.