How to remove / reduce too many repetitive InterlockedCompareExchange?

Question

I've learned that InterlockedCompareExchange() is used to read interlocked variables.

Also, InterlockedCompareExchange() is preferable rather than InterlockedOr() by Raymond Chen from the comments of reference:

Reading interlocked variables

interlocked reading a 64-bit variable

Now, I am facing too many InterlockedCompareExchange() function calls in my thread code, and this doesn't seem right.

For instance, below is simple thread code that uses GetAsyncKeyState() and stores the key event to a queue (Container::CQueue is a custom circular queue for academic purpose). GetAsyncKeyState() and storing the key event is just an example. So please don't focus on why I'm not using a Window message queue and WndProc().

unsigned long __stdcall Thread(void* const _pParameter)
{
    Container::CQueue<EKeyEvent>* pKeyEventQueue = nullptr;

    bool bMouseLeftDown = false;

    while (true)
    {
        // Check Exit Loop Interlocked Variable
        if (InterlockedCompareExchange16(&m_sThreadExit, 0i16, 0i16) != 0i16)
        {
            break;
        }

        if (GetAsyncKeyState(VK_LBUTTON) & 0x8000i16)
        {
            if (bMouseLeftDown == false)
            {
                bMouseLeftDown = true;

                if (static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr)) == nullptr)
                {
                    break;
                }

                if (static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr))->Enqueue(EKeyEvent::MouseLeft_Down) != Container::EQueueResult::Success)
                {
                    break;
                }
            }
        }
        else
        {
            if (bMouseLeftDown)
            {
                bMouseLeftDown = false;

                if (static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr)) == nullptr)
                {
                    break;
                }

                if (static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr))->Enqueue(EKeyEvent::MouseLeft_Up) != Container::EQueueResult::Success)
                {
                    break;
                }
            }
        }
    }

    return 0UL;
}

You may have noticed that this keeps coming up and it deteriorates readability:

static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr))

Q1. If there is a pointer type shared resource, is it right to use the return value of InterlockedCompareExchangePointer()?

For ex, use = InterlockedCompareExchangePointer(&InterlockedPointer, nullptr, nullptr);

I thought so because directly accessing InterlockedCompareExchangePointer() gives a C28112 warning. Many people told me that it is a false positive, though.

Q2. If I need to get a pointer type interlocked resource and do some operation with it, how can it be done safely and cleanly?

if (static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr)) == nullptr)
{
    break;
}

if (static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr))->Enqueue(EKeyEvent::MouseLeft_Down) != Container::EQueueResult::Success)
{
    break;
}

This doesn't seem safe, because if m_pCurrentKeyEventQueue became nullptr right after the null check, second static_cast<Container::CQueue<EKeyEvent>*>(InterlockedCompareExchangePointer(reinterpret_cast<void**>(&m_pCurrentKeyEventQueue), nullptr, nullptr)) will confront a nullptr exception.

Is some other thread assigning new values to `m_pCurrentKeyEventQueue` while this code runs? You don't need `InterlockedCompareExchangePointer` to merely read the value, unless there are reasons to believe that someone is writing to it concurrently. In which case you have bigger problems - e.g. you 1) read the variable, 2) check it for null, 3) read it again, and finally 4) call a method on it without checking. Another thread could set the variable to null between 2 and 3. — Igor Tandetnik, Sep 26 '21 at 15:02
If you nevertheless feel the need to use `InterlockedCompareExchangePointer`, call it once to read the value of `m_pCurrentKeyEventQueue` into a local variable, then use that local variable (without synchronization, since it's not shared). — Igor Tandetnik, Sep 26 '21 at 15:06
Thanks for the comment Igor Tandetnik. There is some other thread assigning new values to m_pCurrentKeyEventQueue. One other thread will swap the pointer m_pCurrentKeyEventQueue with other allocated pointer when and return the queue pointer. In other words, one thread keeps running and enqueues to the "Current" queue. Other thread swaps the "Current" queue with "Idle / Externally accessible" queue. — YoonSeok OH, Sep 26 '21 at 15:08
Well, the swap may very well occur between `InterlockedCompareExchangePointer` that reads the pointer, and `Enqueue` call that modifies the queue that pointer points to. So the queue you modify may no longer be "current". If that's not a problem, then you might as well grab the pointer once at the top of the loop, and save it in a local variable. — Igor Tandetnik, Sep 26 '21 at 15:13
"call it once to read the value of m_pCurrentKeyEventQueue into a local variable, then use that local variable (without synchronization, since it's not shared)" that is the way I thought of too. I wonder if it is safe from the situation Thread 0 : local pointer = InterlockedCompareExchangePointer(). Thread 1 : Swaps pointer. Thread 0 : enqueue by local pointer (which became externally accessible queue, which should not happen). — YoonSeok OH, Sep 26 '21 at 15:14
Like I said, that is already happening in your current code. If you need to prevent that, then you need a lock around the whole "obtain the pointer and enqueue to it" operation, and the same lock around "swap queues" operation, so the two don't interleave. — Igor Tandetnik, Sep 26 '21 at 15:16
"lock around the whole "obtain the pointer and enqueue to it" operation, and the same lock around "swap queues" operation" Ohh this is it! — YoonSeok OH, Sep 26 '21 at 15:20
not need interlocked for read at all. read must be atomic but not interlocked. interlocked need only for RMW operation (when you need inrelocked read and write to variable) — RbMm, Sep 26 '21 at 16:33

score 0 · Answer 1 · answered Sep 27 '21 at 16:44

I've learned that InterlockedCompareExchange() is used to read interlocked variables.

This trick is only needed if you want to read 64-bit value with 32-bit code or 128-bit value with 64-bit code.

Nowadays the first case can be addressed way more efficiently with __iso_volatile_load64 for recent MSVC or clang-cl compiler.

The rest of the cases (where the variable is pointer, pointer sized integer or smaller integer) can be addressed by just plain reads, assuming proper compiler barriers are in place. Or with __iso_volatile_loadXX, if the warning about non-interlocked access bothers you.

Or better yet just use C++11 std::atomic (or C++20 std::atomic_ref, if you insist on plain variable).

Note that the linked questions are quite old, that's why they don't discuss __iso_volatile_loadXX or std::atomic.

Using Interlocked... functions where just plain read is sufficient causes performance penalty.

How to remove / reduce too many repetitive InterlockedCompareExchange?

1 Answers1