From 13b0b54987db2684e46e337d2e5be5b81366083b Mon Sep 17 00:00:00 2001 From: assiduous Date: Tue, 20 Oct 2020 13:26:21 -0700 Subject: Improved exception safety of pipeline state object construction --- .../include/GLProgramResourceCache.hpp | 2 +- .../include/PipelineStateGLImpl.hpp | 2 + .../src/GLProgramResourceCache.cpp | 4 +- .../src/PipelineStateGLImpl.cpp | 130 ++++++++++++++------- 4 files changed, 92 insertions(+), 46 deletions(-) (limited to 'Graphics/GraphicsEngineOpenGL') diff --git a/Graphics/GraphicsEngineOpenGL/include/GLProgramResourceCache.hpp b/Graphics/GraphicsEngineOpenGL/include/GLProgramResourceCache.hpp index eab96f6a..07c8f39e 100644 --- a/Graphics/GraphicsEngineOpenGL/include/GLProgramResourceCache.hpp +++ b/Graphics/GraphicsEngineOpenGL/include/GLProgramResourceCache.hpp @@ -45,7 +45,7 @@ namespace Diligent class GLProgramResourceCache { public: - GLProgramResourceCache() + GLProgramResourceCache() noexcept {} ~GLProgramResourceCache(); diff --git a/Graphics/GraphicsEngineOpenGL/include/PipelineStateGLImpl.hpp b/Graphics/GraphicsEngineOpenGL/include/PipelineStateGLImpl.hpp index 0c5692d0..93180774 100644 --- a/Graphics/GraphicsEngineOpenGL/include/PipelineStateGLImpl.hpp +++ b/Graphics/GraphicsEngineOpenGL/include/PipelineStateGLImpl.hpp @@ -114,6 +114,8 @@ private: void InitResourceLayouts(const std::vector& ShaderStages, LinearAllocator& MemPool); + void Destruct(); + // Linked GL programs for every shader stage. Every pipeline needs to have its own programs // because resource bindings assigned by GLProgramResources::LoadUniforms depend on other // shader stages. diff --git a/Graphics/GraphicsEngineOpenGL/src/GLProgramResourceCache.cpp b/Graphics/GraphicsEngineOpenGL/src/GLProgramResourceCache.cpp index 95557557..84cdc46e 100644 --- a/Graphics/GraphicsEngineOpenGL/src/GLProgramResourceCache.cpp +++ b/Graphics/GraphicsEngineOpenGL/src/GLProgramResourceCache.cpp @@ -92,7 +92,9 @@ GLProgramResourceCache::~GLProgramResourceCache() void GLProgramResourceCache::Destroy(IMemoryAllocator& MemAllocator) { - VERIFY(IsInitialized(), "Resource cache is not initialized"); + if (!IsInitialized()) + return; + VERIFY(m_pdbgMemoryAllocator == &MemAllocator, "The allocator does not match the one used to create resources"); for (Uint32 cb = 0; cb < GetUBCount(); ++cb) diff --git a/Graphics/GraphicsEngineOpenGL/src/PipelineStateGLImpl.cpp b/Graphics/GraphicsEngineOpenGL/src/PipelineStateGLImpl.cpp index 8bc3802e..8b429094 100644 --- a/Graphics/GraphicsEngineOpenGL/src/PipelineStateGLImpl.cpp +++ b/Graphics/GraphicsEngineOpenGL/src/PipelineStateGLImpl.cpp @@ -40,22 +40,37 @@ namespace Diligent template void PipelineStateGLImpl::Initialize(const PSOCreateInfoType& CreateInfo, const std::vector& ShaderStages) { - // Memory must be released if an exception is thrown. LinearAllocator MemPool{GetRawAllocator()}; + VERIFY_EXPR(m_NumShaderStages > 0 && m_NumShaderStages == ShaderStages.size()); + if (!GetDevice()->GetDeviceCaps().Features.SeparablePrograms) + m_NumShaderStages = 1; - MemPool.AddSpace(GetNumShaderStages()); - MemPool.AddSpace(GetNumShaderStages()); + const auto NumPrograms = GetNumShaderStages(); + + MemPool.AddSpace(NumPrograms); + MemPool.AddSpace(NumPrograms); MemPool.AddSpace(m_Desc.ResourceLayout.NumImmutableSamplers); ReserveSpaceForPipelineDesc(CreateInfo, MemPool); MemPool.Reserve(); - InitResourceLayouts(ShaderStages, MemPool); - InitializePipelineDesc(CreateInfo, MemPool); + m_GLPrograms = MemPool.ConstructArray(NumPrograms, false); - void* Ptr = MemPool.Release(); + // The memory is now owned by PipelineStateVkImpl and will be freed by Destruct(). + auto* Ptr = MemPool.ReleaseOwnership(); VERIFY_EXPR(Ptr == m_GLPrograms); + (void)Ptr; + + m_ProgramResources = MemPool.ConstructArray(NumPrograms); + + m_ImmutableSamplers = MemPool.ConstructArray(m_Desc.ResourceLayout.NumImmutableSamplers); + + // It is important to construct all objects before initializing them because if an exception is thrown, + // destructors will be called for all objects + + InitResourceLayouts(ShaderStages, MemPool); + InitializePipelineDesc(CreateInfo, MemPool); } PipelineStateGLImpl::PipelineStateGLImpl(IReferenceCounters* pRefCounters, @@ -74,26 +89,33 @@ PipelineStateGLImpl::PipelineStateGLImpl(IReferenceCounters* m_StaticResourceLayout{*this} // clang-format on { - std::vector ShaderStages; - ExtractShaders(CreateInfo, ShaderStages); + try + { + std::vector ShaderStages; + ExtractShaders(CreateInfo, ShaderStages); + + RefCntAutoPtr pTempPS; + if (CreateInfo.pPS == nullptr) + { + // Some OpenGL implementations fail if fragment shader is not present, so + // create a dummy one. + ShaderCreateInfo ShaderCI; + ShaderCI.SourceLanguage = SHADER_SOURCE_LANGUAGE_GLSL; + ShaderCI.Source = "void main(){}"; + ShaderCI.Desc.ShaderType = SHADER_TYPE_PIXEL; + ShaderCI.Desc.Name = "Dummy fragment shader"; + pDeviceGL->CreateShader(ShaderCI, reinterpret_cast(static_cast(&pTempPS))); + + ShaderStages.emplace_back(SHADER_TYPE_PIXEL, pTempPS); + m_ShaderStageTypes[m_NumShaderStages++] = SHADER_TYPE_PIXEL; + } - RefCntAutoPtr pTempPS; - if (CreateInfo.pPS == nullptr) + Initialize(CreateInfo, ShaderStages); + } + catch (...) { - // Some OpenGL implementations fail if fragment shader is not present, so - // create a dummy one. - ShaderCreateInfo ShaderCI; - ShaderCI.SourceLanguage = SHADER_SOURCE_LANGUAGE_GLSL; - ShaderCI.Source = "void main(){}"; - ShaderCI.Desc.ShaderType = SHADER_TYPE_PIXEL; - ShaderCI.Desc.Name = "Dummy fragment shader"; - pDeviceGL->CreateShader(ShaderCI, reinterpret_cast(static_cast(&pTempPS))); - - ShaderStages.emplace_back(SHADER_TYPE_PIXEL, pTempPS); - m_ShaderStageTypes[m_NumShaderStages++] = SHADER_TYPE_PIXEL; + Destruct(); } - - Initialize(CreateInfo, ShaderStages); } PipelineStateGLImpl::PipelineStateGLImpl(IReferenceCounters* pRefCounters, @@ -112,30 +134,54 @@ PipelineStateGLImpl::PipelineStateGLImpl(IReferenceCounters* p m_StaticResourceLayout{*this} // clang-format on { - std::vector ShaderStages; - ExtractShaders(CreateInfo, ShaderStages); + try + { + std::vector ShaderStages; + ExtractShaders(CreateInfo, ShaderStages); - Initialize(CreateInfo, ShaderStages); + Initialize(CreateInfo, ShaderStages); + } + catch (...) + { + Destruct(); + } } PipelineStateGLImpl::~PipelineStateGLImpl() +{ + Destruct(); +} + +void PipelineStateGLImpl::Destruct() { auto& RawAllocator = GetRawAllocator(); m_StaticResourceCache.Destroy(RawAllocator); GetDevice()->OnDestroyPSO(this); - for (Uint32 i = 0; i < GetNumShaderStages(); ++i) + if (m_ImmutableSamplers != nullptr) { - m_GLPrograms[i].~GLProgramObj(); - m_ProgramResources[i].~GLProgramResources(); + for (Uint32 i = 0; i < m_Desc.ResourceLayout.NumImmutableSamplers; ++i) + { + m_ImmutableSamplers[i].~SamplerPtr(); + } } - for (Uint32 i = 0; i < m_Desc.ResourceLayout.NumImmutableSamplers; ++i) + + for (Uint32 i = 0; i < GetNumShaderStages(); ++i) { - m_ImmutableSamplers[i].~SamplerPtr(); + if (m_GLPrograms != nullptr) + { + m_GLPrograms[i].~GLProgramObj(); + } + if (m_ProgramResources != nullptr) + { + m_ProgramResources[i].~GLProgramResources(); + } } - void* pRawMem = m_GLPrograms; - RawAllocator.Free(pRawMem); + if (void* pRawMem = m_GLPrograms) + { + RawAllocator.Free(pRawMem); + } } IMPLEMENT_QUERY_INTERFACE(PipelineStateGLImpl, IID_PipelineStateGL, TPipelineStateBase) @@ -145,8 +191,8 @@ void PipelineStateGLImpl::InitResourceLayouts(const std::vectorGetDeviceCaps(); - VERIFY(DeviceCaps.DevType != RENDER_DEVICE_TYPE_UNDEFINED, "Device caps are not initialized"); + const auto& deviceCaps = pDeviceGL->GetDeviceCaps(); + VERIFY(deviceCaps.DevType != RENDER_DEVICE_TYPE_UNDEFINED, "Device caps are not initialized"); auto pImmediateCtx = m_pDevice->GetImmediateContext(); VERIFY_EXPR(pImmediateCtx); @@ -157,18 +203,16 @@ void PipelineStateGLImpl::InitResourceLayouts(const std::vector(ShaderStages.size()); - m_ProgramResources = MemPool.ConstructArray(ShaderStages.size()); for (size_t i = 0; i < ShaderStages.size(); ++i) { auto* pShaderGL = ShaderStages[i].pShader; const auto& ShaderDesc = pShaderGL->GetDesc(); - new (m_GLPrograms + i) GLProgramObj{ShaderGLImpl::LinkProgram(&pShaderGL, 1, true)}; + m_GLPrograms[i] = GLProgramObj{ShaderGLImpl::LinkProgram(&pShaderGL, 1, true)}; // Load uniforms and assign bindings m_ProgramResources[i].LoadUniforms(ShaderDesc.ShaderType, m_GLPrograms[i], GLState, m_TotalUniformBufferBindings, @@ -191,8 +235,7 @@ void PipelineStateGLImpl::InitResourceLayouts(const std::vector(ShaderGLImpl::LinkProgram(Shaders.data(), static_cast(ShaderStages.size()), false)); - m_ProgramResources = MemPool.Construct(); + m_GLPrograms[0] = ShaderGLImpl::LinkProgram(Shaders.data(), static_cast(Shaders.size()), false); m_ProgramResources[0].LoadUniforms(ActiveStages, m_GLPrograms[0], GLState, m_TotalUniformBufferBindings, @@ -204,10 +247,9 @@ void PipelineStateGLImpl::InitResourceLayouts(const std::vector(ShaderStages.size()), m_Desc.PipelineType, m_Desc.ResourceLayout, nullptr, 0, nullptr); + m_ResourceLayout.Initialize(m_ProgramResources, GetNumShaderStages(), m_Desc.PipelineType, m_Desc.ResourceLayout, nullptr, 0, nullptr); } - m_ImmutableSamplers = MemPool.ConstructArray(m_Desc.ResourceLayout.NumImmutableSamplers); for (Uint32 s = 0; s < m_Desc.ResourceLayout.NumImmutableSamplers; ++s) { pDeviceGL->CreateSampler(m_Desc.ResourceLayout.ImmutableSamplers[s].Desc, &m_ImmutableSamplers[s]); @@ -216,7 +258,7 @@ void PipelineStateGLImpl::InitResourceLayouts(const std::vector(ShaderStages.size()), m_Desc.PipelineType, m_Desc.ResourceLayout, StaticVars, _countof(StaticVars), &m_StaticResourceCache); + m_StaticResourceLayout.Initialize(m_ProgramResources, GetNumShaderStages(), m_Desc.PipelineType, m_Desc.ResourceLayout, StaticVars, _countof(StaticVars), &m_StaticResourceCache); InitImmutableSamplersInResourceCache(m_StaticResourceLayout, m_StaticResourceCache); } } -- cgit v1.2.3