3
\$\begingroup\$

I have an unordered_map<int, std::shared_ptr<foo>> into which I want to lazily create the instances of foo based on callbacks. These callbacks would come from potentially a pool of threads, so to handle this, at the start I create the map and insert empty shared_ptr<>s into the map for all possible indexes. Now I have the code below which is designed to create the foo instance based on demand.

Is my implementation of double checked locking with shared_ptr<> correct?

void Publish(const bar& data)  {
    auto id = data.get_id();
    auto it = foos_.find(id);
    if (it == foos_.end()) {
      // throw
    }
    // Check if this is present
    auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
    std::atomic_thread_fence(std::memory_order_acquire);
    if (!sp) {
      // Create the instance of the foo for this id
      std::lock_guard<std::mutex> lock(mutex_);
      (void) lock;
      // Check again
      sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
      if (!sp) {
        // Create new foo
        sp = std::make_shared<foo>(...);
        std::atomic_thread_fence(std::memory_order_release);
        std::atomic_store_explicit(&(it->second), sp, std::memory_order_relaxed);
      } // Someone else created it...
    }

    // Push the data in
    sp->Publish(data);
  }
\$\endgroup\$
  • \$\begingroup\$ What's the purpose of (void) lock;? \$\endgroup\$ – πάντα ῥεῖ Jan 17 '17 at 10:33
  • \$\begingroup\$ @πάνταῥεῖ to avoid the unused warning on the lock guard variable. It's only use is to be destroyed at the end of scope. \$\endgroup\$ – ratchet freak Jan 17 '17 at 10:34
  • \$\begingroup\$ @ratchetfreak Would be worth a comment explaining so IMO. \$\endgroup\$ – πάντα ῥεῖ Jan 17 '17 at 10:35
  • \$\begingroup\$ @πάνταῥεῖ, funny you should ask: stackoverflow.com/questions/6145548/… \$\endgroup\$ – Nim Jan 17 '17 at 10:43
3
\$\begingroup\$

Your implementation of Double Checked Locking is semantically correct; the acquire and release fences provide minimal necessary ordering.

I am wondering why you are using standalone fences, which is almost never necessary.. It is more common to use memory ordering directly on atomic operations:

auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_relaxed);
std::atomic_thread_fence(std::memory_order_acquire);

...

std::atomic_thread_fence(std::memory_order_release);
std::atomic_store_explicit(&(it->second), sp, std::memory_order_relaxed);

Can be replaced with:

auto sp = std::atomic_load_explicit(&(it->second), std::memory_order_acquire);

...

std::atomic_store_explicit(&(it->second), sp, std::memory_order_release);

Furthermore, I understand why you are using (void) lock;, but I would not use that line just to suppress a compiler warning. Also I am surprised about that warning since a modern compiler should know that RAII objects have a function. Are you sure there is a warning or is that an assumption ?

\$\endgroup\$
  • \$\begingroup\$ Thanks for the feedback! I used a pattern I saw in preshing.com, I was curious as to why he separated the fences too, I guess it's more for clearly highlighting the role of the fences. As for the (void) lock; - just habit! \$\endgroup\$ – Nim Feb 16 '17 at 9:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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