[Fix] (MG_Backend/DirectGLES, MG_State, Docs): CopyImageSubData held a registry reference across a re-entrant sync; retarget container rationales at the new erase contract

MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp

@@ -605,10 +605,11 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // Cached address of g_xfbObjects[g_currentXfbName]: PrepareForDraw consults
             // CurrentXfb on EVERY draw (StartPendingTransformFeedback) and the map
             // lookup was pure per-draw overhead for the overwhelmingly common no-capture
-            // case. FastSTL's open addressing keeps values in the bucket array, so ANY
-            // insert can rehash and move them (and erase/clear can too): every site that
-            // mutates the map or rebinds the current name resets this to null instead of
-            // reasoning about stability, and CurrentXfb re-resolves lazily.
+            // case. Open addressing keeps values in the bucket array, so ANY insert can
+            // rehash and move them - and erase moves them too, by shifting the rest of the
+            // probe cluster into the hole, which reaches entries other than the erased one.
+            // Every site that mutates the map or rebinds the current name resets this to
+            // null instead of reasoning about stability, and CurrentXfb re-resolves lazily.
             XfbObjectState* g_currentXfbState = nullptr;
 
             XfbObjectState& CurrentXfb() {
@@ -883,7 +884,7 @@ namespace MobileGL::MG_Backend::DirectGLES {
             if (it->second.esId != 0 && g_GLESFuncs.glDeleteTransformFeedbacks != nullptr) {
                 g_GLESFuncs.glDeleteTransformFeedbacks(1, &it->second.esId);
             }
-            g_currentXfbState = nullptr; // erase can move values (open addressing)
+            g_currentXfbState = nullptr; // erase shifts the probe cluster, moving other entries
             g_xfbObjects.erase(it);
             // The frontend reverts to the default object when the bound one is deleted.
             if (g_currentXfbName == name) {
@@ -5172,8 +5173,18 @@ namespace MobileGL::MG_Backend::DirectGLES {
                           const SharedPtr<MG_State::GLState::ITextureObject>& dstTexture,
                           GLenum dstTarget, GLint dstLevel, GLint dstX, GLint dstY, GLint dstZ,
                           GLsizei srcWidth, GLsizei srcHeight, GLsizei srcDepth) {
-        auto& srcBackendTexture = TextureImpl::SyncTextureObjectToBackend(srcTexture);
-        auto& dstBackendTexture = TextureImpl::SyncTextureObjectToBackend(dstTexture);
+        // BY VALUE, not by reference. SyncTextureObjectToBackend hands back a reference to a
+        // slot inside the backend texture registry, and the second call mutates that very map:
+        // GetOrCreate indexes it (an insert rehashes and moves every entry), and Find drops any
+        // entry whose state object has expired - which, with the map open-addressed and erasing
+        // by shifting the probe cluster backwards, relocates entries other than the erased one.
+        // Either way a reference taken by the first call is stale by the time the second returns,
+        // and it is read four more times below. Copying the SharedPtr costs two refcount bumps on
+        // a path that is already doing a texture copy.
+        const SharedPtr<TextureImpl::BackendTextureObject> srcBackendTexture =
+            TextureImpl::SyncTextureObjectToBackend(srcTexture);
+        const SharedPtr<TextureImpl::BackendTextureObject> dstBackendTexture =
+            TextureImpl::SyncTextureObjectToBackend(dstTexture);
 
         const Bool srcIsDepth = MG_Util::IsDepthFormatInternalFormat(srcTexture->GetFormat());
         const Bool dstIsDepth = MG_Util::IsDepthFormatInternalFormat(dstTexture->GetFormat());

MobileGL/MG_Backend/DirectGLES/Managers.h

@@ -129,6 +129,11 @@ namespace MobileGL::MG_Backend::DirectGLES {
 
         // Null when no live state object owns this key. The result points into the map, so
         // it stays valid only until the next GetOrCreate/Find/CollectGarbage on this registry.
+        // Take that literally, including for Find: the map is open-addressed and erases by
+        // shifting the rest of the probe cluster into the hole, so an erase relocates entries
+        // OTHER than the erased one - and Find erases, whenever it lands on a key whose state
+        // object has expired. Callers that need the twin across another registry call must copy
+        // the BackendPtr out (or keep only the pointee, which is heap-allocated and never moves).
         BackendPtr* Find(StateObject* stateObj) {
             const auto entryIt = m_entries.find(stateObj);
             if (entryIt == m_entries.end()) {

MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp

@@ -805,8 +805,8 @@ namespace MobileGL::MG_Backend::DirectVulkan {
         // Resolve the index BEFORE taking the reference, and bounds-check the way the
         // sibling getter does. GetShaderStorageBlockIndex re-enters GetProgramResourceCache,
         // which indexes g_programResourceCaches and can therefore insert - and that map is
-        // FastSTL's open-addressed unordered_map, whose rehash MOVES its buckets, so a
-        // reference taken before the call is left dangling. Binding a program's storage block
+        // open-addressed, so a rehash MOVES its entries and a reference taken before the
+        // call is left dangling. Binding a program's storage block
         // while another program's entry was still absent from the cache was a reproducible
         // segfault (ProgramPipelineScenario's two storage-block cases, in one process).
         const GLuint blockIndex = GetShaderStorageBlockIndex(*programObject, storageBlockName);

MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h

@@ -111,10 +111,12 @@ namespace MobileGL::MG_Backend::DirectVulkan {
 
         const VulkanRendererConfig& m_config;
         VkPhysicalDevice m_physicalDevice = VK_NULL_HANDLE;
-        // Values are heap-allocated: FastSTL::unordered_map is open-addressing,
-        // so INSERT invalidates references to stored values. The draw path (and
-        // the VAOs' state-pointer memos) hold entry pointers across inserts;
-        // only the unique_ptr cell moves, never the pointee.
+        // Values are heap-allocated: UnorderedMap is open-addressing, so INSERT
+        // invalidates references to stored values - and so does ERASE, which shifts
+        // the rest of the probe cluster into the hole and therefore moves entries
+        // other than the erased one. The draw path (and the VAOs' state-pointer
+        // memos) hold entry pointers across both; only the unique_ptr cell moves,
+        // never the pointee.
         UnorderedMap<HashType, UniquePtr<BackendVertexInputState>> m_cache;
         // Monotonic frame-boundary counter (bumped in OnFrameBoundary) for cache aging.
         Uint64 m_frameBoundaryCounter = 0;

MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h

@@ -315,26 +315,30 @@ namespace MobileGL::MG_Backend::DirectVulkan {
             Uint64 deferredAtFrame = 0;
         };
 
-        // Node-based std::unordered_map, deliberately not FastSTL's open-addressing UnorderedMap:
+        // Node-based std::unordered_map, deliberately NOT the open-addressing UnorderedMap:
         // callers cache a RenderbufferResource* - or a bare &resource->layout - and then make further
         // calls that touch this map. BlitFramebuffer is the one that bit: it resolves the source and
         // destination colour bindings (ResolveColorBlitBinding caches &rbResource->layout), then
-        // materializes the source's pending clear, which looks that same resource up again. FastSTL's
-        // operator[] runs its load-factor check before find_key and reallocates the whole bucket array
-        // when occupancy crosses it, so even a plain lookup relocates every element; erase only
-        // tombstones and never decrements the occupancy, so the doubling keeps firing. After a
-        // relocation the cached pointer names freed storage still holding the pre-clear
-        // VK_IMAGE_LAYOUT_UNDEFINED, and BlitFramebuffer bails out at "source image layout is
-        // undefined", silently dropping the blit - renderbuffers_storage_multisample read back zero
-        // instead of the clear colour on exactly the iterations that grew the table.
+        // materializes the source's pending clear, which looks that same resource up again. Growing
+        // an open-addressed table relocates every element, so the cached pointer went on to name
+        // freed storage still holding the pre-clear VK_IMAGE_LAYOUT_UNDEFINED; BlitFramebuffer bailed
+        // out at "source image layout is undefined", silently dropping the blit -
+        // renderbuffers_storage_multisample read back zero instead of the clear colour on exactly the
+        // iterations that grew the table.
         //
         // Reordering the materialize ahead of the resolves - the fix ReadPixels got - does not cover
         // this: the destination resolve still runs after the source pointer is taken. The depth blit,
         // GetOrCreateRenderPass's depthRenderbufferResource and ReadDepthStencilPixels cache the same
         // kind of pointer, so the invariant belongs in the container rather than in a per-call-site
-        // ordering rule. m_textureResources is node-based for the same reason. This buys stability
-        // across rehash and insert only - erase still invalidates the erased element, which is safe
-        // here because a renderbuffer that is an FBO attachment is held alive by that attachment.
+        // ordering rule. m_textureResources is node-based for the same reason.
+        //
+        // The case for keeping this node-based got STRONGER with ska::flat_hash_map, so do not read
+        // the paragraph above as merely historical: ska erases by shifting the rest of the probe
+        // cluster backwards into the hole, so erasing one renderbuffer relocates OTHER renderbuffers'
+        // entries - a cached pointer can now be invalidated by a key it has nothing to do with, which
+        // no call-site ordering rule can defend against. (What did change: ska's operator[] returns on
+        // a hit before it runs its grow check, so a plain lookup of a PRESENT key no longer relocates.
+        // That narrows the insert hazard; it does not touch the erase one.)
         std::unordered_map<MG_State::GLState::RenderbufferObject*, RenderbufferResource> m_renderbufferResources;
         UnorderedMap<MG_State::GLState::RenderbufferObject*, PendingRenderbufferClear> m_pendingRenderbufferClears;
         Vector<DeferredRenderbufferRelease> m_deferredRenderbufferReleases;

MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp

@@ -8660,7 +8660,7 @@ void main() {
         // blit binding below: for a renderbuffer/texture that has never been part of any
         // render pass yet (e.g. a GL_NONE draw buffer slot whose attachment is only ever
         // touched via an explicit glReadBuffer), materializing lazily creates its backing
-        // Vulkan resource for the first time. UnorderedMap (FastSTL, open-addressing) may
+        // Vulkan resource for the first time. UnorderedMap is open-addressing and may
         // rehash on that insertion, invalidating any RenderbufferResource*/TextureResource*
         // obtained beforehand - so ResolveColorBlitBinding's cached `trackedLayout` pointer
         // must be taken AFTER this, never before it.

MobileGL/MG_State/GLState/BufferState/BufferState.cpp

@@ -67,9 +67,13 @@ namespace MobileGL::MG_State::GLState {
                         }
                     }
                 }
-                // Key-based erase skips FastSTL's successor-iterator scan, which is
-                // pure overhead here and dominates delete-heavy frames.
-                m_bufferObjects.erase(index);
+                // Erase through the iterator already in hand: erase(key) would repeat the
+                // find() from line 55, and the successor scan that once made key-based
+                // erase the cheaper of the two no longer happens here - erase(iterator)
+                // hands back an unconverted proxy, and the scan is what converting it
+                // would cost. The unbind loops above touch only the binding arrays, so
+                // `it` is still live.
+                m_bufferObjects.erase(it);
             }
             m_indexGenerator.Delete(index);
         }

MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp

@@ -70,9 +70,10 @@ namespace MobileGL::MG_State::GLState {
     void ShaderCompileAdoptionMap::SweepIfCrowded() {
         if (m_entries.size() < m_sweepThreshold) return;
 
-        // Collect first, erase after: FastSTL::unordered_map is open-addressed, so erasing
-        // through an iterator that the same loop is still advancing is not worth reasoning
-        // about on a path this cold.
+        // Collect first, erase after: the map is open-addressed and erases by shifting the
+        // rest of the probe cluster into the hole, so an erase moves entries other than the
+        // erased one. Copying the keys out sidesteps that entirely, and this path is cold
+        // enough that the extra vector is not worth reasoning about the alternative.
         Vector<ShaderSourceKey> dead;
         for (const auto& entry : m_entries) {
             const SharedPtr<ShaderCompileTask> node = entry.second.lock();

MobileGL/MG_Test/Program/ProgramUtilTest.cpp

@@ -2694,8 +2694,7 @@ void main() {
     ASSERT_TRUE(shaderResult) << shaderResult.error().log;
 
     // PARTIALLY bound, and deliberately not a dense 0..N run - exactly what Iris does.
-    // mc_midTexCoord and a_Unreferenced are left unbound (FastSTL's map has no
-    // initializer-list constructor, hence the explicit inserts).
+    // mc_midTexCoord and a_Unreferenced are left unbound.
     UnorderedMap<String, Uint> explicitVertexIns;
     explicitVertexIns["a_Position"] = 0;
     explicitVertexIns["a_Color"] = 1;

README.md

@@ -42,6 +42,7 @@ MobileGL reuses several open-source projects:
 * **SPIRV-Cross** by **KhronosGroup** - [Apache License 2.0](https://github.com/KhronosGroup/SPIRV-Cross/blob/master/LICENSE): [github](https://github.com/KhronosGroup/SPIRV-Cross)
 * **glslang** by **KhronosGroup** - [Various Licenses](https://github.com/KhronosGroup/glslang/blob/main/LICENSE.txt): [github](https://github.com/KhronosGroup/glslang)
 * **DiligentCore** by **Diligent Graphics** - [Apache License 2.0](https://github.com/DiligentGraphics/DiligentCore/blob/master/License.txt): [github](https://github.com/DiligentGraphics/DiligentCore)
+* **flat_hash_map** by **Malte Skarupke** - [Boost Software License 1.0](https://github.com/MobileGL-Dev/flat_hash_map/blob/master/LICENSE): [github](https://github.com/MobileGL-Dev/flat_hash_map)
 
 Refer to each component's repository for exact license texts. Any bundled third-party code in this repository is included under the upstream project's license.