Bad intrusive reference counting implementation, please criticize

68b0a5876289e2b5351cc1956ba80dc8
0
Almos 101 May 04, 2010 at 06:56

Hi,

I know it’s going to hurt, but it’s inevitable in learning good programming practices, isn’t it?

Here’s an intrusive reference counting implementation I’ve created after reading a chapter on handle classes in Stroustrup; it surely contains some ankward constructs that I’ve inevitably inserted into it as person who largely dealt with scripting for the last two years. I’d be grateful for suggestions and criticism on how to make it better.

Reference counted class:

class cRefCounted {
    public:
        cRefCounted() {
            ref_count = 0;
        }
        void RefAdd() {
            ref_count++;
        }
        void RefRemove() {
            if(--ref_count<=0)
                delete this;
        }

        std::string type() {
            return typeid(this).name();
        }

    protected:
    int ref_count;
};

Handle class:

template<class T> class tHandle {
    public:
        explicit tHandle(cRefCounted* obj) {
            init(obj);
        }

        tHandle(const tHandle& handle) {
            init(handle.obj);
        }

        virtual ~tHandle() {
            obj->RefRemove();
        }

        T& operator*() const {
            return *obj;
        }

        T* operator->() const {
            return obj;
        }

        T* operator=(const tHandle& handle) {
            if(handle.obj!=obj) init(handle.obj);
            return obj;
        }


    protected:
        T* obj;
        void init(cRefCounted* obj) {
                try {
                    this->obj = static_cast<T*>(obj);
                    obj->RefAdd();
                }

            catch(const std::bad_cast &e) {
                    fprintf(stderr, "Not an instance of cRefCounted\n");
                    this->obj = NULL;
            }
        }
};

Thanks in advance.

9 Replies

Please log in or register to post a reply.

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 May 04, 2010 at 11:24

Have you even tested this? It is certainly not going to work like you intended B)

Some things that are just wrong:
cRefCounted::RefRemove(): it does a ‘delete this’ but cRefCounted doesn’t have a virtual destructor. Therefore, any subclass of cRefCounted will never get destructed appropriately. It is always a good idea to include a virtual destructor if your class is going to be derived from. You may want to make this destructor private if you only want a class to be constructed using new(), rather than a local/global or as a member of another class. You can make it protected to leave this decision to the derived class.

cRefCounted::type() function is also flawed. It uses typeid(this), but ‘this’ is a pointer so it will always yield ‘cRefCounted*’. If you want to want an actual runtime type lookup, you should dereference the pointer. Note that this currently won’t work either as cRefCounted doesn’t have a vtable (and therefore no type information), but adding the previously suggested virtual destructor should solve this.

tHandle::init(): a static_cast will never fail and never throw a bad_cast. If you want runtime type checking, use dynamic_cast. But this currently fails to compile because dynamic_cast needs type information, which a cRefCounted doesn’t have as it doesn’t have a vtable B). Another thing is that init() does not remove a reference on the previous held object. If I have a tHandle that points to A and I then let it point to B using tHandle::operator=(), A’s refcount will never get decreased.

Some suggestions:
I would let the tHandle constructor take a T* rather than a cRefCounted*, to make it compile-time type safe. Of course, bad use will cause a runtime exception if you implement the init() using a dynamic_cast, but having a compile-time error will let you catch the error earlier. Also, C++’s design perspective has the overall paradigm that the users choose when they want to pay the price. A dynamic_cast is costlier than a static_cast. Create your own casting functions that implement both static_cast and dynamic_cast on the actual pointers.

For example:

template<class T1, class T2> tHandle<T1> dynamic_handle_cast(const tHandle<T2> & handle)
{
    return tHandle<T1>(dynamic_cast<T1*>(handle.obj));
}

template<class T1, class T2> tHandle<T1> static_handle_cast(const tHandle<T2> & handle)
{
    return tHandle<T1>(static_cast<T1*>(handle.obj));
}

You may want to add a templated tHandle conversion constructor to be able to implicitely contruct a tHandle<A> from a tHandle<B> if B derives from A. You can use SFINAE tricks to only include that constructor if B actually is implicitely convertable to A which I don’t want to delve into right now, or you can simply add the ctor and just let the compiler give an error when the actual template instantiation turns out to be incorrect (when B doesn’t actually derive from A), in which case the user gets a vager error message but the net effect is the same.

There’s no need to make tHandle::\~tHandle() virtual. tHandle is a class that will probably not ever be derived from. The extra vtable only costs extra performance and memory.

Also, keep in mind the current implementation is not thread-safe. This may suite your needs, but it is something to remember :)

68b0a5876289e2b5351cc1956ba80dc8
0
Almos 101 May 04, 2010 at 11:59

Ok - thanks for suggestions. Will try to implement them as soon as I’ve some time.

EDIT: Well - it appeared to work :) At the moment I’m using the MinGW compiler which doesn’t tolerate the private destructors, so I made the destructor virtual but public for the time being.

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 May 04, 2010 at 12:34

Oh duh, my bad, making a dtor private means the class can’t be overridden B). I’d make it protected, so at least you can’t manually delete a pointer to a cRefCounted

68b0a5876289e2b5351cc1956ba80dc8
0
Almos 101 May 04, 2010 at 13:13

But there was another error in the code that you haven’t spotted - and which haven’t manifested itself in my test application just because the destructor wasn’t declared as virtual.

And it was a cardinal sin.

The init() method DIDN’T INCREASE THE REFERENCE COUNT. The reference count was DECREASED each time the handle destructor was called, but it was never increased. When I made the destructor of the cRefCounted class virtual, I got runtime error when the program tried to delete a non-existent object.

Grrr.

EDIT: No, the count was increased in the original version, but it seems I removed it accidentally when rewriting the code. Anyway, only now can I be certain that the garbage is actually collected. Thanks.

820ce9018b365a6aeba6e23847f17eda
0
geon 101 May 05, 2010 at 11:52

Two points:

1: In init(), why do you want to cast the obj pointer from a cRefCounted* into a T* ? It makes no sense, since you only need to access the methods in the base class anyway.

Stupid code:

     void init(cRefCounted* obj) {
                try {
                    this->obj = static_cast<T*>(obj);
                    obj->RefAdd();
                }

            catch(const std::bad_cast &e) {
                    fprintf(stderr, "Not an instance of cRefCounted\n");
                    this->obj = NULL;
            }
        }

Better code:

     void init(cRefCounted* obj) {
          obj->RefAdd();
     }

2: Why force all ref-counted objects to inherit your base class? It’s very intrusive, so you can’t ref-count other stuff like STL vectors or textures.

A more practical solution would be to store the reference count in the handle class:

template<class T> class tHandle {
    protected:

        static std::map<T*, int> refCounts;

    (...)
};
340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 May 05, 2010 at 12:35

@geon

Two points: 1: In init(), why do you want to cast the obj pointer from a cRefCounted* into a T* ? It makes no sense, since you only need to access the methods in the base class anyway.

No, he assigns to this->obj as well, which is a T*.

2: Why force all ref-counted objects to inherit your base class? It’s very intrusive, so you can’t ref-count other stuff like STL vectors or textures.

Because it’s more efficient? Besides, if you want a non-intrusive refcount, you could just as wel use std::tr1::shared_ptr, which is way more likely to be bug-free.

A more practical solution would be to store the reference count in the handle class:

template<class T> class tHandle {
    protected:

        static std::map<T*, int> refCounts;

    (...)
};

So if you have a B that derives from A, and you have a tHandle<A> and tHandle<B> that point to the same object, then what? Again, you’d better rely on proven technology like shared_ptr.

820ce9018b365a6aeba6e23847f17eda
0
geon 101 May 08, 2010 at 18:05

@.oisyn

No, he assigns to this->obj as well, which is a T*.

Because it’s more efficient?

If the pointer-passing is your bottleneck, you are doing something wrong.

Besides, if you want a non-intrusive refcount, you could just as wel use std::tr1::shared_ptr, which is way more likely to be bug-free.

Absolutely! But doing it yourself is educational. Just make sure you throw it away when it “works”, and use something more standard, like boost.

So if you have a B that derives from A, and you have a tHandle<A> and tHandle<B> that point to the same object, then what? Again, you’d better rely on proven technology like shared_ptr.

Good point.

820ce9018b365a6aeba6e23847f17eda
0
geon 101 May 08, 2010 at 18:09

@.oisyn

No, he assigns to this->obj as well, which is a T*.

Well, obj should obviously also be a cRefCounted*. Or did I miss something?

I do consider you a C++ guru, so I’m expecting there’s something I might have misunderstood. :worthy:

340bf64ac6abda6e40f7e860279823cb
0
_oisyn 101 May 09, 2010 at 23:13

@geon

If the pointer-passing is your bottleneck, you are doing something wrong.

True, if pointer passing were as cheap as int passing. A dynamic_cast, however, is not that cheap. Here’s a quick test (using VS 2010 which supports some of the new C++0x features)

#include <iostream>
#include <vector>
#include <algorithm>
#include <cstdlib>
#include <ctime>
#include <windows.h>

struct Base { virtual void foo() = 0; };
struct A : Base { virtual void foo() { } };
struct B : Base { virtual void foo() { } };
struct C : Base { virtual void foo() { } };


int wmain()
{
    std::vector<Base*> v;
    for (int i = 0; i < 1000000; i++)
    {
        int r = std::rand() % 3;
        v.push_back(r == 0 ? (Base*)new A() : r == 1 ? (Base*)new B() : (Base*)new C());
    }
    v.push_back(new A());

    A * ptr;
    auto c = -std::clock();
    for (int i = 0; i < 100; i++)
        std::for_each(v.begin(), v.end(), [&](Base*:) { ptr = dynamic_cast<A*>(B); });
    c += std::clock();
    std::cout << c << " " << ptr << std::endl;

    c = -std::clock();
    for (int i = 0; i < 100; i++)
        std::for_each(v.begin(), v.end(), [&](Base*B) { ptr = static_cast<A*>(B); });
    c += std::clock();
    std::cout << c << " " << ptr << std::endl;
}

On my machine, the dynamic_cast takes 4194 milliseconds, while static_cast takes 58 ms. You pay the price any time you copy-construct or copy-assign a tHandle<T>, no matter which T.

But of course, a better reason not to implement it like this is because the implicit conversion between unrelated types does not fit in the overall C++ paradigm. If you can’t implicitly convert a A* to a B*, why should you be able to implicitely convert a tHandle<A> to a tHandle<B>?

Absolutely! But doing it yourself is educational.

Fair enough! So let’s discuss what a non-intrusive implementation would look like :)

Your map suggestion obviously wouldn’t work for said reasons. You could adjust it slightly to use a single shared instance of map<void*,int>, and use dynamic_cast to cast the T* to void*, which would result in a pointer to the most derived type. Unfortunately this requires T to have a vtable, which means you can’t have a tHandle<int> for example.

The common implementation of std::tr1::shared_ptr allocates a control block upon assignment to the first shared_ptr. This control block holds the reference count, and it’s shared amongst all shared_ptrs that point to this object. However, a shared_ptr constructed or assigned to from a regular T* creates the control block, so in order to keep one control block per object you need to work with shared_ptr (or weak_ptr) everywhere, and never recreate a shared_ptr using the native pointer. An implication of this is that you can’t create a shared_ptr that points to ‘this’ within the class itself. This is what ‘enables_shares_from_this’ is for.