sparse_set/storage:

* reintroduce the swap_and_pop vs in_place_pop split for perf reasons * rollback to a model with two iterators on virtual functions * even faster range-erase/clear
2022-01-28 08:31:14 +01:00
parent 2bba6d4c0d
commit 57de1187fc
5 changed files with 185 additions and 78 deletions
--- a/3
+++ b/3
@@ -4,9 +4,8 @@
 * add examples (and credits) from @alanjfs :)

 WIP:
-* reintroduce swap-and-pop vs in-place-pop difference for perf reasons
-* iterator based try_emplace vs try_insert for perf reasons
 * make clear check free_list, then set it to null and shrink
+* iterator based try_emplace vs try_insert for perf reasons
 * runtime events (dispatcher/emitter), runtime context variables...
 * registry: remove reference to basic_sparse_set<E>
 * dedicated entity storage, in-place O(1) release/destroy for non-orphaned entities, out-of-sync model
--- a/src/entt/entity/sparse_set.hpp
+++ b/src/entt/entity/sparse_set.hpp
@@ -234,26 +234,30 @@ protected:

    /**
     * @brief Erases entities from a sparse set.
-     * @param it Iterator to the first element to erase.
-     * @param count Number of elements to erase.
+     * @param first An iterator to the first element of the range of entities.
+     * @param last An iterator past the last element of the range of entities.
     */
-    virtual void try_erase(basic_iterator it, const std::size_t count) {
-        if(mode == deletion_policy::in_place) {
-            for(const auto last = it + count; it != last; ++it) {
-                sparse_ref(*it) = null;
-                packed[it.index()] = std::exchange(free_list, entity_traits::combine(static_cast<typename entity_traits::entity_type>(it.index()), entity_traits::reserved));
-            }
-        } else {
-            for(const auto last = it + count; it != last; ++it) {
-                auto &ref = sparse_ref(*it);
-                packed[it.index()] = packed.back();
-                sparse_ref(packed.back()) = entity_traits::combine(static_cast<typename entity_traits::entity_type>(it.index()), entity_traits::to_integral(packed.back()));
-                // unnecessary but it helps to detect nasty bugs
-                ENTT_ASSERT((packed.back() = tombstone, true), "");
-                packed.pop_back();
-                // lazy self-assignment guard
-                ref = null;
-            }
+    virtual void swap_and_pop(basic_iterator first, basic_iterator last) {
+        for(; first != last; ++first) {
+            sparse_ref(packed.back()) = entity_traits::combine(static_cast<typename entity_traits::entity_type>(first.index()), entity_traits::to_integral(packed.back()));
+            const auto entt = std::exchange(packed[first.index()], packed.back());
+            // unnecessary but it helps to detect nasty bugs
+            ENTT_ASSERT((packed.back() = tombstone, true), "");
+            // lazy self-assignment guard
+            sparse_ref(entt) = null;
+            packed.pop_back();
+        }
+    }
+
+    /**
+     * @brief Erases entities from a sparse set.
+     * @param first An iterator to the first element of the range of entities.
+     * @param last An iterator past the last element of the range of entities.
+     */
+    virtual void in_place_pop(basic_iterator first, basic_iterator last) {
+        for(; first != last; ++first) {
+            sparse_ref(*first) = null;
+            packed[first.index()] = std::exchange(free_list, entity_traits::combine(static_cast<typename entity_traits::entity_type>(first.index()), entity_traits::reserved));
        }
    }

@@ -688,7 +692,8 @@ public:
     * @param entt A valid identifier.
     */
    void erase(const entity_type entt) {
-        try_erase(--(end() - index(entt)), 1u);
+        const auto it = --(end() - index(entt));
+        (mode == deletion_policy::in_place) ? in_place_pop(it, it + 1u) : swap_and_pop(it, it + 1u);
    }

    /**
@@ -703,7 +708,7 @@ public:
    template<typename It>
    void erase(It first, It last) {
        if constexpr(std::is_same_v<It, basic_iterator>) {
-            try_erase(first, static_cast<size_type>(std::distance(first, last)));
+            (mode == deletion_policy::in_place) ? in_place_pop(first, last) : swap_and_pop(first, last);
        } else {
            for(; first != last; ++first) {
                erase(*first);
@@ -895,7 +900,7 @@ public:
    /*! @brief Clears a sparse set. */
    void clear() {
        if(free_list == null) {
-            try_erase(begin(), size());
+            erase(begin(), end());
        } else {
            for(auto &&entity: *this) {
                // tombstone filter
--- a/src/entt/entity/storage.hpp
+++ b/src/entt/entity/storage.hpp
@@ -277,7 +277,12 @@ class basic_storage: public basic_sparse_set<Entity, typename std::allocator_tra
            alloc_traits::construct(packed.second(), to_address(elem), std::forward<Args>(args)...);
        }
        ENTT_CATCH {
-            base_type::try_erase(it, 1u);
+            if constexpr(comp_traits::in_place_delete) {
+                base_type::in_place_pop(it, it + 1u);
+            } else {
+                base_type::swap_and_pop(it, it + 1u);
+            }
+
            ENTT_THROW;
        }

@@ -324,16 +329,28 @@ private:
 protected:
    /**
     * @brief Erases elements from a storage.
-     * @param it Iterator to the first element to erase.
-     * @param count Number of elements to erase.
+     * @param first An iterator to the first element to erase.
+     * @param last An iterator past the last element to erase.
     */
-    void try_erase(typename underlying_type::basic_iterator it, const std::size_t count) override {
-        for(const auto last = it + count; it != last; ++it) {
-            const auto pos = static_cast<size_type>(it.index());
-            auto &elem = element_at(comp_traits::in_place_delete ? pos : (base_type::size() - 1u));
-            [[maybe_unused]] auto unused = std::exchange(element_at(pos), std::move(elem));
+    void swap_and_pop(typename underlying_type::basic_iterator first, typename underlying_type::basic_iterator last) override {
+        for(; first != last; ++first) {
+            auto &elem = element_at(base_type::size() - 1u);
+            // destroying on exit allows reentrant destructors
+            [[maybe_unused]] auto unused = std::exchange(element_at(static_cast<size_type>(first.index())), std::move(elem));
            std::destroy_at(std::addressof(elem));
-            base_type::try_erase(it, 1u);
+            base_type::swap_and_pop(first, first + 1u);
+        }
+    }
+
+    /**
+     * @brief Erases elements from a storage.
+     * @param first An iterator to the first element to erase.
+     * @param last An iterator past the last element to erase.
+     */
+    void in_place_pop(typename underlying_type::basic_iterator first, typename underlying_type::basic_iterator last) override {
+        for(; first != last; ++first) {
+            base_type::in_place_pop(first, first + 1u);
+            std::destroy_at(std::addressof(element_at(static_cast<size_type>(first.index()))));
        }
    }

@@ -893,16 +910,26 @@ public:
 */
 template<typename Type>
 class sigh_storage_mixin final: public Type {
-    void try_erase(typename Type::basic_iterator it, const std::size_t count) override {
+    template<typename Func>
+    void notify_destruction(typename Type::basic_iterator first, typename Type::basic_iterator last, Func func) {
        ENTT_ASSERT(owner != nullptr, "Invalid pointer to registry");

-        for(const auto last = it + count; it != last; ++it) {
-            const auto entt = *it;
+        for(; first != last; ++first) {
+            const auto entt = *first;
            destruction.publish(*owner, entt);
-            Type::try_erase(Type::find(entt), 1u);
+            const auto it = Type::find(entt);
+            func(it, it + 1u);
        }
    }

+    void swap_and_pop(typename Type::basic_iterator first, typename Type::basic_iterator last) final {
+        notify_destruction(std::move(first), std::move(last), [this](auto... args) { Type::swap_and_pop(args...); });
+    }
+
+    void in_place_pop(typename Type::basic_iterator first, typename Type::basic_iterator last) final {
+        notify_destruction(std::move(first), std::move(last), [this](auto... args) { Type::in_place_pop(args...); });
+    }
+
    typename Type::basic_iterator try_emplace(const typename Type::entity_type entt, const bool force_back, const void *value) final {
        ENTT_ASSERT(owner != nullptr, "Invalid pointer to registry");
        Type::try_emplace(entt, force_back, value);
--- a/test/entt/entity/sigh_storage_mixin.cpp
+++ b/test/entt/entity/sigh_storage_mixin.cpp
@@ -8,7 +8,7 @@
 struct empty_type {};

 struct stable_type {
-    int value;
+    int value{};
 };

 struct non_default_constructible {
@@ -17,7 +17,7 @@ struct non_default_constructible {
    non_default_constructible(int v)
        : value{v} {}

-    int value;
+    int value{};
 };

 template<>
@@ -34,9 +34,9 @@ void listener(counter &counter, entt::registry &, entt::entity) {
 }

 TEST(SighStorageMixin, GenericType) {
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
    entt::sigh_storage_mixin<entt::storage<int>> pool;
    entt::sparse_set &base = pool;
-    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
    entt::registry registry{};

    pool.bind(entt::forward_as_any(registry));
@@ -99,10 +99,76 @@ TEST(SighStorageMixin, GenericType) {
    ASSERT_TRUE(pool.empty());
 }

+TEST(SighStorageMixin, StableType) {
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
+    entt::sigh_storage_mixin<entt::storage<stable_type>> pool;
+    entt::sparse_set &base = pool;
+    entt::registry registry{};
+
+    pool.bind(entt::forward_as_any(registry));
+
+    counter on_construct{};
+    counter on_destroy{};
+
+    pool.on_construct().connect<&listener>(on_construct);
+    pool.on_destroy().connect<&listener>(on_destroy);
+
+    ASSERT_NE(base.emplace(entities[0u]), base.end());
+
+    pool.emplace(entities[1u]);
+
+    ASSERT_EQ(on_construct.value, 2);
+    ASSERT_EQ(on_destroy.value, 0);
+    ASSERT_FALSE(pool.empty());
+
+    ASSERT_EQ(pool.get(entities[0u]).value, 0);
+    ASSERT_EQ(pool.get(entities[1u]).value, 0);
+
+    base.erase(entities[0u]);
+    pool.erase(entities[1u]);
+
+    ASSERT_EQ(on_construct.value, 2);
+    ASSERT_EQ(on_destroy.value, 2);
+    ASSERT_FALSE(pool.empty());
+
+    ASSERT_NE(base.insert(std::begin(entities), std::end(entities)), base.end());
+
+    ASSERT_EQ(pool.get(entities[0u]).value, 0);
+    ASSERT_EQ(pool.get(entities[1u]).value, 0);
+    ASSERT_FALSE(pool.empty());
+
+    base.erase(entities[1u]);
+
+    ASSERT_EQ(on_construct.value, 4);
+    ASSERT_EQ(on_destroy.value, 3);
+    ASSERT_FALSE(pool.empty());
+
+    base.erase(entities[0u]);
+
+    ASSERT_EQ(on_construct.value, 4);
+    ASSERT_EQ(on_destroy.value, 4);
+    ASSERT_FALSE(pool.empty());
+
+    pool.insert(std::begin(entities), std::end(entities), stable_type{3});
+
+    ASSERT_EQ(on_construct.value, 6);
+    ASSERT_EQ(on_destroy.value, 4);
+    ASSERT_FALSE(pool.empty());
+
+    ASSERT_EQ(pool.get(entities[0u]).value, 3);
+    ASSERT_EQ(pool.get(entities[1u]).value, 3);
+
+    pool.erase(std::begin(entities), std::end(entities));
+
+    ASSERT_EQ(on_construct.value, 6);
+    ASSERT_EQ(on_destroy.value, 6);
+    ASSERT_FALSE(pool.empty());
+}
+
 TEST(SighStorageMixin, EmptyType) {
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
    entt::sigh_storage_mixin<entt::storage<empty_type>> pool;
    entt::sparse_set &base = pool;
-    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
    entt::registry registry{};

    pool.bind(entt::forward_as_any(registry));
@@ -166,9 +232,9 @@ TEST(SighStorageMixin, EmptyType) {
 }

 TEST(SighStorageMixin, NonDefaultConstructibleType) {
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
    entt::sigh_storage_mixin<entt::storage<non_default_constructible>> pool;
    entt::sparse_set &base = pool;
-    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
    entt::registry registry{};

    pool.bind(entt::forward_as_any(registry));
--- a/test/entt/entity/storage.cpp
+++ b/test/entt/entity/storage.cpp
@@ -1658,70 +1658,80 @@ TEST(Storage, CustomAllocator) {
        pool.clear();

        ASSERT_NE(pool.capacity(), 0u);
-        ASSERT_EQ(pool.size(), 0u);
+        ASSERT_EQ(pool.size(), pool.policy() == entt::deletion_policy::in_place ? 2u : 0u);

        pool.shrink_to_fit();

-        ASSERT_EQ(pool.capacity(), 0u);
+        ASSERT_EQ(pool.capacity(), pool.policy() == entt::deletion_policy::in_place ? ENTT_PACKED_PAGE : 0u);
    };

    test::throwing_allocator<entt::entity> allocator{};

    test(entt::basic_storage<entt::entity, int, test::throwing_allocator<int>>{allocator}, allocator);
    test(entt::basic_storage<entt::entity, std::true_type, test::throwing_allocator<std::true_type>>{allocator}, allocator);
+    test(entt::basic_storage<entt::entity, stable_type, test::throwing_allocator<stable_type>>{allocator}, allocator);
 }

 TEST(Storage, ThrowingAllocator) {
-    entt::basic_storage<entt::entity, int, test::throwing_allocator<int>> pool;
-    typename std::decay_t<decltype(pool)>::base_type &base = pool;
+    auto test = [](auto pool) {
+        using pool_allocator_type = typename decltype(pool)::allocator_type;
+        using value_type = typename decltype(pool)::value_type;

-    test::throwing_allocator<int>::trigger_on_allocate = true;
+        typename std::decay_t<decltype(pool)>::base_type &base = pool;

-    ASSERT_THROW(pool.reserve(1u), test::throwing_allocator<int>::exception_type);
-    ASSERT_EQ(pool.capacity(), 0u);
+        pool_allocator_type::trigger_on_allocate = true;

-    test::throwing_allocator<int>::trigger_after_allocate = true;
+        ASSERT_THROW(pool.reserve(1u), typename pool_allocator_type::exception_type);
+        ASSERT_EQ(pool.capacity(), 0u);

-    ASSERT_THROW(pool.reserve(2 * ENTT_PACKED_PAGE), test::throwing_allocator<int>::exception_type);
-    ASSERT_EQ(pool.capacity(), ENTT_PACKED_PAGE);
+        pool_allocator_type::trigger_after_allocate = true;

-    pool.shrink_to_fit();
+        ASSERT_THROW(pool.reserve(2 * ENTT_PACKED_PAGE), typename pool_allocator_type::exception_type);
+        ASSERT_EQ(pool.capacity(), ENTT_PACKED_PAGE);

-    ASSERT_EQ(pool.capacity(), 0u);
+        pool.shrink_to_fit();

-    test::throwing_allocator<entt::entity>::trigger_on_allocate = true;
+        ASSERT_EQ(pool.capacity(), 0u);

-    ASSERT_THROW(pool.emplace(entt::entity{0}, 0), test::throwing_allocator<entt::entity>::exception_type);
-    ASSERT_FALSE(pool.contains(entt::entity{0}));
-    ASSERT_TRUE(pool.empty());
+        test::throwing_allocator<entt::entity>::trigger_on_allocate = true;

-    test::throwing_allocator<entt::entity>::trigger_on_allocate = true;
+        ASSERT_THROW(pool.emplace(entt::entity{0}, 0), test::throwing_allocator<entt::entity>::exception_type);
+        ASSERT_FALSE(pool.contains(entt::entity{0}));
+        ASSERT_TRUE(pool.empty());

-    ASSERT_THROW(base.emplace(entt::entity{0}), test::throwing_allocator<entt::entity>::exception_type);
-    ASSERT_FALSE(base.contains(entt::entity{0}));
-    ASSERT_TRUE(base.empty());
+        test::throwing_allocator<entt::entity>::trigger_on_allocate = true;

-    test::throwing_allocator<int>::trigger_on_allocate = true;
+        ASSERT_THROW(base.emplace(entt::entity{0}), test::throwing_allocator<entt::entity>::exception_type);
+        ASSERT_FALSE(base.contains(entt::entity{0}));
+        ASSERT_TRUE(base.empty());

-    ASSERT_THROW(pool.emplace(entt::entity{0}, 0), test::throwing_allocator<int>::exception_type);
-    ASSERT_FALSE(pool.contains(entt::entity{0}));
-    ASSERT_TRUE(pool.empty());
+        pool_allocator_type::trigger_on_allocate = true;

-    pool.emplace(entt::entity{0}, 0);
-    const entt::entity entities[2u]{entt::entity{1}, entt::entity{ENTT_SPARSE_PAGE}};
-    test::throwing_allocator<entt::entity>::trigger_after_allocate = true;
+        ASSERT_THROW(pool.emplace(entt::entity{0}, 0), pool_allocator_type::exception_type);
+        ASSERT_FALSE(pool.contains(entt::entity{0}));
+        ASSERT_NO_THROW(pool.compact());
+        ASSERT_TRUE(pool.empty());

-    ASSERT_THROW(pool.insert(std::begin(entities), std::end(entities), 0), test::throwing_allocator<entt::entity>::exception_type);
-    ASSERT_TRUE(pool.contains(entt::entity{1}));
-    ASSERT_FALSE(pool.contains(entt::entity{ENTT_SPARSE_PAGE}));
+        pool.emplace(entt::entity{0}, 0);
+        const entt::entity entities[2u]{entt::entity{1}, entt::entity{ENTT_SPARSE_PAGE}};
+        test::throwing_allocator<entt::entity>::trigger_after_allocate = true;

-    pool.erase(entt::entity{1});
-    const int components[2u]{1, ENTT_SPARSE_PAGE};
-    test::throwing_allocator<entt::entity>::trigger_on_allocate = true;
+        ASSERT_THROW(pool.insert(std::begin(entities), std::end(entities), value_type{0}), test::throwing_allocator<entt::entity>::exception_type);
+        ASSERT_TRUE(pool.contains(entt::entity{1}));
+        ASSERT_FALSE(pool.contains(entt::entity{ENTT_SPARSE_PAGE}));

-    ASSERT_THROW(pool.insert(std::begin(entities), std::end(entities), std::begin(components)), test::throwing_allocator<entt::entity>::exception_type);
-    ASSERT_TRUE(pool.contains(entt::entity{1}));
-    ASSERT_FALSE(pool.contains(entt::entity{ENTT_SPARSE_PAGE}));
+        pool.erase(entt::entity{1});
+        const value_type components[2u]{value_type{1}, value_type{ENTT_SPARSE_PAGE}};
+        test::throwing_allocator<entt::entity>::trigger_on_allocate = true;
+        pool.compact();
+
+        ASSERT_THROW(pool.insert(std::begin(entities), std::end(entities), std::begin(components)), test::throwing_allocator<entt::entity>::exception_type);
+        ASSERT_TRUE(pool.contains(entt::entity{1}));
+        ASSERT_FALSE(pool.contains(entt::entity{ENTT_SPARSE_PAGE}));
+    };
+
+    test(entt::basic_storage<entt::entity, int, test::throwing_allocator<int>>{});
+    test(entt::basic_storage<entt::entity, stable_type, test::throwing_allocator<stable_type>>{});
 }

 TEST(Storage, ThrowingComponent) {