From afe91384086551ea3156fc67c525fdf864aa8437 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Mon, 4 Oct 2021 12:31:22 +0000 Subject: [PATCH] Reland "OpenGL: delete shaders and pipelines when they are not used any longer" This is a reland of a57c1db878e2f8a7eb352c8f253db5488cdca6f6 in current Dawn implementation RenderPipelineGL and ComputePipelineGL will be destroyed before calling their initialize() function when we can find a proper pipeline object in the front-end cache, so mProgram may not be assigned to a valid GL program ID when the destructor of PipelineGL is called. In this CL we always initialize mProgram to 0 in the constructor of PipelineGL so that it won't be a garbage value in the destructor of PipelineGL (0 is safe for glDeleteProgram() according to OpenGL SPEC). Original change's description: > OpenGL: delete shaders and pipelines when they are not used any longer > > Previously on OpenGL backend the GL pipelines and shaders are never > deleted. With this patch the GL pipelines and shaders will be able to > be destroyed correctly after they are not needed any longer. > > BUG=dawn:529 > > Change-Id: I4f7f22c7b536825363fe1ecc0f5ffd1bb86fd774 > Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65140 > Reviewed-by: Corentin Wallez > Reviewed-by: Stephen White > Commit-Queue: Jiawei Shao Bug: dawn:529 Change-Id: Ie04ab069b9d26658f2b0d1b070d86bb650f3c878 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65486 Commit-Queue: Corentin Wallez Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Reviewed-by: Stephen White --- src/dawn_native/opengl/ComputePipelineGL.cpp | 4 ++++ src/dawn_native/opengl/ComputePipelineGL.h | 2 +- src/dawn_native/opengl/PipelineGL.cpp | 16 +++++++++++++++- src/dawn_native/opengl/PipelineGL.h | 4 ++-- src/dawn_native/opengl/RenderPipelineGL.cpp | 1 + 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/dawn_native/opengl/ComputePipelineGL.cpp b/src/dawn_native/opengl/ComputePipelineGL.cpp index fb333444b3..e51e217a8f 100644 --- a/src/dawn_native/opengl/ComputePipelineGL.cpp +++ b/src/dawn_native/opengl/ComputePipelineGL.cpp @@ -27,6 +27,10 @@ namespace dawn_native { namespace opengl { return pipeline; } + ComputePipeline::~ComputePipeline() { + DeleteProgram(ToBackend(GetDevice())->gl); + } + MaybeError ComputePipeline::Initialize() { DAWN_TRY( InitializeBase(ToBackend(GetDevice())->gl, ToBackend(GetLayout()), GetAllStages())); diff --git a/src/dawn_native/opengl/ComputePipelineGL.h b/src/dawn_native/opengl/ComputePipelineGL.h index 8e7744060b..dd5c0b3f36 100644 --- a/src/dawn_native/opengl/ComputePipelineGL.h +++ b/src/dawn_native/opengl/ComputePipelineGL.h @@ -35,7 +35,7 @@ namespace dawn_native { namespace opengl { private: using ComputePipelineBase::ComputePipelineBase; - ~ComputePipeline() override = default; + ~ComputePipeline() override; MaybeError Initialize() override; }; diff --git a/src/dawn_native/opengl/PipelineGL.cpp b/src/dawn_native/opengl/PipelineGL.cpp index 8ec9d61c82..c2fcbe6eac 100644 --- a/src/dawn_native/opengl/PipelineGL.cpp +++ b/src/dawn_native/opengl/PipelineGL.cpp @@ -45,7 +45,9 @@ namespace dawn_native { namespace opengl { } // namespace - PipelineGL::PipelineGL() = default; + PipelineGL::PipelineGL() : mProgram(0) { + } + PipelineGL::~PipelineGL() = default; MaybeError PipelineGL::InitializeBase(const OpenGLFunctions& gl, @@ -87,6 +89,7 @@ namespace dawn_native { namespace opengl { // Create an OpenGL shader for each stage and gather the list of combined samplers. PerStage combinedSamplers; bool needsDummySampler = false; + std::vector glShaders; for (SingleShaderStage stage : IterateStages(activeStages)) { const ShaderModule* module = ToBackend(stages[stage].module.Get()); std::string glsl; @@ -96,6 +99,7 @@ namespace dawn_native { namespace opengl { GLuint shader; DAWN_TRY_ASSIGN(shader, CreateShader(gl, GLShaderType(stage), glsl.c_str())); gl.AttachShader(mProgram, shader); + glShaders.push_back(shader); } if (needsDummySampler) { @@ -178,9 +182,19 @@ namespace dawn_native { namespace opengl { textureUnit++; } + + for (GLuint glShader : glShaders) { + gl.DetachShader(mProgram, glShader); + gl.DeleteShader(glShader); + } + return {}; } + void PipelineGL::DeleteProgram(const OpenGLFunctions& gl) { + gl.DeleteProgram(mProgram); + } + const std::vector& PipelineGL::GetTextureUnitsForSampler( GLuint index) const { ASSERT(index < mUnitsForSamplers.size()); diff --git a/src/dawn_native/opengl/PipelineGL.h b/src/dawn_native/opengl/PipelineGL.h index e21060696a..be6c1dd553 100644 --- a/src/dawn_native/opengl/PipelineGL.h +++ b/src/dawn_native/opengl/PipelineGL.h @@ -47,12 +47,12 @@ namespace dawn_native { namespace opengl { const std::vector& GetTextureUnitsForTextureView(GLuint index) const; GLuint GetProgramHandle() const; - void ApplyNow(const OpenGLFunctions& gl); - protected: + void ApplyNow(const OpenGLFunctions& gl); MaybeError InitializeBase(const OpenGLFunctions& gl, const PipelineLayout* layout, const PerStage& stages); + void DeleteProgram(const OpenGLFunctions& gl); private: GLuint mProgram; diff --git a/src/dawn_native/opengl/RenderPipelineGL.cpp b/src/dawn_native/opengl/RenderPipelineGL.cpp index b6a5a4fb6f..1b4b5cee74 100644 --- a/src/dawn_native/opengl/RenderPipelineGL.cpp +++ b/src/dawn_native/opengl/RenderPipelineGL.cpp @@ -238,6 +238,7 @@ namespace dawn_native { namespace opengl { const OpenGLFunctions& gl = ToBackend(GetDevice())->gl; gl.DeleteVertexArrays(1, &mVertexArrayObject); gl.BindVertexArray(0); + DeleteProgram(gl); } GLenum RenderPipeline::GetGLPrimitiveTopology() const {