Avoiding iterator invalidation using indices, maintaining clean interface

Question

I have created a MemoryManager<T> class which is basically a wrapper around two vectors of pointers that manage lifetime of heap-allocated objects.

One vector stores the "alive" objects, the other one stores the object that will be added on next MemoryManager<T>::refresh.

This design was chosen to avoid iterator invalidation when looping over the MemoryManager<T>, as adding a new object directly to the MemoryManager<T>::alive vector can invalidate existing iterators (if it grows in size).

template<typename T> struct MemoryManager {
    std::vector<std::unique_ptr<T>> alive;
    std::vector<T*> toAdd;

    T& create() { 
        auto r(new T); 
        toAdd.push_back(r); 
        return *r; 
    }

    T& refresh() { 
         // Use erase-remove idiom on dead objects
         eraseRemoveIf(alive, [](const std::unique_ptr<T>& p){ return p->alive; });

         // Add all "toAdd" objects and clear the "toAdd" vector
         for(auto i : toAdd) alive.emplace_back(i); 
         toAdd.clear(); 
    }  

    void kill(T& mItem)  { mItem.alive = false; }

    IteratorType begin() { return alive.begin(); }
    IteratorType end()   { return alive.end(); }
}

I use it in my game engine to store entities, and update every "alive" entity every frame:

void game() {
    MemoryManager<Entity> mm;

    while(gameLoop) {
        mm.refresh();
        for(auto e : mm) processEntity(e);
        auto& newEntity = mm.create();
        // do something with newEntity
    }
}

This has allowed me to constantly create/kill entities without having to worry about their lifetime too much.

---

However, I've recently come to the conclusion that using two std::vector is unnecessary. I could simply use a single vector and store an iterator to the "last alive object", adding the newly create objects immediately after the aforementioned iterator:

The idea, in my mind, works fine... but I cannot actually use a iterator type for end (as shown in the diagram), as it could get invalidated after the addition of some new elements to the vector. I've tested it, and this happens often, causing a crash.

The other solution I can think of is using an index instead of an iterator. This would solve the crashing, but I wouldn't be able to use the cool C++11 for(x : y) foreach loop because MemoryManager<T>::begin and MemoryManager<T>::end need to return an iterator.

Is there a way to achieve the current behavior with a single vector and still maintain a clear interface that can be used with C++11 for-each loops?

Answer 1

One of the simplest ways to get stable iterators (and references) is to use std::list<T>. And unless you are needing T to be a pointer to a polymorphic base class, it is better to use std::list<T>, as opposed to std::list<std::unique_ptr<T>>.

If on the other hand, your Entity is a polymorphic base, then consider using std::vector<std::unique_ptr<T>>. Although you can not depend upon iterators remaining valid, you can depend upon pointers and references to Entity remaining valid with std::vector<std::unique_ptr<T>>.

In your game() example, you never take advantage of stable iterators or pointers. You could just as easily (and more simply) do:

void game() {
    std::vector<Entity> mm;

    while(gameLoop) {
        mm.erase(std::remove_if(mm.begin(), mm.end(), [](const Entity& e)
                                                      { return e.alive; }),
                                                      mm.end());
        for(auto e : mm) processEntity(e);
        mm.push_back(create());
        auto& newEntity = mm.back();
        // do something with newEntity
    }
}

During the processEntity loop, there is no way to invalidate iterators. If you did, you had better not use the range-based-for as the end iterator is only evaluated once, at the beginning of the loop.

But if you really do need stable iterators/references, substituting in std::list<Entity> would be very easy. I would change the erase/remove to use list's member remove_if instead. It will be more efficient.

If you do this, and performance testing (not guessing) indicates you've suffered a performance hit over your existing MemoryManager, you can optimize list by using a "stack allocator" such as the one demonstrated here:

http://howardhinnant.github.io/stack_alloc.html

This allows you to preallocate space (could be on the stack, could be on the heap), and have your container allocate from that. This will be both high performance and cache-friendly until the pre-allocated space is exhausted. And you've still got your iterator/pointer/reference stability.

In summary:

1. Find out / tell us if unique_ptr<Entity> is actually necessary because Entity is a base class. Prefer container<Entity> over container<unique_ptr<Entity>>.

2. Do you actually need iterator/pointer/reference stability? Your sample code does not. If you don't actually need it, don't pay for it. Use vector<Entity> (or vector<unique_ptr<Entity>> if you must).

3. If you actually need container<unique_ptr<Entity>>, can you get away with pointer/reference stability while sacrificing iterator stability? If yes, vector<unique_ptr<Entity>> is the way to go.

4. If you actually need iterator stability, strongly consider using std::list.

5. If you use std::list and discover via testing it has performance problems, optimize it with an allocator tuned to your needs.

6. If all of the above fails, then start designing your own data structure. If you get this far, know that this is the most difficult route, and everything will need to be backed up by both correctness and performance tests.