[intrusive_ptr] atomic ref counting?
I'm thinking about replacing some uses of shared_ptr with intrusive_ptr, but I've got questions. One of the nice things about shared_ptr is the lock-free thread-safe ref counting. I want my intrusive_ptr to have the same safety and efficiency, but I don't see how to get it. I see $boost_root/detail/atomic_count.hpp, but shared_ptr doesn't use it. Why not? And why is it in detail/? Should I not consider using it? Taking in the bigger picture, intrusive_ptr requires people to write their own add_ref and release functions, most of which will probably end up using ++ and -- on a plain integer. This is totally broken in multi-threaded applications, right? Isn't it dangerous to not provide complementary utilities for correctly managing reference counts? It seems to me intrusive_ptr is impossible to use safely and portably. What have I missed? -- Eric Niebler Boost Consulting www.boost-consulting.com
Eric Niebler wrote:
I'm thinking about replacing some uses of shared_ptr with intrusive_ptr, but I've got questions. One of the nice things about shared_ptr is the lock-free thread-safe ref counting. I want my intrusive_ptr to have the same safety and efficiency, but I don't see how to get it. I see $boost_root/detail/atomic_count.hpp, but shared_ptr doesn't use it. Why not? And why is it in detail/? Should I not consider using it?
shared_ptr doesn't use atomic_count because the addition of weak_ptr has made atomic_count's interface insufficient. atomic_count is in detail:: because I didn't consider it ready for boost::. On the other hand, if it works for you, there is no reason to not use it.
Taking in the bigger picture, intrusive_ptr requires people to write their own add_ref and release functions, most of which will probably end up using ++ and -- on a plain integer. This is totally broken in multi-threaded applications, right?
Using ++ and -- on a plain integer without synchronization is broken in MT apps, yes.
Isn't it dangerous to not provide complementary utilities for correctly managing reference counts? It seems to me intrusive_ptr is impossible to use safely and portably. What have I missed?
It's possible to use intrusive_ptr safely and portably if you already have an existing library that provides intrusive reference counting without also supplying a corresponding pointer type, and one of the goals of boost::intrusive_ptr is to establish a standard "corresponding pointer type" for such libraries. However, you are right that intrusive_ptr doesn't help the implementers of these libraries to get their reference counting correct, and that it probably should do that.
shared_ptr doesn't use atomic_count because the addition of weak_ptr has made atomic_count's interface insufficient.
Can you please elaborate on what you mean? Are you saying that you cannot implement atomic refcouting because of weak pointers? Didn't 1.33 add atomics support? (Sorry, I haven't looked at the shared_ptr source in a while) Thanks.
Tomas Puverle wrote:
shared_ptr doesn't use atomic_count because the addition of weak_ptr has made atomic_count's interface insufficient.
Can you please elaborate on what you mean? Are you saying that you cannot implement atomic refcouting because of weak pointers?
No. I am saying that the sort of atomic reference counting needed by shared_ptr can't be implemented on top of the current interface (and implementation) of boost::detail::atomic_count.
Eric Niebler wrote: [snipped] FWIW, I rolled my own counted_base class for statechart. It can be found here: http://cvs.sourceforge.net/viewcvs.py/boost/boost/boost/statechart/detail/co... I'd also be interested in intrusive_ptr providing a class along the lines of counted_base... Regards, -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.
Andreas Huber wrote:
Eric Niebler wrote: [snipped]
FWIW, I rolled my own counted_base class for statechart. It can be found here: http://cvs.sourceforge.net/viewcvs.py/boost/boost/boost/statechart/detail/co...
I'd also be interested in intrusive_ptr providing a class along the lines of counted_base...
I've added something similar to your counted_base to xpressive, and it works well. I think there is a clear need for this to be a standard part of intrusive_ptr. A quick grep through the archives turns up several requests for this functionality, including a recent request from Ulrich Eckhardt. The implementation is quite trivial (see below). Peter, can this (or something like it) just be added to intrusive_ptr.hpp, or should we go through a fast-track review? If you want to just add it, I can see what I can do about tests and docs. Open questions: 1) What about the name (counted_base)? Can we do better? 2) Should there be a way to get the actual ref-count, or is bool unique() enough (like shared_ptr)? <<< Begin Code >> ////////////////////////////////////////////////////////////////////////////// // (c) Copyright Andreas Huber Doenni 2002-2005, Eric Niebler 2006 // Distributed under the Boost Software License, Version 1.0. (See // accompanying file LICENSE_1_0.txt or copy at // http://www.boost.org/LICENSE_1_0.txt) ////////////////////////////////////////////////////////////////////////////// #ifndef BOOST_XPRESSIVE_DETAIL_UTILITY_COUNTED_BASE_HPP_EAN_04_16_2006 #define BOOST_XPRESSIVE_DETAIL_UTILITY_COUNTED_BASE_HPP_EAN_04_16_2006 #include <boost/assert.hpp> #include <boost/noncopyable.hpp> #include <boost/checked_delete.hpp> #include <boost/detail/atomic_count.hpp> namespace boost { namespace xpressive { namespace detail { template<typename Derived> struct counted_base_access; //////////////////////////////////////////////////////////////////// // counted_base template<typename Derived> struct counted_base : private noncopyable { bool unique() const { return 1 == this->count_; } protected: counted_base() : count_(0) { } private: friend struct counted_base_access<Derived>; mutable boost::detail::atomic_count count_; }; //////////////////////////////////////////////////////////////////// // counted_base_access template<typename Derived> struct counted_base_access { static void add_ref(counted_base<Derived> const *that) { ++that->count_; } static void release(counted_base<Derived> const *that) { BOOST_ASSERT(0 < that->count_); if(0 == --that->count_) { boost::checked_delete( static_cast<Derived const *>(that)); } } }; template<typename Derived> inline void intrusive_ptr_add_ref(counted_base<Derived> const *that) { counted_base_access<Derived>::add_ref(that); } template<typename Derived> inline void intrusive_ptr_release(counted_base<Derived> const *that) { counted_base_access<Derived>::release(that); } }}} // namespace boost::xpressive::detail #endif -- Eric Niebler Boost Consulting www.boost-consulting.com
Eric Niebler wrote:
The implementation is quite trivial (see below).
Seems pretty complicated to me. The "canonical" counted_base is (assuming namespace boost): class counted_base { private: mutable detail::atomic_count count_; protected: counted_base(): count_( 0 ) {} virtual ~counted_base() {} counted_base( counted_base const & ): count_( 0 ) {} counted_base& operator=( counted_base const & ) { return *this; } public: inline friend void intrusive_ptr_add_ref( counted_base const * p ) { ++p->count_; } inline friend void intrusive_ptr_release( counted_base const * p ) { if( --p->count_ == 0 ) delete p; } long use_count() const { return count_; } }; Seeing so many variations of it is a pretty good indication that we need it in Boost. :-)
Peter Dimov wrote:
Eric Niebler wrote:
The implementation is quite trivial (see below).
Seems pretty complicated to me. The "canonical" counted_base is (assuming namespace boost):
class counted_base { private:
mutable detail::atomic_count count_;
protected:
counted_base(): count_( 0 ) {} virtual ~counted_base() {}
Using CRTP (as in the code I posted) avoids the need for a virtual destructor. I see no need to force the virtual on people.
counted_base( counted_base const & ): count_( 0 ) {} counted_base& operator=( counted_base const & ) { return *this; }
Huh. I was thinking it was safer to make these guys noncopyable, but ok.
public:
inline friend void intrusive_ptr_add_ref( counted_base const * p ) { ++p->count_; }
inline friend void intrusive_ptr_release( counted_base const * p ) { if( --p->count_ == 0 ) delete p; }
Tried that. Under certain circumstances, VC7.1 wasn't finding the friend functions. Hence my counted_base_access hack, and the intrusive_ptr_add_ref/release functions at namespace scope.
long use_count() const { return count_; }
Sure.
};
Seeing so many variations of it is a pretty good indication that we need it in Boost. :-)
So you're going to add it? :-) -- Eric Niebler Boost Consulting www.boost-consulting.com
Eric Niebler wrote:
Peter Dimov wrote:
Seeing so many variations of it is a pretty good indication that we need it in Boost. :-)
So you're going to add it? :-)
I can't help but think that even if I did add the above, you wouldn't use it because of the virtual destructor, and Andreas wouldn't use it because of the lack of bool NeedsLocking parameter. Anyway, ...
Tried that. Under certain circumstances, VC7.1 wasn't finding the friend functions. Hence my counted_base_access hack, and the intrusive_ptr_add_ref/release functions at namespace scope.
... I'm interested in those certain circumstances.
Peter Dimov wrote:
Eric Niebler wrote:
Peter Dimov wrote:
Seeing so many variations of it is a pretty good indication that we need it in Boost. :-) So you're going to add it? :-)
I can't help but think that even if I did add the above, you wouldn't use it because of the virtual destructor,
You're right, because in xpressive alone I use counted_base in no less than 3 places where the virtual destructor would be needless overhead! Clearly, it can't be all that uncommon. and Andreas wouldn't use it because of
the lack of bool NeedsLocking parameter.
I thought NeedsLocking was reasonable until I noticed that atomic_count is a typedef for long if BOOST_HAS_THREADS is not defined. That should be good enough, IMO.
Anyway, ...
Tried that. Under certain circumstances, VC7.1 wasn't finding the friend functions. Hence my counted_base_access hack, and the intrusive_ptr_add_ref/release functions at namespace scope.
... I'm interested in those certain circumstances.
The following program exposes the problem for me with vc7.1, vc8 and gcc 3.4.4. If you comment out the friend functions and uncomment the global free functions, the problem goes away. I don't understand it -- is some name look-up subtlety at play here? #include <boost/intrusive_ptr.hpp> template<typename Derived> struct counted_base { friend void intrusive_ptr_add_ref(counted_base<Derived> *that){} friend void intrusive_ptr_release(counted_base<Derived> *that){} }; //template<typename Derived> //void intrusive_ptr_add_ref(counted_base<Derived> *that){} //template<typename Derived> //void intrusive_ptr_release(counted_base<Derived> *that){} template<typename T> struct impl : counted_base<impl<T> > { }; template<typename T> struct outer { boost::intrusive_ptr<impl<T> > p; }; int main() { outer<int> impl; return 0; } -- Eric Niebler Boost Consulting www.boost-consulting.com
Eric Niebler wrote:
Peter Dimov wrote:
Eric Niebler wrote:
Peter Dimov wrote:
Seeing so many variations of it is a pretty good indication that we need it in Boost. :-) So you're going to add it? :-)
I can't help but think that even if I did add the above, you wouldn't use it because of the virtual destructor,
You're right, because in xpressive alone I use counted_base in no less than 3 places where the virtual destructor would be needless overhead! Clearly, it can't be all that uncommon.
It may be common. The point is that we need to settle on a single version of boost::counted_base. The virtual destructor version does have its benefits: it's easier to understand and use (CRTP can be a bit hard to swallow) and it allows intrusive_ptr<counted_base> to be used as the intrusive version of shared_ptr<void> and void* (with the additional benefit of a working dynamic_cast.) counted_base is somewhat bicycle sheddish and I've considered adding one as documentation rather than code. But one problem with that is atomic_count being in detail. :-)
The following program exposes the problem for me with vc7.1, vc8 and gcc 3.4.4. If you comment out the friend functions and uncomment the global free functions, the problem goes away. I don't understand it -- is some name look-up subtlety at play here?
I don't understand it either. Comeau/EDG compiles it. A non-template counted_base doesn't have this problem, FWIW.
Peter Dimov wrote:
Eric Niebler wrote:
Peter Dimov wrote:
I can't help but think that even if I did add the above, you wouldn't use it because of the virtual destructor,
You're right, because in xpressive alone I use counted_base in no less than 3 places where the virtual destructor would be needless overhead! Clearly, it can't be all that uncommon.
It may be common. The point is that we need to settle on a single version of boost::counted_base. The virtual destructor version does have its benefits: it's easier to understand and use (CRTP can be a bit hard to swallow) and it allows intrusive_ptr<counted_base> to be used as the intrusive version of shared_ptr<void> and void* (with the additional benefit of a working dynamic_cast.)
counted_base is somewhat bicycle sheddish and I've considered adding one as documentation rather than code. But one problem with that is atomic_count being in detail. :-)
I'm confident that we can settle on a single design, and we should. I'd rather include your counted_base with a virtual than leaving it in the docs where people will ignore it and it won't get used. How about this: template<typename Derived> struct basic_counted_base { ... my counted_base implementation here ... ... with no virtual destructor ... }; struct counted_base : basic_counted_base<counted_base> { virtual ~counted_base() {} }; That way we get the best of both worlds.
The following program exposes the problem for me with vc7.1, vc8 and gcc 3.4.4. If you comment out the friend functions and uncomment the global free functions, the problem goes away. I don't understand it -- is some name look-up subtlety at play here?
I don't understand it either. Comeau/EDG compiles it. A non-template counted_base doesn't have this problem, FWIW.
Confirmed. Also, the problem doesn't happen when impl in my example is a non-template. Looks like compiler bugs, but it's strange that both VC and gcc reject it with similar error messages. <shrug> -- Eric Niebler Boost Consulting www.boost-consulting.com
Resending. It's been a while, and I don't want this issue to get lost. Peter? Eric Niebler wrote:
Peter Dimov wrote:
Eric Niebler wrote:
Peter Dimov wrote:
I can't help but think that even if I did add the above, you wouldn't use it because of the virtual destructor,
You're right, because in xpressive alone I use counted_base in no less than 3 places where the virtual destructor would be needless overhead! Clearly, it can't be all that uncommon.
It may be common. The point is that we need to settle on a single version of boost::counted_base. The virtual destructor version does have its benefits: it's easier to understand and use (CRTP can be a bit hard to swallow) and it allows intrusive_ptr<counted_base> to be used as the intrusive version of shared_ptr<void> and void* (with the additional benefit of a working dynamic_cast.)
counted_base is somewhat bicycle sheddish and I've considered adding one as documentation rather than code. But one problem with that is atomic_count being in detail. :-)
I'm confident that we can settle on a single design, and we should. I'd rather include your counted_base with a virtual than leaving it in the docs where people will ignore it and it won't get used.
How about this:
template<typename Derived> struct basic_counted_base { ... my counted_base implementation here ... ... with no virtual destructor ... };
struct counted_base : basic_counted_base<counted_base> { virtual ~counted_base() {} };
That way we get the best of both worlds.
The following program exposes the problem for me with vc7.1, vc8 and gcc 3.4.4. If you comment out the friend functions and uncomment the global free functions, the problem goes away. I don't understand it -- is some name look-up subtlety at play here?
I don't understand it either. Comeau/EDG compiles it. A non-template counted_base doesn't have this problem, FWIW.
Confirmed. Also, the problem doesn't happen when impl in my example is a non-template. Looks like compiler bugs, but it's strange that both VC and gcc reject it with similar error messages. <shrug>
-- Eric Niebler Boost Consulting www.boost-consulting.com
Eric Niebler wrote:
You're right, because in xpressive alone I use counted_base in no less than 3 places where the virtual destructor would be needless overhead! Clearly, it can't be all that uncommon.
I did not use the CRTP in statechart for the following reasons: - If you have a hierarchy that is several levels deep, then needing to propagate the Derived parameter through all those levels will lead to considerable code bloat if you have many most-derived classes. I faintly remember someone saying that compilers should be able to detect and automatically reduce such code-bloat but I'm sceptical whether most of them do that today. To avoid the bloat you could have your most-derived classes inherit from counted_base *and* BaseClass but this also means that you can no longer use intrusive_ptr<BaseClass>. - If you have full control over object construction and destruction, you can get away with defining the add_ref & release functions for the derived classes or class templates (scroll right to the bottom in both files): <http://cvs.sourceforge.net/viewcvs.py/boost/boost/boost/statechart/detail/state_base.hpp?rev=1.22> <http://cvs.sourceforge.net/viewcvs.py/boost/boost/boost/statechart/simple_state.hpp?rev=1.35> This avoids code bloat by only duplicating the release function but still does not require any virtual destructors.
I thought NeedsLocking was reasonable until I noticed that atomic_count is a typedef for long if BOOST_HAS_THREADS is not defined. That should be good enough, IMO.
Sometimes it isn't good enough. I have a scenario where I know that counted_base subclass objects will *always* be accessed by one and the same thread. The program is still multithreaded, so doing away with NeedsLocking would generate unnecessary overhead here. Regards, -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.
participants (4)
-
Andreas Huber -
Eric Niebler -
Peter Dimov -
Tomas Puverle