diff --git a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp b/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp index 444833bb..f06a1706 100644 --- a/MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp +++ b/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& 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 srcBackendTexture = + TextureImpl::SyncTextureObjectToBackend(srcTexture); + const SharedPtr dstBackendTexture = + TextureImpl::SyncTextureObjectToBackend(dstTexture); const Bool srcIsDepth = MG_Util::IsDepthFormatInternalFormat(srcTexture->GetFormat()); const Bool dstIsDepth = MG_Util::IsDepthFormatInternalFormat(dstTexture->GetFormat()); diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.h b/MobileGL/MG_Backend/DirectGLES/Managers.h index 594f8cc7..ac13dfc1 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.h +++ b/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()) { diff --git a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp b/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp index 046c8a9e..a870db33 100644 --- a/MobileGL/MG_Backend/DirectVulkan/DirectVulkan.cpp +++ b/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); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h index 6d307eee..e5642fbd 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VertexInputStateFactory.h +++ b/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> m_cache; // Monotonic frame-boundary counter (bumped in OnFrameBoundary) for cache aging. Uint64 m_frameBoundaryCounter = 0; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h index 0077d918..ed8ef277 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkRenderPassManager.h +++ b/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 m_renderbufferResources; UnorderedMap m_pendingRenderbufferClears; Vector m_deferredRenderbufferReleases; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 14281e58..62f812c0 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/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. diff --git a/MobileGL/MG_State/GLState/BufferState/BufferState.cpp b/MobileGL/MG_State/GLState/BufferState/BufferState.cpp index b9217f23..cd965d70 100644 --- a/MobileGL/MG_State/GLState/BufferState/BufferState.cpp +++ b/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); } diff --git a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp b/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp index b4f38401..0b64736e 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ShaderCompileAdoptionMap.cpp +++ b/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 dead; for (const auto& entry : m_entries) { const SharedPtr node = entry.second.lock(); diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 744fbf71..6994c1db 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/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 explicitVertexIns; explicitVertexIns["a_Position"] = 0; explicitVertexIns["a_Color"] = 1; diff --git a/README.md b/README.md index b7564ebb..693615d4 100644 --- a/README.md +++ b/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.