Is "Reference counting" example misleading and wrong?

[Adam Badura <adam.f.badura@…>]

Boost.Atomic in its documentation provides "Reference counting" example: http://www.boost.org/doc/libs/1_61_0/doc/html/atomic/usage_examples.html#boost_atomic.usage_examples.example_reference_counters

The example itself is excellent. After all this is a typical use case so providing an example with brief discussion is mostly welcome.

The problem is however whether the example is correct after all. I mean here the part about counter decrement. Example uses release order and then a fance with acquire:

friend void intrusive_ptr_release(const X * x)
{
  if (x->refcount_.fetch_sub(1, boost::memory_order_release) == 1) {
    boost::atomic_thread_fence(boost::memory_order_acquire);
    delete x;
  }
}

while description states later:

It would be possible to use memory_order_acq_rel for the fetch_sub operation, but this results in unneeded "acquire" operations when the reference counter does not yet reach zero and may impose a performance penalty.

At the same time boost::shared_ptr uses acq_rel in its std::atomic implementation in file boost/smart_ptr/detail/sp_counted_base_std_atomic.hpp (http://www.boost.org/doc/libs/1_61_0/boost/smart_ptr/detail/sp_counted_base_std_atomic.hpp).

Also Herb Sutter in his "C++ and Beyond 2012: Herb Sutter - atomic<> Weapons, 2 of 2" (​https://channel9.msdn.com/Shows/Going+Deep/Cpp-and-Beyond-2012-Herb-Sutter-atomic-Weapons-2-of-2) lecture mentions reference counting topic (starting at 1:19:51) where he also calls for acq_rel. And it seems that he discourages use fences in this talk.

So which one is good? Because either Boost.Atomic documentation should be updated or boost::shared_ptr should be improved.

And let me note here that this Boost.Atomic documentation part seems to be quoted (or refereed to) at various places, including Stack Overflow, quite often. So it is important for it to be correct.

---

BTW (although beyond this ticket) it seems strange to me that boost::shared_ptr doesn't use boost::atomic and instead provides like 19 different implementations. It smells like code repetition to me. But maybe there are valid reasons for it...

[Andrey Semashev]

I think the code in the documentation is valid.

The release semantics is needed to guarantee that any modifications made by a thread before it decrements the counter are visible to other threads.

The acquire semantics is only needed if the object destructor is called. It makes sure that the destructor observes the actual state of the object that might have been modified by another thread that previously decremented the counter from 2 to 1. The standard (C++11, [atomics.fences]/4) says that in this code the atomic_thread_fence(acquire) synchronizes with fetch_sub(1, release) issued by other threads, so it should not be reordered with the fetch_sub.

It is formally correct to merge this acquire fence with the fetch_sub operation, but depending on the CPU architecture it may or may not be slightly slower.

As for boost::shared_ptr, you can propose the change to that library. Boost.SmartPtr predates Boost.Atomic, so it contains its own implementation of atomic operations for many platforms.