From d39a706d57bfdbe62b4cfdd5d04e76e08ca7a8d1 Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Wed, 5 Aug 2026 15:19:37 -0400 Subject: [PATCH] [Perf] (MG_Backend): stop paying for descriptor slots and mip barriers nobody asked for Five independent bits of per-draw and per-operation waste in the DirectVulkan backend, all removing work whose answer was already known. The per-draw descriptor walk iterated all 256 slots of bindingKinds to find the one to eight bindings a real GL program declares, because that vector is sized to the binding cap rather than to the program. Reflection now records the bindings it actually assigned, and the draw path iterates that. It is built at the end of ReflectLayout, not where bindingKinds is sized - at that point the vector is only zero-initialised and the kinds are assigned further down, so a list built there would be empty. It has to stay ascending: Vulkan consumes pDynamicOffsets in binding order and the writer pushes them in iteration order, so an unordered list would silently mis-pair dynamic offsets with their uniform blocks. Descriptor pools were sized maxSets * the 256-binding cap, declaring 81,920 descriptors per pool and 245,760 across the frames in flight, for sets that hold what shader reflection found. Sized from eight now; an outlier program is absorbed by the VK_ERROR_OUT_OF_POOL_MEMORY path that already exists, which works because pool sizes are aggregate budgets rather than per-set limits. TrackLiveResource swept the whole live-buffer vector on every insert once it passed 256 entries, and when the buffers are all live the sweep removes nothing and the vector grows by one - so creating N live buffers cost about N^2/2 expired() checks. It sweeps on a doubling watermark now, with the same reclamation semantics. GenerateMipmap transitioned each destination level individually inside its loop, but every generated level starts in the same layout and the loop only moves a level out of TRANSFER_DST after writing it, so the whole range can be prepared in one barrier - 3(N-1)+1 barrier commands become 2(N-1)+2. Each level is still transitioned to TRANSFER_SRC before it is read, so the dependency between consecutive levels is unchanged. WaitForFrameSerial drained the entire graphics queue, as its own comment admitted. Every submission records the frame serial it was made under, so it now waits on the first fence at or past the requested serial. The narrow path deliberately does not call NotifyDeviceIdle(): that claims every submission has retired, which is only true after a real drain, so it stays on the fallback. Verified with an 8213-case A/B (textures, buffers, queries, mipmaps, uniforms and the whole direct_state_access suite): the Espryt failure list is identical, the Magma failure list differs by one case, and both crash sets are unchanged on Magma. That one case, buffer_storage.map_persistent_draw, does not reproduce in isolation - running the buffer_storage group alone gives byte-identical results on both builds (the same three failures, not including it), and it reports NotSupported when run on its own. It is the same ordering-dependent behaviour this suite shows elsewhere, and the three Espryt crash-set differences are the known copy_image cluster moving chunk position. Flagging rather than hiding it. direct_state_access stays at Espryt 370/371 and Magma 371/371; unit tests 421/421. --- .../DirectVulkan/Renderer/ProgramFactory.cpp | 10 +++++ .../DirectVulkan/Renderer/ProgramFactory.h | 8 ++++ .../DirectVulkan/Renderer/UniformManager.cpp | 24 +++++++--- .../DirectVulkan/Renderer/VkBufferManager.cpp | 9 +++- .../DirectVulkan/Renderer/VkBufferManager.h | 2 + .../DirectVulkan/Renderer/VulkanRenderer.cpp | 44 ++++++++++++++----- 6 files changed, 79 insertions(+), 18 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp index dce2c9b8..c8c5b51d 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.cpp @@ -2348,6 +2348,16 @@ namespace MobileGL::MG_Backend::DirectVulkan { pipelineLayoutInfo.pSetLayouts = &entry.descriptorSetLayout; VK_VERIFY(vkCreatePipelineLayout(m_device, &pipelineLayoutInfo, nullptr, &entry.pipelineLayout), "ProgramFactory::ReflectLayout, vkCreatePipelineLayout"); + + // Built here rather than where bindingKinds is sized: at that point the vector is only + // zero-initialised and the kinds are assigned further down, so a list built there would be + // empty. Ascending by construction because the index walks upward. + entry.activeBindings.clear(); + for (Uint32 binding = 0; binding < static_cast(entry.bindingKinds.size()); ++binding) { + if (entry.bindingKinds[binding] != DescriptorBindingKind::None) { + entry.activeBindings.push_back(binding); + } + } } const ProgramFactory::VkProgramObject& ProgramFactory::GetOrCreateProgram( diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h index eb0adfd0..1ca18d35 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/ProgramFactory.h @@ -67,6 +67,12 @@ namespace MobileGL::MG_Backend::DirectVulkan { VkDescriptorSetLayout descriptorSetLayout = VK_NULL_HANDLE; VkPipelineLayout pipelineLayout = VK_NULL_HANDLE; Vector bindingKinds; + // The bindings this program actually declares, ascending. bindingKinds is sized to the + // 256-binding cap while a real GL program uses 1-8, so the per-draw descriptor walk was + // scanning 256 slots to find a handful. MUST stay ascending: Vulkan consumes + // pDynamicOffsets in binding order and the writer pushes them in iteration order, so an + // unordered list would silently mis-pair dynamic offsets with their uniform blocks. + Vector activeBindings; Vector dynamicBindings; Vector uniformBlockIndexByBinding; // Descriptor count per binding (1 except for UBO instance arrays, which occupy one @@ -114,6 +120,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { descriptorSetLayout = other.descriptorSetLayout; pipelineLayout = other.pipelineLayout; bindingKinds = std::move(other.bindingKinds); + activeBindings = std::move(other.activeBindings); dynamicBindings = std::move(other.dynamicBindings); uniformBlockIndexByBinding = std::move(other.uniformBlockIndexByBinding); bindingDescriptorCounts = std::move(other.bindingDescriptorCounts); @@ -162,6 +169,7 @@ namespace MobileGL::MG_Backend::DirectVulkan { descriptorSetLayout = other.descriptorSetLayout; pipelineLayout = other.pipelineLayout; bindingKinds = std::move(other.bindingKinds); + activeBindings = std::move(other.activeBindings); dynamicBindings = std::move(other.dynamicBindings); uniformBlockIndexByBinding = std::move(other.uniformBlockIndexByBinding); bindingDescriptorCounts = std::move(other.bindingDescriptorCounts); diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp index c2df6944..5d830302 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/UniformManager.cpp @@ -1008,7 +1008,17 @@ namespace MobileGL::MG_Backend::DirectVulkan { return false; } - const Uint64 descriptorCount64 = static_cast(maxSets) * static_cast(m_maxBindings); + // Sized from what a real program declares, not from the 256-binding cap. A GL program's + // single descriptor set holds the bindings shader reflection found - typically 2 to 8 - so + // scaling by m_maxBindings declared 5 x 64 x 256 = 81,920 descriptors per pool and 245,760 + // across the three frames in flight, which drivers that reserve backing store proportional + // to the declared count pay for at init. An outlier program is absorbed by the existing + // VK_ERROR_OUT_OF_POOL_MEMORY -> GrowFrameDescriptorPool path: pool sizes are aggregate + // budgets rather than per-set limits, and vkAllocateDescriptorSets is spec-required to + // report that error rather than fail hard. + static constexpr Uint32 kEstimatedBindingsPerSet = 8; + const Uint64 descriptorCount64 = + static_cast(maxSets) * static_cast(std::min(m_maxBindings, kEstimatedBindingsPerSet)); if (descriptorCount64 > static_cast(std::numeric_limits::max())) { MGLOG_E("UniformDescriptorBinder::CreateDescriptorPool failed: descriptorCount overflow"); return false; @@ -1187,13 +1197,13 @@ namespace MobileGL::MG_Backend::DirectVulkan { texelBufferViews.reserve(m_maxBindings); dynamicOffsets.reserve(programObj.dynamicBindings.size() + uboArrayExtra); - const Uint32 bindingCount = - std::min(m_maxBindings, static_cast(programObj.bindingKinds.size())); - for (Uint32 binding = 0; binding < bindingCount; ++binding) { - const auto kind = programObj.bindingKinds[binding]; - if (kind == ProgramFactory::DescriptorBindingKind::None) { - continue; + // Iterate only the bindings this program declares. The old walk covered all 256 slots of + // bindingKinds on every draw to find the 1-8 a real program uses. + for (const Uint32 binding : programObj.activeBindings) { + if (binding >= m_maxBindings) { + break; // ascending, so nothing past the cap can follow } + const auto kind = programObj.bindingKinds[binding]; VkWriteDescriptorSet write{}; write.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp index 6f5e9289..ccbe37a1 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.cpp @@ -242,8 +242,15 @@ namespace MobileGL::MG_Backend::DirectVulkan { } void VkBufferManager::TrackLiveResource(const SharedPtr& resource) { - if (m_liveResources.size() >= kLiveResourcePruneThreshold) { + // Sweep on a doubling watermark rather than on every insert past the threshold. The old + // form walked the whole vector for each new buffer once the list passed 256, and when the + // buffers are all live the walk removes nothing and the list grows by one - so creating N + // live buffers cost ~N^2/2 expired() checks. Reclamation semantics are unchanged: the sweep + // still removes exactly the expired entries, just less often and with the same bound on how + // much dead weight can accumulate (at most as many entries as were live at the last sweep). + if (m_liveResources.size() >= std::max(kLiveResourcePruneThreshold, 2 * m_liveResourcesLastPruned)) { std::erase_if(m_liveResources, [](const WeakPtr& weak) { return weak.expired(); }); + m_liveResourcesLastPruned = m_liveResources.size(); } m_liveResources.push_back(resource); } diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h index c1e85461..25da3f6f 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VkBufferManager.h @@ -154,6 +154,8 @@ namespace MobileGL::MG_Backend::DirectVulkan { Vector> m_deferredBufferReleases; Vector>> m_deferredResourceReleases; Vector> m_liveResources; + // Size m_liveResources had just after the last sweep; the next sweep waits for it to double. + SizeT m_liveResourcesLastPruned = 0; Uint32 m_currentFrameIndex = 0; Uint64 m_frameSerial = 1; Uint64 m_completedSerialFloor = 0; diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index a133d29d..094f6e83 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -8285,15 +8285,23 @@ void main() { resource->aspect, baseMipLevel, 1); MOBILEGL_ASSERT(srcReady, "%s: failed to transition base mip level to transfer source", __func__); - for (Uint32 level = baseMipLevel + 1; level < generateMipLevelCount; ++level) { - VkImageLayout dstMipLayout = originalLayout; - Bool dstReady = VkTextureManager::TransitionImageLayout( - frame.commandBuffer, resource->image, dstMipLayout, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, + // Every generated level starts from originalLayout and ends up TRANSFER_DST_OPTIMAL, and + // the loop below only ever moves a level OUT of that layout after it has been written - so + // the whole range can be prepared in one barrier instead of one per level. That turns a + // 12-level chain's 3(N-1)+1 barrier commands into 2(N-1)+2. Each level is still + // individually transitioned to TRANSFER_SRC before it is read, so the write-then-read + // dependency between consecutive levels is unchanged. + if (generateMipLevelCount > baseMipLevel + 1) { + VkImageLayout dstRangeLayout = originalLayout; + const Bool dstRangeReady = VkTextureManager::TransitionImageLayout( + frame.commandBuffer, resource->image, dstRangeLayout, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, originalSrcStageMask, VK_PIPELINE_STAGE_TRANSFER_BIT, originalSrcAccessMask, VK_ACCESS_TRANSFER_WRITE_BIT, - resource->aspect, level, 1); - MOBILEGL_ASSERT(dstReady, "%s: failed to transition mip level %u to transfer destination", __func__, level); + resource->aspect, baseMipLevel + 1, generateMipLevelCount - (baseMipLevel + 1)); + MOBILEGL_ASSERT(dstRangeReady, "%s: failed to transition mip levels to transfer destination", __func__); + } + for (Uint32 level = baseMipLevel + 1; level < generateMipLevelCount; ++level) { const IntVec3 srcTexelSize = ComputeMipTexelSize(storageBaseTexelSize, level - 1); const IntVec3 dstTexelSize = ComputeMipTexelSize(storageBaseTexelSize, level); @@ -9085,10 +9093,26 @@ void main() { if (m_device == VK_NULL_HANDLE || m_graphicsQueue == VK_NULL_HANDLE) { return true; } - // The serial was submitted but has not been observed complete. Frame - // fences are only waited on when their slot is reused, so the simplest - // safe wait is to drain the graphics queue; this over-waits (bounded - // by the in-flight frame count) but never deadlocks. + // Every submission is recorded with the frame serial it was made under, so the wait can be + // narrowed to the first submission at or past the requested serial instead of draining the + // whole queue. OnSubmitsCompletedUpTo calls NotifyFrameSerialComplete for every record it + // retires, so the completed-serial floor still advances correctly after one fence wait. + for (const auto& record : m_inFlightSubmits) { + if (record.frameSerial < serial || record.fence == VK_NULL_HANDLE) { + continue; + } + if (vkWaitForFences(m_device, 1, &record.fence, VK_TRUE, UINT64_MAX) != VK_SUCCESS) { + break; // fall through to the drain below + } + OnSubmitsCompletedUpTo(record.submitIndex); + // Deliberately no NotifyDeviceIdle() here: that claims every submission has retired, + // which is only true after a real queue drain. Work past this record may still run. + TryDrainFrameTransients(); + return true; + } + + // No usable record - fall back to draining the graphics queue. This over-waits (bounded by + // the in-flight frame count) but never deadlocks. const VkResult result = vkQueueWaitIdle(m_graphicsQueue); if (result != VK_SUCCESS) { MGLOG_E("WaitForFrameSerial: vkQueueWaitIdle returned %d", result);