Threading with shared variables

Question

I tried to use multiple threads to insert into a boost::bimap. I have some shared variable between the threads, which I need to pass by reference and some of them are modified by each thread execution. However, I get the error:

Segmentation fault (core dumped)

I have the following code with me. I have tried to avoid concurrent access to the variables by using std::lock_guard<std::mutex> lock(mtx), but not able to make it work.

parallel_index.cpp

#include <iostream>
#include <string>
#include <algorithm>
#include <thread>
#include <mutex>
#include <boost/bimap.hpp>
#include <boost/bimap/unordered_set_of.hpp>
#include <boost/bimap/unordered_multiset_of.hpp>
#include "parallel_index.h"


namespace bimaps = boost::bimaps;
typedef boost::bimap<bimaps::unordered_set_of<uint64_t>,
        bimaps::unordered_multiset_of<std::string> > bimap_reference;
typedef bimap_reference::value_type position;
bimap_reference reference_index_vector;


size_t total_threads = std::thread::hardware_concurrency();

std::string sequence_content = "ABCDDBACDDDCBBAAACBDAADCBDAAADCBDADADACBDDCBBBCDCBCDAADCBBCDAAADCBDA";
uint64_t sequence_length = sequence_content.length();
int split = 5;
uint64_t erase_length = 0;
unsigned int seq_itr = 0;

std::mutex mtx;   // to protect against concurent access
int main(){
    thread_test::create_index index;
    std::thread threads[total_threads];
    std::cout << total_threads <<  " threads lanched" << std::endl;
    for(unsigned int i = 0; i < total_threads; i++){
        threads[i] = std::thread(&thread_test::create_index::reference_index_hash, index,
                std::ref(sequence_length), std::ref(split), std::ref(sequence_content), std::ref(erase_length));
    }

    for(unsigned int i = 0; i < total_threads; i++){
        threads[i].join();
    }
}



/*
 * Creating index
 */
void thread_test::create_index::reference_index_hash(uint64_t &sequence_length, int &split,
        std::string &sequence_content, uint64_t &erase_length  ){

    for (; seq_itr < sequence_length; ++seq_itr ){

        std::lock_guard<std::mutex> lock(mtx);    
        std::string splitstr = sequence_content.substr(erase_length, split);
        reference_index_vector.insert(position(seq_itr, splitstr));
        seq_itr += split-1;
        erase_length += split;

        if(erase_length > 10000){
            sequence_content.erase(0,erase_length);
            erase_length = 0;
        }
    }


    for( bimap_reference::const_iterator iter = reference_index_vector.begin(), iend = reference_index_vector.end();
            iter != iend; ++iter ) {
        std::cout << iter->left << " <--> "<< iter->right <<std::endl;
    }


}

parallel_index.h

#ifndef PARALLEL_INDEX_H_
#define PARALLEL_INDEX_H_


#include<iostream>
#include <algorithm>
#include <utility>
#include <boost/bimap.hpp>
#include <boost/bimap/unordered_set_of.hpp>
#include <boost/bimap/unordered_multiset_of.hpp>




//typedef boost::unordered_map<int, std::pair<int, unsigned long int>& > reference_map;

namespace bimaps = boost::bimaps;

typedef boost::bimap<bimaps::unordered_set_of<uint64_t>,
        bimaps::unordered_multiset_of<std::string > > bimap_reference;
typedef bimap_reference::value_type position;
extern bimap_reference reference_index_vector;


namespace thread_test{

class create_index{
public:
    void reference_index_hash(uint64_t &sequence_length, int &split,
            std::string &sequence_content, uint64_t &erase_length);
};
}


#endif /* PARALLEL_INDEX_H_ */

-------------------------------EDIT---------------------------------

I tried to divide the string content into partitions equal to the number of threads to have each thread its part locally available. But nothing seems to work. Some times it finishes the first thread and stops there after with a Segmentation fault (core dumped).

parallel_index.cpp

#include <iostream>
#include <string>
#include <algorithm>
#include <thread>
#include <mutex>
#include <boost/bimap.hpp>
#include <boost/bimap/unordered_set_of.hpp>
#include <boost/bimap/unordered_multiset_of.hpp>
#include "parallel_index.h"


namespace bimaps = boost::bimaps;
typedef boost::bimap<bimaps::unordered_set_of<uint64_t>,
        bimaps::unordered_multiset_of<std::string> > bimap_reference;
typedef bimap_reference::value_type position;
bimap_reference reference_index_vector;


//create threads
size_t total_threads = std::thread::hardware_concurrency();


std::string sequence_content = "ABCDDBACDDDCBBAAACBDAADCBDAAADCBDADADACBDDCBBBCDCBCDAADCBBCDAAADCBDADDCCCAAABBBAAACDCA";
uint64_t sequence_length = sequence_content.length();
int split = 5;

// split the sequence_content equal to the number of threads, and assign each partition to each thread.
uint64_t each_partition_len = sequence_content.length()/total_threads- (sequence_content.length()/total_threads)%split ;
uint64_t last_partition_len = sequence_content.length()/total_threads +
        (((sequence_content.length()/total_threads)%split)*(total_threads-1)) + sequence_content.length()%total_threads;


std::mutex mtx;   // to protect against concurent access
int main(){
    thread_test::create_index index;
    std::thread threads[total_threads];

    std::cout << total_threads <<  " threads lanched" << std::endl;

    for(unsigned int i = 0; i < total_threads; i++){

        if(i < total_threads-1)
            threads[i] = std::thread(&thread_test::create_index::reference_index_hash, index,
                            std::ref(each_partition_len), std::ref(split), std::ref(sequence_content), i);

        else
            threads[i] = std::thread(&thread_test::create_index::reference_index_hash, index,
                                        std::ref(last_partition_len), std::ref(split), std::ref(sequence_content), i);

        //std::lock_guard<std::mutex> lck(mtx);
        std::cout << "launched thread " << i << "with id " << threads[i].get_id() << std::endl;
    }


    for( bimap_reference::const_iterator iter = reference_index_vector.begin(), iend = reference_index_vector.end();
            iter != iend; ++iter ) {
        std::cout << iter->left << " <--> "<< iter->right <<std::endl;
    }

    for( unsigned int i = 0; i < total_threads; ++i){
        if(threads[i].joinable()){
            std::cout << "trying to join thread " << i << std:: endl;
            threads[i].join();
            std::cout << "thread joined " << i << std:: endl;
        }
    }

    for( bimap_reference::const_iterator iter = reference_index_vector.begin(), iend = reference_index_vector.end();
            iter != iend; ++iter ) {
        std::cout << iter->left << " <--> "<< iter->right <<std::endl;
    }
}





/*
 * Creating index
 */
void thread_test::create_index::reference_index_hash(uint64_t &sequence_length, int &split,
        std::string &sequence_content, int i  ){

    uint64_t seq_strt = 0;

//  set seq_strt
    if(i == 0)
        seq_strt = sequence_length * i;
    else
        seq_strt = sequence_length * i + 1;

    for (uint64_t seq_itr = seq_strt; seq_itr <= sequence_length; ++seq_itr ){


        std::string splitstr = sequence_content.substr(seq_itr, split);     

        mtx.lock();
        //std::lock_guard<std::mutex> lock(mtx);
        reference_index_vector.insert(position(seq_itr, splitstr));
        mtx.unlock();

        seq_itr += split-1;     

    }
}

parallel_index.h

#ifndef PARALLEL_INDEX_H_
#define PARALLEL_INDEX_H_


#include<iostream>
#include <algorithm>
#include <utility>
#include <boost/bimap.hpp>
#include <boost/bimap/unordered_set_of.hpp>
#include <boost/bimap/unordered_multiset_of.hpp>

namespace bimaps = boost::bimaps;

typedef boost::bimap<bimaps::unordered_set_of<uint64_t>,
        bimaps::unordered_multiset_of<std::string > > bimap_reference;
typedef bimap_reference::value_type position;
extern bimap_reference reference_index_vector;


namespace thread_test{

class create_index{
public:
    void reference_index_hash(uint64_t &sequence_length, int &split,
            std::string &sequence_content, int i);
};
}


#endif /* PARALLEL_INDEX_H_ */

I suspect you'll get better performance not locking at the start of each loop and holding for the duration of the loop; but just locking outside the loop and releasing after; it seems that you're only allowing 1 thread to run at a time in the expensive part anyway; so why bother with threads? — UKMonkey, Feb 28 '18 at 16:20
I want only one entry into the `bimap` at a time, so I used lock after the start of the loop. There may be some other flaws also because I am new to threads. — AwaitedOne, Feb 28 '18 at 16:26
Declare seq_itr as local variable inside reference_index_hash() instead of global ? — seccpur, Feb 28 '18 at 16:52
@engf-010 Why you think the code will not compile?. That is the class object by pointer or reference `(index or &index or std::ref(index))` — AwaitedOne, Feb 28 '18 at 17:00
@AwaitedOne Do you have static or dynamic linking of the libraries. — Agaz Wani, Mar 05 '18 at 15:41

score 1 · Accepted Answer · answered Mar 05 '18 at 15:50

I feel the culprit for segmentation fault is nothing but static linking of the libraries. Its not by incrementing seq_itr to a value bigger than the actual sequence length, because your for loop will never allow to enter if seq_itr is greater than actual sequence length. You try by removing the -static flag and it should work by not giving the segmentation fault, however it does not guarantee the correctness of the other code. More details about segmentation fault with thread can be found here

Thanks for pointing to `static` linking. It runs without segmentation fault now. I will check for the correctness of other code. — AwaitedOne, Mar 05 '18 at 16:03

seccpur · Answer 2 · 2018-02-28T17:34:33.063

0

All the threads will try to get the lock in the critical section, to keep the bitmap intact, you need a conditional variable so that threads will orderly get executed. This is justified as you are using seq_itr as local variable inside reference_index_hash() and it needs to be incremented in proper sequence.

edited Feb 28 '18 at 17:34

answered Feb 28 '18 at 17:23

seccpur

4,996
2
13
21

Thank you for your comment. I tried using local variables but not working. I have edited that above. – AwaitedOne Mar 02 '18 at 14:43

score 0 · Answer 3 · answered Mar 04 '18 at 05:42

One problem in your original code is that unsigned int seq_itr is accessed without synchronization from multiple threads. Besides yielding invalid results it might lead to seq_itr being incremented to a value bigger than the actual sequence length, and the following accesses might lead to a crash.

The new code addresses this by just passing indexes, which should be OK as long as those indexes are non-overlapping and correctly calculated. I can't follow the logic completely, but in case your seq_strt calculation is off the program might also crash due to an invalid index. Should be easy to verify in a debugger or with some index assertions.

However there is an issue in the second code example with printing the map directly after threads are started with

for( bimap_reference::const_iterator iter = reference_index_vector.begin(), iend = reference_index_vector.end();
        iter != iend; ++iter ) {
    std::cout << iter->left << " <--> "<< iter->right <<std::endl;
}

This will not yield correct results, since the map is concurrently accessed by all worker threads. Access after the join()s is safe.

Threading with shared variables

3 Answers3