Concurrent_hash_map implementation throwing SIGSEGV

user_2037570 0
Viewed 157

I am trying to use tbb’s concurrent_hash_map to increase my application’s concurrent performance. Read up about it and implemented it according to my application but I am seeing crashes..

So, my application is a multi-threadedd application where I am storing pairs,the key is char* and the value is an integer. Pseudocode looks like this:

In .h file,

typedef tbb::concurrent_hash_map<const void*, unsigned, Hasher> tbb_concurrent_hash;
tbb_concurrent_hash concurrent_hash_table;
tbb_concurrent_hash::accessor  write_lock;
tbb_concurrent_hash::const_accessor read_lock;

In .c file,

void  storeName(const char * name)   {
    int id=0;

    // This creates a pair object of name and index
    std::pair object(name, 0);

    // read_lock is a const accessor for reading. This find function searches for char* in the table and if not found, create a write_lock.

    bool found = concurrent_hash_table.find(read_lock, name);  
    if (found == FALSE) {
      concurrent_hash_table.insert(write_lock, name);
      // Get integer id somehow.
      id = somefunction();
      write_lock->second = id; 
      write_lock.release();         
    } else {
      // if the name is found in the table then get the value and release it later
      id = read_lock->second;
      read_lock.release();
    }
}

As per my understanding, I am good with the implementation but as I said, there are multiple crashes coming when find returns me FALSE. Crash have traces of mutexs as well.

tbb hashmap concurrency multithreading c++
1 Answers

Your 'locks', i.e. accessors are declared global in .h file. so, basically you write to a shared scoped_lock instance... which logically leads to a data race. Accessors are like fused std::shared_ptr and std::scoped_lock classes in one, or simpler - a pointer for your result and a lock guard for the data it points. You don't want to use one global pointer from multiple threads. Declare them locally in a scope you want to have that access (and you'd not need to call .release() as well)

Another problem is the data race between find() and insert (). Two or more threads can decide that they have to insert since they found nothing. In this case, the first thread will insert the new element while other threads will return existing element because insert() acts as find() if there's existing element. The problem is that your code doesn't account for that.

I can see why you might want to double check using const_accessor as the read lock is more scalable. But instead, you might want to use bool insert( const_accessor& result, const value_type& value ); with read lock (const_accessor) and value_type instead of a key only, which will initialize the whole pair in the case when a new element is added.

user_25277970
Related