4

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.

halfer
  • 19,824
  • 17
  • 99
  • 186
Hemant Bhargava
  • 3,251
  • 4
  • 24
  • 45
  • 1
    Why do you have `read_lock` and `write_lock`? Can you please show how are they declared? – orhtej2 Sep 02 '18 at 17:58
  • @orhtej2, Edited the question. Sorry for not writing it earler. Cheers! – Hemant Bhargava Sep 02 '18 at 18:27
  • Also see this documentation - I think you can simplify your solution quite a bit: https://www.threadingbuildingblocks.org/docs/help/index.htm#reference/containers_overview/concurrent_hash_map_cls/concurrent_access.html – Will Bickford Sep 02 '18 at 18:59
  • @WillBickford, What kind of simplification you are suggesting? Overloading? I thought I have quite a bit of simplistic implementation. – Hemant Bhargava Sep 03 '18 at 05:14
  • The docs indicate that `insert` returns false if your key was already present, so you should be able to eliminate the find altogether, since you always insert if it wasn't present. – Will Bickford Sep 03 '18 at 15:27
  • @WillBickford, What about the reading part? I read from the pointer if it is already there. If it is present in the map, insert will return me false. Right? Then how am I supposed to read the pair's second value from it? – Hemant Bhargava Sep 04 '18 at 05:17

1 Answers1

4

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.

Anton
  • 6,349
  • 1
  • 25
  • 53