views:

224

answers:

3

In the boost::shared_ptr destructor, this is done:

if(--*pn == 0)
{
    boost::checked_delete(px);
    delete pn;
}

where pn is a pointer to the reference counter, which is typedefed as

shared_ptr::count_type -> detail::atomic_count -> long

I would have expected the long to be volatile long, given threaded usage and the non-atomic 0-check-and-deletion in the shared_ptr destructor above. Why isn't it volatile?

EDIT:

It turns out I looked at the header used when multi-threaded usage is not specified (atomic_count.hpp). In atomic_count_win32.hpp, the decrement is properly implemented for multithreaded usage.

+7  A: 

volatile has virtually nothing to do with threading. See here.

Marcelo Cantos
Would you say that there's no problem with the above treatment of the counter in the destructor, if executed on different threads?
Johann Gerell
+12  A: 

Because volatile is not necessary for multithreading, and does nothing beneficial, but potentially destroys a number of optimizations.

In order to ensure safe multithreaded access to a variable, the primitive we need is a memory barrier, which provides both the guarantee of volatile and a few others (it prevents memory access reordering across the barrier, which volatile doesn't do)

I believe shared_ptr uses atomic operations when possible, which implicitly provide a memory barrier. Otherwise it falls back to a mutex, which also provides a memory barrier.

See http://stackoverflow.com/questions/2484980/why-is-volatile-not-considered-useful-in-multithreaded-c-or-c-programming or http://software.intel.com/en-us/blogs/2007/11/30/volatile-almost-useless-for-multi-threaded-programming/ for more details

Edit
count_type is not a long in the general case. It is convertible to a long. If you look in atomic_count.hpp, the typedef to long is only applied if no threading is available (in which case of course, no synchronization is necessary). Otherwise it uses the implementation defined in boost/smart_ptr/detail/atomic_count_pthreads.hpp or boost/smart_ptr/detail/atomic_count_win32.hpp or one of the other files listed. And those are synchronized wrapper classes that ensures all operations are done atomically.

jalf
"I believe shared_ptr uses atomic operations when possible" - not in my quoted example with the destructor, which is where my problem lies now...
Johann Gerell
Well, making the variable `volatile` wouldn't have changed anything. You're right, decrementing and comparing a long against zero does seem dangerous, but it depends on what the *rest* of the pointer class is doing.
jalf
@jalf: Well, that's were you lost me. ;-) Right now, in my example, two threads enter the shared_ptr dtor and "the rest of the pointer class" does nothing else.
Johann Gerell
@Johann: But again, even if it *is* a bug, how would `volatile` have fixed it?
jalf
Ok, check my edit. :)
jalf
@jalf: Ok, sorry, I wasn't explicit enough - I agree (after reading your links) that volatile is out. But given that fact, I couldn't see how your "it depends on what the rest of the pointer class is doing" could be justified in the dtor example. Note - I'm not saying you're wrong, I just want to understand.
Johann Gerell
@jalf: Great! My Visual Assist "Goto definition" jumps went nuts and missed that one! atomic_count_win32.hpp perfectly explains my doubts. Thanks!
Johann Gerell
@jalf: Edited the question to clarify things.
Johann Gerell
+2  A: 

You are misreading the code. atomic_count is defined simply as a long if the code is not using multithreading:

#ifndef BOOST_HAS_THREADS

namespace boost
{

namespace detail
{

typedef long atomic_count;

}

}

#elif //... include various platform-specific headers that define atomic_count class

#endif
visitor
Thanks! Edited the question to clarify things. (Had already accepted jalf's answer)
Johann Gerell