From f3d52faad415ce6f6b230f3f53762007e79bf65b Mon Sep 17 00:00:00 2001 From: BZLZHH Date: Wed, 5 Aug 2026 12:49:54 -0400 Subject: [PATCH] [Perf] (MG_State, MG_Backend): stop glViewport from evicting a cached VkPipeline RenderState kept one version counter for all render state, and DirectVulkan read it in three places: the pipeline memo key, the SetupDrawSnapshot fast-path guard, and that guard's store. So glViewport, glScissor, glBlendColor, glStencilMask, glClearColor, glPolygonOffset, glLineWidth and the point-size family - none of which can alter a VkPipeline, all of which an application changes between draws - knocked the next draw off both fast paths and made it rebuild a pipeline lookup that was already correct. The counter is now split. m_version still moves on every state change, because the draw snapshot really does depend on all of it. m_pipelineStateVersion moves only for the state a backend bakes into a pipeline object, and it is what the three DirectVulkan sites read. The exclusion list is the eight VkDynamicState entries PipelineFactory declares plus the state that is not pipeline state at all (the clear values, hints, the point-size family, clamp read colour, the primitive restart index). glStencilFunc is the one setter that had to be split rather than classified: Func is in the pipeline payload but Ref and ValueMask are dynamic state, so it bumps the pipeline version only when Func actually changes. Capabilities are deliberately NOT in the exclusion list even though several look like dynamic state: GL_FRAMEBUFFER_SRGB feeds the render-pass hash, depth and stencil test feed drawUsesDepthStencil, and scissor test, blend, cull face, polygon offset fill, primitive restart, colour logic op and rasterizer discard all feed the pipeline payload. Two smaller draw-path wins ride along, both removing work whose answer was already in hand. UploadAndBindVertexStreams searched all 32 VAO attribute slots for the SharedPtr matching a binding's buffer key, once per binding per draw - but VertexInputStateFactory writes bindingBufferKeys[b] and bindingAttributeLocations[b] from the same loop iteration, one binding per attribute with no merging, so the attribute at that location IS the buffer, by construction. UploadAndBindIndexBuffer round-tripped the element-array buffer's raw pointer back through the GL name table on every indexed draw, costing a map lookup and an atomic refcount pair, when the binding slot's SharedPtr was already in scope forty lines above - where a comment says exactly that about the vertex path. Behaviour-neutral by construction and verified as such: a 13355-case subset of GL30-GL45 covering viewport, scissor, blend, stencil, depth, polygon offset, clear, multisample, cull, logic op, line width and point state, plus the whole direct_state_access suite, is identical before and after on both backends - in the failure list and in the crashed-case set. direct_state_access stays at Espryt 370/371 and Magma 371/371. --- .../DirectVulkan/Renderer/VulkanRenderer.cpp | 43 +++++++--------- MobileGL/MG_State/GLState/Core.cpp | 4 ++ MobileGL/MG_State/GLState/Core.h | 2 + .../GLState/RenderState/RenderState.cpp | 49 +++++++++++-------- .../GLState/RenderState/RenderState.h | 16 ++++++ 5 files changed, 70 insertions(+), 44 deletions(-) diff --git a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp index 7837d147..a133d29d 100644 --- a/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp +++ b/MobileGL/MG_Backend/DirectVulkan/Renderer/VulkanRenderer.cpp @@ -3037,17 +3037,6 @@ void main() { vkBuffers.assign(bindingCount, VK_NULL_HANDLE); vkOffsets.assign(bindingCount, 0); - auto findBufferByKey = [&](SizeT bufferKey) -> const SharedPtr* { - const auto& attrs = vao.GetAllAttributes(); - for (Uint32 location = 0; location < MG_State::GLState::VertexArrayObject::MAX_VERTEX_ATTRIBS; ++location) { - const auto& attr = attrs[location]; - if (attr.Buffer && reinterpret_cast(attr.Buffer.get()) == bufferKey) { - return &attr.Buffer; - } - } - return nullptr; - }; - auto uploadConvertedStream = [&](VertexInputStateFactory::VertexStreamConversion conversion, const MG_State::GLState::VertexAttribute& attribute, const Uint8* sourceData, SizeT sourceStride, @@ -3150,14 +3139,14 @@ void main() { continue; } - const SizeT bufferKey = vertexInputState.bindingBufferKeys[binding]; - // The VAO attribute already holds the buffer's SharedPtr; use it by reference directly - // instead of re-resolving it from the GL context by external index (a map lookup + - // atomic refcount every binding every draw). - const SharedPtr* sourceBufferSharedPtr = findBufferByKey(bufferKey); - MOBILEGL_ASSERT(sourceBufferSharedPtr != nullptr && *sourceBufferSharedPtr != nullptr, + // VertexInputStateFactory fills bindingBufferKeys[b] and bindingAttributeLocations[b] + // from the SAME loop iteration, one binding per enabled attribute with no merging, so + // this attribute's Buffer IS the SharedPtr by construction - no need to search the VAO's + // 32 slots for it. The client-memory branch above has already returned, so the location + // is in range here. + const auto& sourceBufferShared = vao.GetAttribute(bindingLocation).Buffer; + MOBILEGL_ASSERT(sourceBufferShared != nullptr, "UploadAndBindVertexStreams failed to resolve source buffer"); - const auto& sourceBufferShared = *sourceBufferSharedPtr; BufferSlice slice{}; const SizeT sourceSize = sourceBufferShared->GetSize(); const SizeT baseOffset = @@ -3401,8 +3390,12 @@ void main() { substituteRestartIndex = restartIndex; } - const auto* indexBuffer = - pIndexBufferView->forceClientMemory ? nullptr : vao.GetIndexBufferBindingSlot().GetBoundObject().get(); + // Bound by reference so the SharedPtr below is the one already in hand rather than a fresh + // GL-name map lookup plus an atomic refcount pair on every indexed draw - the vertex path + // above documents the same cost. + const SharedPtr& indexBufferShared = + vao.GetIndexBufferBindingSlot().GetBoundObject(); + const auto* indexBuffer = pIndexBufferView->forceClientMemory ? nullptr : indexBufferShared.get(); if (indexBuffer == nullptr) { // No element-array buffer: the view's byte offset is a raw client pointer // (desktop drivers accept client-memory indices and the GL CTS relies on @@ -3441,7 +3434,6 @@ void main() { "DrawElements index range out of bounds"); BufferSlice slice{}; - auto indexBufferShared = MG_State::pGLContext->GetBufferObject(indexBuffer->GetExternalIndex()); MOBILEGL_ASSERT(indexBufferShared != nullptr, "UploadAndBindIndexBuffer failed to resolve shared EBO"); if (substituteRestart) { // The whole buffer is rewritten, not just this draw's range, so that every element @@ -4068,7 +4060,10 @@ void main() { auto& vis = m_vertexInputStateFactory->GetOrCreateVertexInputState(vao); const Uint64 vertexLayoutHash = vis.layoutHash; const Uint64 renderPassHash = renderPassEntry.hash; - const Uint renderStateVersion = MG_State::pGLContext->GetRenderStateParametersVersion(); + // The pipeline-relevant subset only: glViewport / glScissor / glBlendColor / glStencilMask + // and friends are dynamic state or not pipeline state at all, and keying the memo on the + // all-state counter made any of them evict a perfectly good VkPipeline. + const Uint renderStateVersion = MG_State::pGLContext->GetPipelineStateVersion(); for (Uint32 i = 0; i < m_pipelineMemoCount; ++i) { const PipelineMemoEntry& entry = m_pipelineMemo[i]; if (entry.pipeline != VK_NULL_HANDLE && entry.mode == mode && @@ -4708,7 +4703,7 @@ void main() { drawFbo->GetObjectVersion() != snap.fboVersion) { return false; } - if (MG_State::pGLContext->GetRenderStateParametersVersion() != snap.renderStateVersion || + if (MG_State::pGLContext->GetPipelineStateVersion() != snap.renderStateVersion || MG_State::pGLContext->GetTextureBindGeneration() != snap.bindGeneration) { return false; } @@ -5172,7 +5167,7 @@ void main() { snap.drawFbo = drawFbo.get(); snap.fboVersion = drawFbo->GetObjectVersion(); snap.drawFboIsDefault = drawFbo->IsDefaultFramebuffer(); - snap.renderStateVersion = MG_State::pGLContext->GetRenderStateParametersVersion(); + snap.renderStateVersion = MG_State::pGLContext->GetPipelineStateVersion(); snap.bindGeneration = MG_State::pGLContext->GetTextureBindGeneration(); snap.baseTransformFlags = GetShaderTransformFlags(m_swapchainObject.GetPreTransform()).GetRaw(); snap.resolvedTransformFlags = transformFlags.GetRaw(); diff --git a/MobileGL/MG_State/GLState/Core.cpp b/MobileGL/MG_State/GLState/Core.cpp index 08a51e02..f2ca7f40 100644 --- a/MobileGL/MG_State/GLState/Core.cpp +++ b/MobileGL/MG_State/GLState/Core.cpp @@ -393,6 +393,10 @@ namespace MobileGL::MG_State { } // RenderState + Uint GLContext::GetPipelineStateVersion() const { + return m_renderState.GetPipelineStateVersion(); + } + Uint GLContext::GetRenderStateParametersVersion() const { return m_renderState.GetVersion(); } diff --git a/MobileGL/MG_State/GLState/Core.h b/MobileGL/MG_State/GLState/Core.h index f3a8e620..b462a992 100644 --- a/MobileGL/MG_State/GLState/Core.h +++ b/MobileGL/MG_State/GLState/Core.h @@ -159,6 +159,8 @@ namespace MobileGL { // RenderState Uint GetRenderStateParametersVersion() const; + // Only the pipeline-relevant subset - see RenderState::m_pipelineStateVersion. + Uint GetPipelineStateVersion() const; const RenderStateParameters& GetRenderStateParameters() const; void SetViewport(IntVec4 viewport); // x, y, width, height const IntVec4& GetViewport() const; // x, y, width, height diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp index 74bb7603..06a07ecd 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.cpp +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.cpp @@ -37,6 +37,10 @@ namespace MobileGL { return m_version; } + Uint RenderState::GetPipelineStateVersion() const { + return m_pipelineStateVersion; + } + const RenderStateParameters& RenderState::GetAllParameters() const { return m_parameters; } @@ -122,7 +126,7 @@ namespace MobileGL { if (m_parameters.PolygonModeFront == front && m_parameters.PolygonModeBack == back) return; m_parameters.PolygonModeFront = front; m_parameters.PolygonModeBack = back; - ++m_version; + BumpVersions(); } GLenum RenderState::GetPolygonModeFront() const { @@ -158,7 +162,7 @@ namespace MobileGL { if (m_parameters.PatchVertices == vertices) return; m_parameters.PatchVertices = vertices; - ++m_version; + BumpVersions(); } Uint RenderState::GetPatchVertices() const { @@ -187,7 +191,7 @@ namespace MobileGL { case CapabilityInput::capability: \ if (m_parameters.capability##Enabled == (flag)) break; \ m_parameters.capability##Enabled = (flag); \ - ++m_version; \ + BumpVersions(); \ break; switch (cap) { @@ -220,7 +224,7 @@ namespace MobileGL { blendState.Enabled = enabled; stateChanged = true; } - if (stateChanged) ++m_version; + if (stateChanged) BumpVersions(); break; } default: // not supported currently @@ -276,7 +280,7 @@ namespace MobileGL { if (m_parameters.BlendStates[index].Enabled == enabled) return; m_parameters.BlendStates[index].Enabled = enabled; - ++m_version; + BumpVersions(); } Bool RenderState::IsCapabilityEnabledIndexed(CapabilityInput cap, Uint index) const { @@ -308,7 +312,7 @@ namespace MobileGL { stateChanged = true; } if (!stateChanged) return; - ++m_version; + BumpVersions(); } void RenderState::GetBlendFunc(BlendFactor& srcRGB, BlendFactor& dstRGB, BlendFactor& srcAlpha, @@ -334,7 +338,7 @@ namespace MobileGL { blendState.DstFactorRGB = dstRGB; blendState.SrcFactorAlpha = srcAlpha; blendState.DstFactorAlpha = dstAlpha; - ++m_version; + BumpVersions(); } void RenderState::GetBlendFuncIndexed(Uint index, BlendFactor& srcRGB, BlendFactor& dstRGB, @@ -360,7 +364,7 @@ namespace MobileGL { stateChanged = true; } if (!stateChanged) return; - ++m_version; + BumpVersions(); } void RenderState::GetBlendEquation(BlendEquation& color, BlendEquation& alpha) const { @@ -379,7 +383,7 @@ namespace MobileGL { } blendState.ColorEquation = color; blendState.AlphaEquation = alpha; - ++m_version; + BumpVersions(); } void RenderState::GetBlendEquationIndexed(Uint index, BlendEquation& color, BlendEquation& alpha) const { @@ -395,7 +399,7 @@ namespace MobileGL { if (m_parameters.LogicOp == logicOp) return; m_parameters.LogicOp = logicOp; - ++m_version; + BumpVersions(); } LogicOperation RenderState::GetLogicOp() const { @@ -407,7 +411,7 @@ namespace MobileGL { if (m_parameters.DepthFunc == func) return; m_parameters.DepthFunc = func; - ++m_version; + BumpVersions(); } DepthTestFunc RenderState::GetDepthFunc() const { @@ -418,7 +422,7 @@ namespace MobileGL { if (m_parameters.DepthMask == flag) return; m_parameters.DepthMask = flag; - ++m_version; + BumpVersions(); } Bool RenderState::GetDepthMask() const { @@ -429,10 +433,15 @@ namespace MobileGL { StencilFaceState& state = m_parameters.StencilStates[GetStencilFaceIndex(face)]; if (state.Func == func && state.Ref == ref && state.ValueMask == mask) return; + // Only Func is baked into the pipeline; Ref and ValueMask are dynamic state + // (VK_DYNAMIC_STATE_STENCIL_REFERENCE / _COMPARE_MASK), so glStencilFunc changing + // only the reference must not evict a cached pipeline. + const Bool pipelineRelevantChange = state.Func != func; state.Func = func; state.Ref = ref; state.ValueMask = mask; ++m_version; + if (pipelineRelevantChange) ++m_pipelineStateVersion; } void RenderState::SetStencilMask(StencilFace face, Uint32 mask) { @@ -454,7 +463,7 @@ namespace MobileGL { state.FailOp = fail; state.PassDepthFailOp = depthFail; state.PassDepthPassOp = depthPass; - ++m_version; + BumpVersions(); } const StencilFaceState& RenderState::GetStencilState(StencilFace face) const { @@ -471,7 +480,7 @@ namespace MobileGL { changed = true; } } - if (changed) ++m_version; + if (changed) BumpVersions(); } BoolVec4 RenderState::GetColorMask() const { @@ -482,7 +491,7 @@ namespace MobileGL { void RenderState::SetColorMaskIndexed(Uint index, BoolVec4 mask) { if (m_parameters.ColorMasks[index] == mask) return; m_parameters.ColorMasks[index] = mask; - ++m_version; + BumpVersions(); } BoolVec4 RenderState::GetColorMaskIndexed(Uint index) const { @@ -550,7 +559,7 @@ namespace MobileGL { m_parameters.SampleCoverageValue = value; m_parameters.SampleCoverageInvert = invert; - ++m_version; + BumpVersions(); } Float RenderState::GetSampleCoverageValue() const { @@ -565,7 +574,7 @@ namespace MobileGL { if (m_parameters.SampleMaskValue == mask) return; m_parameters.SampleMaskValue = mask; - ++m_version; + BumpVersions(); } Uint32 RenderState::GetSampleMaskValue() const { @@ -639,7 +648,7 @@ namespace MobileGL { if (m_parameters.CullFaceModeSetting == mode) return; m_parameters.CullFaceModeSetting = mode; - ++m_version; + BumpVersions(); } CullFaceMode RenderState::GetCullFaceMode() const { @@ -650,7 +659,7 @@ namespace MobileGL { if (m_parameters.FrontFaceModeSetting == mode) return; m_parameters.FrontFaceModeSetting = mode; - ++m_version; + BumpVersions(); } FrontFaceMode RenderState::GetFrontFaceMode() const { @@ -661,7 +670,7 @@ namespace MobileGL { if (m_parameters.ProvokingVertexModeSetting == mode) return; m_parameters.ProvokingVertexModeSetting = mode; - ++m_version; + BumpVersions(); } ProvokingVertexMode RenderState::GetProvokingVertexMode() const { diff --git a/MobileGL/MG_State/GLState/RenderState/RenderState.h b/MobileGL/MG_State/GLState/RenderState/RenderState.h index 65e22393..b272fc2e 100644 --- a/MobileGL/MG_State/GLState/RenderState/RenderState.h +++ b/MobileGL/MG_State/GLState/RenderState/RenderState.h @@ -312,6 +312,8 @@ namespace MobileGL { RenderState(); Uint GetVersion() const; + // Version of the pipeline-relevant subset only - see m_pipelineStateVersion. + Uint GetPipelineStateVersion() const; const RenderStateParameters& GetAllParameters() const; // Rasterization @@ -418,7 +420,21 @@ namespace MobileGL { const IntVec4& GetScissorBox() const; // x, y, width, height private: + // Bump both: any state change invalidates the draw snapshot, and this one also + // changes the VkPipeline (or its DirectGLES equivalent). + void BumpVersions() { + ++m_version; + ++m_pipelineStateVersion; + } + Uint16 m_version = 0; + // Only the subset of render state that a backend bakes INTO a pipeline object. + // Viewport, scissor, depth range, blend colour, line width, polygon offset, stencil + // write mask, the clear values, hints and the point-size family are all either + // dynamic pipeline state or not pipeline state at all, so changing one of them must + // not evict a cached pipeline. Keeping one counter for both made a glViewport call + // knock the next draw off the pipeline memo AND the draw fast path. + Uint16 m_pipelineStateVersion = 0; RenderStateParameters m_parameters; // Pixel Store