Migrating RefCountedPtr to std::shared_ptr?

[lwho]

When doing the "No global galaxy" refactoring in #3198, I really wished that we had weak pointers. I thought about implementing them for our RefCountedPtr, but unfortunately it seems to be quite hard to get weak pointers for intrusive ref-counted pointers thread-safe.

On the other hand, in C++11 there is std::shared_ptr and std::weak_ptr. Our code precedes C++11, that's why we are having our own smart pointer class. Our RefCountedPtr is intrusive, which std::shared_ptr is not, and we are exploiting this property in our code (i.e. we create other RefCountedPtr instances from bare pointers). However, this is also possible with C++11 by inheriting from std::enable_shared_from_this (which I just discovered, I really wish I had known this before "no_global_galaxy").

I'm wondering if we should start migrating to the C++11 way enabling us to use weak pointers. I had to use some hacks to break cycles in the "No global galaxy" code and I would rather get rid of them sooner or later. There is no need to migrate all RefCounted classes at once. Migration could be done class-by-class as needed. For each class it would involve:

1. Inheriting from std::enable_shared_from_this instead of from RefCounted
2. Change all RefCountedPtr<Class> to std::shared_ptr<Class>
3. Change all constructions of smart pointers from bare pointers (except for the first creation) from a constructor call

   Code: Select all

   smart_pointer = std::shared_ptr<Class>(bare_pointer)

   or

   Code: Select all

   smart_pointer = make_shared(bare_pointer)

   to

   Code: Select all

   smart_pointer = bare_pointer->shared_from_this()

Before starting this for Galaxy, I would like to hear opinions about it.

[FluffyFreak]

The usual: check it works in Visual Studio 2013 :)

Other than that I say rip out RefCountedPtr and switch in one go.

[FluffyFreak]

Oh, obviously I'd be happy to help you test that of course

[lwho]

The usual: check it works in Visual Studio 2013 :)

MSDN says it does. We'll only be sure if we try, though ;) Maybe we should start with something smaller than Galaxy to try.

[jpab]

TLDR: Sure, go for it.

Weak pointer support is probably a good enough reason. As we increase the amount of multi-threaded code shared_ptr's thread safety may also be useful. Other than that, I do prefer intrusive ref-counting, as it avoids some pitfalls.

Regarding refcounting in general: It's very easy to end up using refcounting all over the place even where it's not really needed, ie, in places where single ownership (or single-transferrable-ownership) would actually work fine if you're careful to choose the correct single owner. That can quickly lead to code where ownership is very unclear, which is bad, and also has a potential performance impact (which I hope will be very small in our case). So I would say although it is sometimes useful and the best option, it would be best to try not to let shared_ptr (or RefCounted) creep everywhere. It might already be used in places that don't need it.

[lwho]

Other than that, I do prefer intrusive ref-counting, as it avoids some pitfalls.

Are you thinking about anything else than the possibility to safely create new smart pointers from bare pointers? This is possible for std::shared_ptr through std::enable_shared_from_this, which essentially puts a weak_ptr to itself in an object.

Regarding refcounting in general: It's very easy to end up using refcounting all over the place even where it's not really needed, ie, in places where single ownership (or single-transferrable-ownership) would actually work fine if you're careful to choose the correct single owner. That can quickly lead to code where ownership is very unclear, which is bad, and also has a potential performance impact (which I hope will be very small in our case). So I would say although it is sometimes useful and the best option, it would be best to try not to let shared_ptr (or RefCounted) creep everywhere. It might already be used in places that don't need it.

Yes, I suspect so. I have the feeling that I somewhat overdid this recently in my Galaxy code. I hope, this can be improved with weak pointers in some places. While this technically still requires ref-counted smart pointers, some of them will degenerate to the case of one longer-lifed shared_ptr establishing quasi-exclusive ownership and the rest weak_ptrs (which only when locked will spawn temporary additional shared pointers).

Many of our objects logically have a well-defined ownership and lifetime. But then multi-threaded jobs that might run slightly beyond this lifetime come into play and complicate matters a lot. These places typically need RefCountedPtr at the moment and a lot of care to avoid cycles, but could be made a lot easier by using weak pointers. This procedural generation in background is a nightmare.

[jpab]

Other than that, I do prefer intrusive ref-counting, as it avoids some pitfalls.

Are you thinking about anything else than the possibility to safely create new smart pointers from bare pointers? This is possible for std::shared_ptr through std::enable_shared_from_this, which essentially puts a weak_ptr to itself in an object.

Yes, that's the only reason I can think of right now. Or rather, the other side of that, which is that if you take a normal object it's possible to create multiple independent shared_ptrs that all think they own the object, and that of course leads to use-after-free and double-free bugs.

[FluffyFreak]

So that this doesn't get lost:
I've heard that std::shared_ptr has a lot of performance(2) and usage issues so it might be worth benchmarking our RefCountedPtr vs std::shared_ptr.