utils: Secure EntityManager and hide implementation details

Move the `mGens` array and `isAlive()` implementation from `EntityManager`
to `EntityManagerImpl` to hide implementation details and maintain a clean
public header. Make `isAlive()` strictly thread-safe by acquiring the
`mFreeListLock`.

isAlive() was relying on undefined behavior, and the comment about
the memory barrier being provided by the lock was misleading (wrong)
because the lock only provided the "release", but isAlive() didn't
have an "acquire". 

Better safe than sorry, we now have to acquire the lock to test isAlive().

libs/utils/include/utils/EntityManager.h

@@ -51,7 +51,7 @@ public:
         virtual ~Listener() noexcept;
     };
 
-    using ChangeCallback = std::function<void(utils::Slice<const Entity>)>;
+    using ChangeCallback = std::function<void(Slice<const Entity>)>;
 
     /**
      * Registers a callback to be triggered when entities are destroyed.
@@ -105,10 +105,7 @@ public:
 
     // Return whether the given Entity has been destroyed (false) or not (true).
     // Thread safe.
-    bool isAlive(Entity const e) const noexcept {
-        assert(getIndex(e) < RAW_INDEX_COUNT);
-        return (!e.isNull()) && (getGeneration(e) == mGens[getIndex(e)]);
-    }
+    bool isAlive(Entity e) const noexcept;
 
     // Registers a listener to be called when an entity is destroyed. Thread safe.
     // If the listener is already registered, this method has no effect.
@@ -118,12 +115,8 @@ public:
     void unregisterListener(Listener* l) noexcept;
 
 
-    /* no user serviceable parts below */
 
-    // current generation of the given index. Use for debugging and testing.
-    uint8_t getGenerationForIndex(size_t const index) const noexcept {
-        return mGens[index];
-    }
+    /* no user serviceable parts below */
 
     // singleton, can't be copied
     EntityManager(const EntityManager& rhs) = delete;
@@ -157,9 +150,6 @@ private:
     static Entity::Type makeIdentity(Entity::Type const g, Entity::Type const i) noexcept {
         return (g << GENERATION_SHIFT) | (i & INDEX_MASK);
     }
-
-    // stores the generation of each index.
-    uint8_t* const mGens;
 };
 
 } // namespace utils

libs/utils/include/utils/JobSystem.h

@@ -43,7 +43,7 @@
 
 namespace utils {
 
-class UTILS_PUBLIC JobSystem {
+class JobSystem {
     static constexpr size_t MAX_JOB_COUNT = 1 << 14; // 16384
     static constexpr uint32_t JOB_COUNT_MASK = MAX_JOB_COUNT - 1;
     static constexpr uint32_t WAITER_COUNT_SHIFT = 24;

libs/utils/src/EntityManager.cpp

@@ -18,21 +18,21 @@
 
 #include "EntityManagerImpl.h"
 
+#include <utils/Entity.h>
+
+#include <cassert>
+#include <cstddef>
+#include <mutex>
 #include <new>
+#include <utility>
 
 namespace utils {
 
 EntityManager::Listener::~Listener() noexcept = default;
 
-EntityManager::EntityManager()
-        : mGens(new uint8_t[RAW_INDEX_COUNT]) {
-    // initialize all the generations to 0
-    std::fill_n(mGens, RAW_INDEX_COUNT, 0);
-}
+EntityManager::EntityManager() = default;
 
-EntityManager::~EntityManager() {
-    delete [] mGens;
-}
+EntityManager::~EntityManager() = default;
 
 EntityManager& EntityManager::get() noexcept {
     // note: we leak the EntityManager because it's more important that it survives everything else
@@ -41,11 +41,11 @@ EntityManager& EntityManager::get() noexcept {
     return *instance;
 }
 
-void EntityManager::create(size_t n, Entity* entities) {
+void EntityManager::create(size_t const n, Entity* entities) {
     static_cast<EntityManagerImpl *>(this)->create(n, entities);
 }
 
-void EntityManager::destroy(size_t n, Entity* entities) noexcept {
+void EntityManager::destroy(size_t const n, Entity* entities) noexcept {
     static_cast<EntityManagerImpl *>(this)->destroy(n, entities);
 }
 
@@ -84,4 +84,8 @@ void EntityManager::dumpActiveEntities(utils::io::ostream& out) const {
 
 #endif
 
+bool EntityManager::isAlive(Entity const e) const noexcept {
+    return static_cast<EntityManagerImpl const *>(this)->isAlive(e);
+}
+
 } // namespace utils

libs/utils/src/EntityManagerImpl.h

@@ -40,6 +40,7 @@
 #include <deque>
 #include <memory>
 #include <mutex> // for std::lock_guard
+#include <new>
 #include <utility>
 #include <vector>
 
@@ -49,15 +50,38 @@ static constexpr size_t MIN_FREE_INDICES = 1024;
 
 class UTILS_PRIVATE EntityManagerImpl : public EntityManager {
 public:
+    friend class EntityManager;
+
     using EntityManager::getGeneration;
     using EntityManager::getIndex;
     using EntityManager::makeIdentity;
     using EntityManager::create;
     using EntityManager::destroy;
 
+    EntityManagerImpl() noexcept
+            : mGens(new(std::nothrow) uint8_t[RAW_INDEX_COUNT]) {
+        // initialize all the generations to 0
+        std::fill_n(mGens, RAW_INDEX_COUNT, 0);
+    }
+
+    ~EntityManagerImpl() noexcept {
+        delete [] mGens;
+    }
+
+    bool isAlive(Entity const e) const noexcept {
+        assert(getIndex(e) < RAW_INDEX_COUNT);
+        if (e.isNull()) {
+            return false;
+        }
+
+        std::lock_guard const lock(mFreeListLock);
+        bool const alive = (getGeneration(e) == mGens[getIndex(e)]);
+        return alive;
+    }
+
     UTILS_NOINLINE
     size_t getEntityCount() const noexcept {
-        std::lock_guard<Mutex> const lock(mFreeListLock);
+        std::lock_guard const lock(mFreeListLock);
         if (mCurrentIndex < RAW_INDEX_COUNT) {
             return (mCurrentIndex - 1) - mFreeList.size();
         } else {
@@ -66,13 +90,13 @@ public:
     }
 
     UTILS_NOINLINE
-    void create(size_t n, Entity* entities) {
+    void create(size_t const n, Entity* entities) {
         Entity::Type index{};
         auto& freeList = mFreeList;
         uint8_t const* const gens = mGens;
 
         // this must be thread-safe, acquire the free-list mutex
-        std::lock_guard<Mutex> const lock(mFreeListLock);
+        std::lock_guard const lock(mFreeListLock);
         Entity::Type currentIndex = mCurrentIndex;
         for (size_t i = 0; i < n; i++) {
             if (UTILS_UNLIKELY(currentIndex >= RAW_INDEX_COUNT || freeList.size() >= MIN_FREE_INDICES)) {
@@ -98,11 +122,11 @@ public:
     }
 
     UTILS_NOINLINE
-    void destroy(size_t n, Entity* entities) noexcept {
+    void destroy(size_t const n, Entity* entities) noexcept {
         auto& freeList = mFreeList;
         uint8_t* const gens = mGens;
 
-        std::unique_lock<Mutex> lock(mFreeListLock);
+        std::unique_lock lock(mFreeListLock);
         for (size_t i = 0; i < n; i++) {
             if (!entities[i]) {
                 // behave like free(), ok to free null Entity.
@@ -110,19 +134,15 @@ public:
             }
 
             // it's an error to delete an Entity twice...
-            assert(isAlive(entities[i]));
+            bool const isAlive = getGeneration(entities[i]) == mGens[getIndex(entities[i])];
+            assert(isAlive);
 
             // ... deleting a dead Entity will corrupt the internal state, so we protect ourselves
             // against it. We don't guarantee anything about external state -- e.g. the listeners
             // will be called.
-            if (isAlive(entities[i])) {
+            if (UTILS_LIKELY(isAlive)) {
                 Entity::Type const index = getIndex(entities[i]);
                 freeList.push_back(index);
-
-                // The generation update doesn't require the lock because it's only used for isAlive()
-                // and entities work as weak references -- it just means that isAlive() could return
-                // true a little longer than expected in some other threads.
-                // We do need a memory fence though, it is provided by the mFreeListLock.unlock() below.
                 gens[index]++;
 
 #if FILAMENT_UTILS_TRACK_ENTITIES
@@ -151,22 +171,22 @@ public:
     }
 
     void registerListener(Listener* l) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mListeners.insert(l);
     }
 
     void unregisterListener(Listener* l) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mListeners.erase(l);
     }
 
     void registerChangeCallback(void const* token, ChangeCallback callback) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mChangeCallbacks.push_back({token, std::move(callback)});
     }
 
     void unregisterChangeCallback(void const* token) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mChangeCallbacks.erase(
                 std::remove_if(mChangeCallbacks.begin(), mChangeCallbacks.end(),
                         [token](auto const& info) { return info.token == token; }),
@@ -174,7 +194,7 @@ public:
     }
 
     void flushNotifications() noexcept {
-        std::unique_lock<Mutex> lock(mFreeListLock);
+        std::unique_lock lock(mFreeListLock);
         if (mDirtyCount > 0) {
             Entity localBuffer[MAX_DIRTY_COUNT];
             assert_invariant(mDirtyCount <= MAX_DIRTY_COUNT);
@@ -208,7 +228,7 @@ public:
 
 private:
     std::vector<ChangeCallback> getChangeCallbacks() const noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         std::vector<ChangeCallback> result;
         result.reserve(mChangeCallbacks.size());
         for (auto const& info : mChangeCallbacks) {
@@ -217,7 +237,7 @@ private:
         return result;
     }
 
-    void triggerChangeCallbacks(Entity const* entities, size_t n) const noexcept {
+    void triggerChangeCallbacks(Entity const* entities, size_t const n) const noexcept {
         auto const callbacks = getChangeCallbacks();
         Slice const slice(entities, n);
         for (auto const& callback : callbacks) {
@@ -226,7 +246,7 @@ private:
     }
 
     FixedCapacityVector<Listener*> getListeners() const noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         tsl::robin_set<Listener*> const& listeners = mListeners;
         FixedCapacityVector<Listener*> result(listeners.size());
         result.resize(result.capacity()); // unfortunately this memset()
@@ -234,22 +254,20 @@ private:
         return result; // the c++ standard guarantees a move
     }
 
-    uint32_t mCurrentIndex = 1;
-
-    // stores indices that got freed
-    mutable Mutex mFreeListLock;
-    std::deque<Entity::Type> mFreeList;
-
-    mutable Mutex mListenerLock;
-    tsl::robin_set<Listener*> mListeners;
-
+    static constexpr size_t MAX_DIRTY_COUNT = 16;
     struct CallbackInfo {
         void const* token;
         ChangeCallback callback;
     };
-    std::vector<CallbackInfo> mChangeCallbacks;
 
-    static constexpr size_t MAX_DIRTY_COUNT = 16;
+    mutable Mutex mFreeListLock;
+    uint32_t mCurrentIndex = 1;
+    std::deque<Entity::Type> mFreeList;     // stores indices that got freed
+    uint8_t* const mGens;                   // stores the generation of each index.
+
+    mutable Mutex mListenerLock;
+    tsl::robin_set<Listener*> mListeners;
+    std::vector<CallbackInfo> mChangeCallbacks;
     Entity mDirtyEntities[MAX_DIRTY_COUNT];
     size_t mDirtyCount = 0;