0
\$\begingroup\$

I am looking for "complete" producer-consumer sample using C++11 memory barrier.

(I have derived following example from Jeff's article and added a line to make it complete.)

void SendTestMessage(void* param)
{
    // Copy to shared memory using non-atomic stores.
    g_payload.tick  = clock();
    g_payload.str   = "TestMessage";
    g_payload.param = param;

    // Release fence.
    std::atomic_thread_fence(std::memory_order_release);

    // Perform an atomic write to indicate that the message is ready.
    g_guard.store(1, std::memory_order_relaxed);
}

bool TryReceiveMessage(Message& result)
{
    // Perform an atomic read to check whether the message is ready.
    int ready = g_guard.load(std::memory_order_relaxed);

    if (ready != 0)
    {   
        g_guard.store(0, std::memory_order_relaxed);

        // Acquire fence.
        std::atomic_thread_fence(std::memory_order_acquire);

        // Yes. Copy from shared memory using non-atomic loads.
        result.tick  = g_payload.tick;
        result.str   = g_payload.str;
        result.param = g_payload.param;

        return true;
    }

    // No.
    return false;
}

if you notice, i have added g_guard.store(0, std::memory_order_relaxed); just before acquire.

This will help following ways

  1. It would avoid TryReciveMessage to cosnume same message if called multiple times before new message is written

  2. It would not add explicit memory fence and hence won't affect performance

  3. since std::memory_order_relaxed guarantees the ordering, it would get overwritten by SendTestMessage value if new load added after the std::memory_order_acquire is called. So, we will not miss any load.

Please provide your comments and/or suggestions.

\$\endgroup\$

closed as off-topic by Dannnno, Stephen Rauch, Billal Begueradj, Incomputable, Zeta Jun 29 '18 at 7:24

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Authorship of code: Since Code Review is a community where programmers improve their skills through peer review, we require that the code be posted by an author or maintainer of the code, that the code be embedded directly, and that the poster know why the code is written the way it is." – Stephen Rauch, Zeta
  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Dannnno, Billal Begueradj, Incomputable
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Can you provide some more context? Especially in multithreaded code it is very important for us to see how functions are expected to be called. Also, we have no idea what the types in g_payload actually are, \$\endgroup\$ – hoffmale Jun 27 '18 at 21:03
1
\$\begingroup\$

It's always difficult, and sometimes impossible, to review a snippet of code without knowing how it is intended to be used. In this case, I can say confidently that this code will do the job. Whether that happens to be the job you want it to do is another thing entirely.

As long as you understand the limitations of the pattern you're using (and assuming g_guard is initialized to 0 properly before either of the functions are called), this will "work".

The limitations are:

  • SendTestMessage() can only be called once. Not once-per-thread; once for the entire program.
  • TryReceiveMessage() can be called repeatedly... but only non-concurrently in a single thread.

You didn't say how you intend to use this code, but from your questions, I can deduce that this won't work for you.

  1. It would avoid TryReciveMessage to cosnume same message if called multiple times before new message is written

    With this code, it is safe to call SendTestMessage() EXACTLY ONCE, concurrently with TryReceiveMessage() any number of times IN A SINGLE THREAD. So TryReceiveMessage() will not consume the same message more than once... but it can also not consume more than one message in total. There can be no "new message".

    Safely synchronizing the guard does not safely synchronize the payload. This code "works" only because the payload is written ONCE, and then read, with the guard making sure the read is absolutely ordered after the write. If you want to write a second time (for a new message): you can't. You'd need a second guard to prevent the second write until after the read is finished. That would turn SendTestMessage() into a blocking call... and then you might as well just use regular synchronization techniques.

  2. It would not add explicit memory fence and hence won't affect performance

    "Performance" is always a vague term. There are many different ways for code to be performant.

    If your goals are:

    1. You want to minimize inter-thread synchronization costs.

    2. You don't care how long you have to wait to receive the message.

    Then this code is about as performant as it can be.

    Note that "performance" in this case doesn't mean "fast". After you call SendTestMessage(), you could end up waiting a looong time before TryReceiveMessage() realizes that there's a message ready. If you have other things to do in the receive thread, that's fine I guess. But if all you're doing is a busy-wait spin... this is not the best design for this.

  3. since std::memory_order_relaxed guarantees the ordering, it would get overwritten by SendTestMessage value if new load added after the std::memory_order_acquire is called. So, we will not miss any load.

    I assume there's a typo here, and you mean to say: "since std::memory_order_ release guarantees the ordering...." Because std::memory_order_relaxed does literally the opposite of guaranteeing ordering.

    But that's just a guess, because I can't make any sense of what you said. I can't even figure out what "it" is that you're concerned about being overwritten: Do you mean g_guard or g_payload? Are you asking about if you try to call SendTestMessage() twice? I can't figure out what you're asking.

Is this the best way to do this? That depends on what "this" is; you didn't say. It's a passable demonstration on how to use thread fences... but it doesn't look like anything that would be useful in a real-world program generally. This setup works for sending exactly one message in the entire lifetime of the program, and receiving it exactly once in a single thread. That's not very useful, generally.

If what you want is a way to set up a producer-consumer operation between threads of any kind - multi-consumer, multi-producer, whatever - there are much better ways to do it, generally.

Relaxed memory ordering is one of the deepest and darkest corners of C++, and you really shouldn't be screwing around with it unless you're an expert. Unless and until you've done everything else possible to speed up your code in a bottleneck, and have confirmed that it is inter-thread synchronization holding you up, you shouldn't even consider touching relaxed memory ordering. And even then, you shouldn't.

I'd say the bottom line of this review is: whatever you're planning to do with this code... don't.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.