Avoid race condition performance

Question

I have two asynchronous threads (explicitly asked to do it that way) running 2 different methods from an object, move_robot() and get_area(). The first method moves a vehicle through different positions and the second one calculates the area covered thus far. Both share two object attributes:

vector<double> position; // saves current position of the vehicle
bool moving; // true if the vehicle has not reach the end of the path

In order to avoid race conditions I am using locks the following way:

   void get_area() {
        std::unique_lock<std::mutex> lck(mtx); // get mutex
        static vector<double> previous_measure = this->position; // initialized with the first position the robot is in
        lck.unlock();
        while (this->moving) {
            lck.lock();
            auto auxiliar = this->position;
            lck.unlock();
            this->area_covered += distance(auxiliar , previous_measure) * this->width;
            previous_measure = this->position; // save the new position for the next iteration
            this_thread::sleep_for(chrono::milliseconds(10)); // sleep for 10 ms
            std::cout << "Area:" << this->area_covered << " m²" << std::endl;
        }
    }

    void next_step() {
        std::unique_lock<std::mutex> lck(mtx); // get mutex
        this->position[0] += DT * this->direction[0];
        this->position[1] += DT * this->direction[1];

    }

    void move_robot(const vector<vector<double> > &map) {
    // accumulate the footprint while the robot moves
    // Iterate through the path
        for (unsigned int i=1; i < map.size(); i++) {
            while (distance(position , map[i]) > DISTANCE_TOLERANCE ) {
                this->direction = unitary_vector(map[i], this->position);
                this->next_step();
                this_thread::sleep_for(chrono::milliseconds(10)); // sleep for 10 ms
            }
        }
        this->moving = false; // notify get area to leave
    }

In the get_area() method I am locking to save the this->position attribute in a variable and have the same one over the whole iteration and to initialize the first previous_measure. In next_step() is used when this->position changes according the dynamics of the vehicle.

Moreover, how would you lock the moving attribute inside the while condition? And how to avoid this situation: I execute get_area(), the robot moves and ends the path and I leave the get_area method without doing the last iteration.

My question is whether it would be possible to optimize this locking and unlocking, as it is done pretty often (I do not want to set a producer-client structure because I want get_area() to run independent).

---

EDIT:

I have changed the condition in the while to:

bool is_moving() {
    std::unique_lock<std::mutex> lck(mtx);
    return moving;
}



void get_area() {
    std::unique_lock<std::mutex> lck(mtx);
    static vector<double> previous_measure = this->position; 
    lck.unlock();
    while (this->is_moving()) {...}
}

But do not know if there is something even cleaner

Answer 1

Optimization #1: assuming the speed of the robot is slow compared to the speed of computation

Considering the speed of the robot, there is no real need to lock both the position and moving since the rate of change of this values, compared to the rate of computation will be enormously small, so the margin of error due to stale read will be negligible.

In any case the above code is doing exactly that, since the previous_measure = this->position is reading the position while it's unlocked, therefore there is small chance that previous_measure != auxiliar at the end of the loop.

To assure that a final read is executed (meaning get_area is computed again after moving is false), we can repeat the calculation once more after leaving the loop, since then the robot had stoped moving and the position will never change.

Optimization #2: Consumer-Producer communication trough Ring Buffer

In case the speed of change of the variables is close to the speed of computiation, the optimization #1 will obviously create significant error. In such case, you can consider using ring buffer to move the values from move_robot to get_area without locking. If you have only one consumer and only one producer thread, there is no need to lock. Here is an example of implementation of ring buffer (or circular buffer).

Answer 2

In the first example you're reading this->moving while not holding the lock. This will lead to UB if another thread is also writing it. You can reorder the locking a bit to make it correct, for example like this:

   void get_area() {
        std::unique_lock<std::mutex> lck(mtx); // get mutex
        static vector<double> previous_measure = this->position; // initialized with the first position the robot is in
        while (this->moving) {
            auto auxiliar = this->position;
            this->area_covered += distance(auxiliar , previous_measure) * this->width;
            previous_measure = this->position; // save the new position for the next iteration
            lck.unlock();
            this_thread::sleep_for(chrono::milliseconds(10)); // sleep for 10 ms
            std::cout << "Area:" << this->area_covered << " m²" << std::endl;
            lck.lock();
        }
    }

Also, move_robot must also hold the lock while updating shared values. Just make sure to not hold the lock while sleeping.

    void move_robot(const vector<vector<double> > &map) {
        std::unique_lock<std::mutex> lck(mtx);
        for (unsigned int i=1; i < map.size(); i++) {
            while (distance(position , map[i]) > DISTANCE_TOLERANCE ) {
                this->direction = unitary_vector(map[i], this->position);
                this->next_step();
                lck.unlock();
                this_thread::sleep_for(chrono::milliseconds(10)); // sleep for 10 ms
                lck.lock();
            }
        }
        this->moving = false; // notify get area to leave
    }

Remember, if writing is involved, then all parallel access to a variable must either be protected by a mutex or the variable must be made atomic.