How do we know that the code is thread safe?

Question

I am following this tutorial to understand how to remove locks , using VTUNE
This page says the following after collecting Vtune results:

Identify the Hottest Code Lines

Click the hotspot navigation button to go to the code line that took the most Wait time. VTune Amplifier highlights line 170 entering the critical section rgb_critical_section in the draw_task function. The draw_task function was waiting for almost 27 seconds while this code line was executing and most of the time the processor was underutilized. During this time, the critical section was contended 438 times.

The rgb_critical section is the place where the application is serializing. Each thread has to wait for the critical section to be available before it can proceed. Only one thread can be in the critical section at a time. You need to optimize the code to make it more concurrent.

I was able to follow this tutorial until I reach the next section: Remove the Lock

Remove the Lock

The rgb_critical_section was introduced to protect calculation from multithreaded access. The brief analysis shows that the code is thread safe and the critical section is not really needed.

My question is how do we know the code is thread safe?

As suggested I commented those line (EnterCritical... and LeaveCritical...), and saw huge performance gain, but I did not get why this critical section is not required? Which analysis tells us this ?

The relevant code is here in analyze_locks.cpp:

public:
    void operator () (const tbb::blocked_range <int> &r) const {

        unsigned int serial = 1;
        unsigned int mboxsize = sizeof(unsigned int)*(max_objectid() + 20);
        unsigned int * local_mbox = (unsigned int *) alloca(mboxsize);
        memset(local_mbox,0,mboxsize);

        for (int y=r.begin(); y!=r.end(); ++y) {
            drawing_area drawing(startx, totaly-y, stopx-startx, 1);

            // Enter Critical Section to protect pixel calculation from multithreaded access (Needed?)
        //  EnterCriticalSection(&rgb_critical_section);

            for (int x = startx; x < stopx; x++) {
                color_t c = render_one_pixel (x, y, local_mbox, serial, startx, stopx, starty, stopy);
                drawing.put_pixel(c);
            }

            // Exit from the critical section
        //  LeaveCriticalSection(&rgb_critical_section);

            if(!video->next_frame()) tbb::task::self().cancel_group_execution();
        }
    }

    draw_task () {}

};

Answer 1

Something is threadsafe if no global state (that is, state outside of this particular thread) is being modified. It's hard for us to tell what render_one_pixel and drawing.put_pixel actually does, and in what order it may need to be performed.

Assuming that the order of the calls to put_pixel(c) doesn't matter (or which thread makes the call), it would be safe to remove the critical section here. If there is strict ordering required, I'm not sure a critical section is the right solution anyway. (The same rules obviouysly apply to render_one_pixel, if it changes some global state, which would of course also have to be taken into account in the "is it safe").