From 21ec744ef22c32b83062616c94a12c718088969d Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 11 Aug 2026 23:31:00 -0400 Subject: [PATCH] [Refactor] (MG_Util, MG_Test): retire FastSTL for ska::flat_hash_map, the table MobileGlues settled on --- .gitmodules | 6 ++--- MobileGL/Includes.h | 4 ++-- MobileGL/MG_Test/SanityTest.cpp | 42 ++++++++++++++++++++------------- MobileGL/MG_Util/Types.h | 18 ++++++++++++-- include/FastSTL | 1 - include/ska | 1 + 6 files changed, 48 insertions(+), 24 deletions(-) delete mode 160000 include/FastSTL create mode 160000 include/ska diff --git a/.gitmodules b/.gitmodules index 5bebb9c2..3ae968ea 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,9 +7,6 @@ [submodule "3rdparty/SPIRV-Cross"] path = 3rdparty/SPIRV-Cross url = https://github.com/KhronosGroup/SPIRV-Cross.git -[submodule "include/FastSTL"] - path = include/FastSTL - url = https://github.com/MobileGL-Dev/FastSTL.git [submodule "3rdparty/tracy"] path = 3rdparty/tracy url = https://github.com/wolfpld/tracy.git @@ -34,3 +31,6 @@ [submodule "3rdparty/asio"] path = 3rdparty/asio url = https://github.com/chriskohlhoff/asio.git +[submodule "include/ska"] + path = include/ska + url = https://github.com/MobileGL-Dev/flat_hash_map.git diff --git a/MobileGL/Includes.h b/MobileGL/Includes.h index d3a3eff5..50c3d9d6 100644 --- a/MobileGL/Includes.h +++ b/MobileGL/Includes.h @@ -49,8 +49,8 @@ #include #endif -// Include FastSTL -#include +// Include ska::flat_hash_map +#include // Include xxHash #include diff --git a/MobileGL/MG_Test/SanityTest.cpp b/MobileGL/MG_Test/SanityTest.cpp index 6a7b9f3f..6a3be004 100644 --- a/MobileGL/MG_Test/SanityTest.cpp +++ b/MobileGL/MG_Test/SanityTest.cpp @@ -33,7 +33,7 @@ #include #include #include -#include +#include namespace { class DynamicParameterBackend final : public MobileGL::MG_Backend::BackendObject { @@ -1998,16 +1998,20 @@ TEST(DirectGLESStateGuards, DefaultFramebufferBindGoesThroughShadow) { EXPECT_EQ(mocks.log.Count("BindFramebuffer:"), 3u); } -// FastSTL::unordered_map::erase(iterator) regression coverage. The open-addressing -// iterator constructor snaps forward from a tombstoned slot to the successor, so -// erase must NOT advance the rebuilt iterator again: the old double-advance skipped -// one live element per erase, and erasing the element in the highest occupied -// bucket pushed the returned index past bucket_count where it never compared equal -// to end() again - erase-while-iterating sweeps (pipeline/program cache eviction) -// then ran off the bucket array and fed garbage handles to vkDestroyPipeline -// (device crash on first mass eviction during world load). -TEST(FastSTLSanity, EraseWhileIteratingVisitsEveryElementExactlyOnce) { - FastSTL::unordered_map map; +// UnorderedMap::erase(iterator) contract coverage. Erase-while-iterating sweeps +// (pipeline/program cache eviction) depend on `it = map.erase(it)` naming the next +// live element exactly once: a sweep that skips entries leaks them, and one that +// runs off the end feeds garbage handles to vkDestroyPipeline (device crash on the +// first mass eviction during world load - the failure FastSTL's double-advancing +// erase actually produced before it was fixed). +// +// These pin the behaviour the call sites rely on, not one map's implementation, so +// they are written against MobileGL::UnorderedMap and survive changing what it +// names. Under ska::flat_hash_map the mechanism is different - erase backward-shifts +// the rest of the probe cluster into the hole and hands back the same slot, which +// now holds the shifted-in successor - but the observable contract is the same. +TEST(UnorderedMapSanity, EraseWhileIteratingVisitsEveryElementExactlyOnce) { + MobileGL::UnorderedMap map; constexpr MobileGL::Uint64 kCount = 1000; for (MobileGL::Uint64 key = 0; key < kCount; ++key) { map.emplace(key * 0x9e3779b97f4a7c15ull, key); @@ -2024,8 +2028,8 @@ TEST(FastSTLSanity, EraseWhileIteratingVisitsEveryElementExactlyOnce) { EXPECT_EQ(map.size(), 0u); } -TEST(FastSTLSanity, EraseReturnsTheSuccessorElement) { - FastSTL::unordered_map map; +TEST(UnorderedMapSanity, EraseReturnsTheSuccessorElement) { + MobileGL::UnorderedMap map; for (MobileGL::Uint32 key = 1; key <= 64; ++key) { map.emplace(key, key); } @@ -2048,10 +2052,16 @@ TEST(FastSTLSanity, EraseReturnsTheSuccessorElement) { EXPECT_EQ(map.size(), 64u - erased); } -TEST(FastSTLSanity, ErasingTheOnlyElementReturnsEnd) { - FastSTL::unordered_map map; +TEST(UnorderedMapSanity, ErasingTheOnlyElementReturnsEnd) { + using Map = MobileGL::UnorderedMap; + Map map; map.emplace(42u, 1u); - auto next = map.erase(map.begin()); + + // Spell the type: erase(iterator) hands back a proxy that is convertible to an + // iterator but is not one, because finding the next element is not free and the + // callers that discard the result should not pay for it. `auto next = ...` binds + // the proxy instead, and then nothing it is compared against compiles. + Map::iterator next = map.erase(map.begin()); EXPECT_EQ(next, map.end()); EXPECT_TRUE(map.empty()); } diff --git a/MobileGL/MG_Util/Types.h b/MobileGL/MG_Util/Types.h index d34443f1..da680069 100644 --- a/MobileGL/MG_Util/Types.h +++ b/MobileGL/MG_Util/Types.h @@ -55,9 +55,23 @@ namespace MobileGL { using SizeT = std::size_t; template using Array = std::array; + // ska::flat_hash_map, the same table MobileGlues settled on, at the same commit. + // + // Open addressing with robin-hood probing, so a rehash moves the elements: any + // insert, emplace, operator[], reserve or rehash invalidates every iterator, + // reference and pointer into the map. Erase does too, and less obviously - + // deletion shifts the rest of the probe cluster backwards, so erasing one key + // can move a DIFFERENT key's element. Where a mapped value's address has to + // outlive later mutation, the map holds a UniquePtr/SharedPtr and the pointee + // stays put; those sites say so where they are declared. + // + // Its value_type is pair with the key exposed mutably, so `it->first =` + // compiles and silently corrupts the table - the one sharp edge this map has + // that a node-based one does not. Note the Allocator default matches that + // value_type: pair, not pair. template , class KeyEqual = std::equal_to, - class Allocator = std::allocator>> - using UnorderedMap = FastSTL::unordered_map; + class Allocator = std::allocator>> + using UnorderedMap = ska::flat_hash_map; template inline constexpr std::remove_reference_t&& Move(T&& t) noexcept { return static_cast&&>(t); diff --git a/include/FastSTL b/include/FastSTL deleted file mode 160000 index 022211c9..00000000 --- a/include/FastSTL +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 022211c9983c70daf86d7d4cfbdb017eb1598c81 diff --git a/include/ska b/include/ska new file mode 160000 index 00000000..21c1cec9 --- /dev/null +++ b/include/ska @@ -0,0 +1 @@ +Subproject commit 21c1cec95abee1beef827e4a7c95f692875d9594