Should I call reset on a weak_ptr if I happen to notice it's expired?

Question

I have a collection of Creature objects that are created and owned in one part of my application using std::make_shared and std::shared_ptr.

I also keep track of a selection of zero or one Creature in a World object using a std::weak_ptr<Creature>.

void World::SetSelection(const std::shared_ptr<Creature>& creature) {
    selection = creature;
}

std::shared_ptr<Creature> World::GetSelection() const {
    return selection.lock();
}

The caller of GetSelection is responsible for checking if the pointer is empty. If it is, that means there is currently no selection.

This all works perfectly to my liking: when the selected Creature dies of natural causes (elsewhere in the application), GetSelection starts returning a nullptr again as if nothing was ever selected.

However in that case the World::selection member still points to the std::shared_ptr's control block. This can be quite big because I use std::make_shared to create my Creature objects (I realize that the Creature object was correctly destroyed at the right time but the memory for it is still allocated). I'm considering changing GetSelection to this:

std::shared_ptr<Creature> World::GetSelection() {
    const auto ret = selection.lock();
    if (!ret)
        selection.reset();

    return ret;
}

This frees up the memory as soon as I notice it's not needed anymore. Annoyingly, this version of GetSelection cannot be const.

My questions:

1. Which version of GetSelection would be considered best practice in this situation?

2. Does the answer change if something similar happens in templated code, where sizeof(T) is unknown and could be huge? Or in C++14 where std::make_shared<T[]> could be involved?

3. If the second version is always best, what is the rationale for std::weak_ptr<T>::expired and lock not doing it themselves?

Answer 1

It should be noted, first off, that the emplacement strategy of std::make_shared is optional, i.e. the standard does not mandate that implementations perform this optimization. It's a non-binding requirement, which means that perfectly conforming implementations may elect to forego it.

To answer your questions:

1. Given that you seem to have only one selection (and you're not therefore bloating your memory use by keeping many of these control blocks around), I'd argue for keeping it simple. Is memory a bottleneck? This screams micro-optimization to me. You should write the simpler code, where you can apply const, and then go back and optimize later if the need arises.

2. The answer doesn't unconditionally change, it changes conditional upon the problem domain and what your bottleneck is. If you're allocating one object that's "huge" (say a hundred kilobytes), and the space for that object is kicking around in a relatively unused control block until replaced, that probably isn't your bottleneck, and probably isn't worth writing more code (which is intrinsically more error prone, difficult to maintain, and decipher) to "solve".

3. As std::weak_ptr::lock and std::weak_ptr::expired are const, under the interpretation of const for C++11, they must be thread safe. Therefore, given some std::weak_ptr, it must be safe to call any combination of lock() and expired() concurrently. Under the hood, the std::weak_ptr stores a pointer to the control block, which it looks through to examine/increment/etc. atomic counters to determine if the object has expired, or to see if it can acquire the lock. If you wanted to implement your optimization internal to std::weak_ptr, you'd have to somehow inspect the control block's state, and then atomically remove the pointer to the control block if the pointer has expired. This would cause overhead (even if this could be done simply with atomics, it would still have overhead) on every access to the std::weak_ptr, all for the sake of a small optimization.

Answer 2

1. The first version of GetSelection is better for the vast majority of cases. This version can be const and doesn't need extra synchronization code to be thread-safe.

2. In generic library code where the exact usage pattern cannot be predicted in advance, the first version is still preferred. However in a situation where synchronization code is already in place protecting access to the weak_ptr, it cannot hurt to slip in a call to reset to free memory and make later uses of the pointer faster. This very small optimization is not in itself worthy of putting in that synchronization code.

3. Given the first two answers, this last question is moot. However here are two strong rationales for not having weak_ptr::lock automatically reset the pointer when it is found to be expired:

   • With that behavior it would be impossible to implement weak_ptr::owner_before, and thus use a weak_ptr as the key type in an associative container.

   • Also, even ordinary usage of weak_ptr::lock on a live object could not be implemented without extra synchronization code. This would cause a performance penalty far greater than the minor gain that could be expected from freeing memory more eagerly.

Alternative solution:
If the wasted memory is deemed to be an actual problem that needs to be solved (perhaps the shared objects are truly big and/or the target platform has very limited memory), another option is to create the shared objects with shared_ptr<T>(new T) instead of make_shared<T>. This will free the memory allocated for T even earlier (when the last shared_ptr pointing to it is destroyed) while the small control block lives separately.