[Mesa-dev] mesa: Free the compiled shader IR after it has been linked.

Submitted by Eric Anholt on May 28, 2014, 8:57 p.m.

Details

Message ID 1401310653-27024-1-git-send-email-eric@anholt.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Eric Anholt May 28, 2014, 8:57 p.m.
If the shader compiled once, then we can compile it again.  Compiled
shaders almost always get used in just one program, so holding that
compiled IR until the program is freed is just a waste of memory.

On the other hand, if they are either reusing shader objects to compile
multiple times, or linking the same shader into multiple programs, we turn
off this memory savings hack to avoid spending CPU on recompiling.

Reduces peak memory allocation of glretrace of a trace of dolphin-emu by
5.5MB.  It seems like this should be a big deal to DOTA2, but it was
triggering RecompiledAnyShader, and I failed to see a benefit even if I
removed the RecompiledAnyShader check (which confuses me).
---
 src/mesa/main/mtypes.h          |  6 ++++++
 src/mesa/main/shaderapi.c       | 19 +++++++++++++++++++
 src/mesa/main/shaderapi.h       |  3 +++
 src/mesa/program/ir_to_mesa.cpp | 23 ++++++++++++++++++++++-
 4 files changed, 50 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 917d071..79a1514 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -4210,6 +4210,12 @@  struct gl_context
    /*@}*/
 
    /**
+    * Set if we've had to recompile any shaders in order to link them into a
+    * new program after we'd freed their IR.
+    */
+   bool RecompiledAnyShaders;
+
+   /**
     * False if this context was created without a config. This is needed
     * because the initial state of glDrawBuffers depends on this
     */
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 28739da..44e8db6 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -803,6 +803,8 @@  shader_source(struct gl_context *ctx, GLuint shader, const GLchar *source)
    if (!sh)
       return;
 
+   _mesa_glsl_lazy_recompile_ir(ctx, sh);
+
    /* free old shader source string and install new one */
    free((void *)sh->Source);
    sh->Source = source;
@@ -812,6 +814,23 @@  shader_source(struct gl_context *ctx, GLuint shader, const GLchar *source)
 #endif
 }
 
+/**
+ * Used to ensure that there is a valid set of IR in the program when we might
+ * have freed it after glLinkProgram() on the assumption that nobody would go
+ * looking for it again.
+*/
+void
+_mesa_glsl_lazy_recompile_ir(struct gl_context *ctx, struct gl_shader *sh)
+{
+   if (sh->ir || !sh->CompileStatus)
+      return;
+
+   _mesa_glsl_compile_shader(ctx, sh, false, false);
+   /* The recompile had better have succeeded! */
+   assert(sh->CompileStatus);
+
+   ctx->RecompiledAnyShaders = true;
+}
 
 /**
  * Compile a shader.
diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
index 17b05b3..4e6c31f 100644
--- a/src/mesa/main/shaderapi.h
+++ b/src/mesa/main/shaderapi.h
@@ -228,6 +228,9 @@  extern GLuint GLAPIENTRY
 _mesa_CreateShaderProgramv(GLenum type, GLsizei count,
                            const GLchar* const *strings);
 
+void
+_mesa_glsl_lazy_recompile_ir(struct gl_context *ctx, struct gl_shader *sh);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
index 59cf123..d4b66bf 100644
--- a/src/mesa/program/ir_to_mesa.cpp
+++ b/src/mesa/program/ir_to_mesa.cpp
@@ -3079,8 +3079,14 @@  _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
    prog->LinkStatus = GL_TRUE;
 
    for (i = 0; i < prog->NumShaders; i++) {
-      if (!prog->Shaders[i]->CompileStatus) {
+      struct gl_shader *sh = prog->Shaders[i];
+      if (!sh->CompileStatus) {
 	 linker_error(prog, "linking with uncompiled shader");
+      } else {
+         /* If we'd freed the compiled IR after a previous link in order to
+          * save memory, recreate it.
+          */
+         _mesa_glsl_lazy_recompile_ir(ctx, sh);
       }
    }
 
@@ -3104,6 +3110,21 @@  _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
 	 fprintf(stderr, "%s\n", prog->InfoLog);
       }
    }
+
+   /* Free the compiled shaders' IR now that they've been linked into a
+    * program.  This saves memory in the common case of people using each
+    * shader in one program, at the cost of needing to lazily recompile one
+    * set of them if somebody does actually use the same gl_shader in multiple
+    * gl_shader_programs.
+    */
+   if (prog->LinkStatus && !ctx->RecompiledAnyShaders) {
+      for (i = 0; i < prog->NumShaders; i++) {
+         struct gl_shader *sh = prog->Shaders[i];
+
+         ralloc_free(sh->ir);
+         sh->ir = NULL;
+      }
+   }
 }
 
 } /* extern "C" */

Comments

On 05/28/2014 01:57 PM, Eric Anholt wrote:
> If the shader compiled once, then we can compile it again.  Compiled
> shaders almost always get used in just one program, so holding that
> compiled IR until the program is freed is just a waste of memory.

Would this work with some madness like:

   glAttachShader(prog, sh1);
   glAttachShader(prog, sh2);
   glLinkProgram(prog);

   GLchar *empty = "";
   glShaderSource(sh1, 1, &empty, NULL);

   glBindAttribLocation(prog, 0, "foo");
   glLinkProgram(prog);

If I'm reading this patch correctly, it will try to recompile the new
souce string on the second glLinkProgram.

There are some things like this that I think would work very nicely with
glCreateShaderProgramv, but it may be difficult to get all the corner
cases right otherwise.

> On the other hand, if they are either reusing shader objects to compile
> multiple times, or linking the same shader into multiple programs, we turn
> off this memory savings hack to avoid spending CPU on recompiling.
> 
> Reduces peak memory allocation of glretrace of a trace of dolphin-emu by
> 5.5MB.  It seems like this should be a big deal to DOTA2, but it was
> triggering RecompiledAnyShader, and I failed to see a benefit even if I
> removed the RecompiledAnyShader check (which confuses me).

I think they use the same vertex shader with multiple fragment
shaders... but it does seem weird that disabling the check didn't change
the memory usage.  Did it change anything (bad rendering)?

> ---
>  src/mesa/main/mtypes.h          |  6 ++++++
>  src/mesa/main/shaderapi.c       | 19 +++++++++++++++++++
>  src/mesa/main/shaderapi.h       |  3 +++
>  src/mesa/program/ir_to_mesa.cpp | 23 ++++++++++++++++++++++-
>  4 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 917d071..79a1514 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -4210,6 +4210,12 @@ struct gl_context
>     /*@}*/
>  
>     /**
> +    * Set if we've had to recompile any shaders in order to link them into a
> +    * new program after we'd freed their IR.
> +    */
> +   bool RecompiledAnyShaders;
> +
> +   /**
>      * False if this context was created without a config. This is needed
>      * because the initial state of glDrawBuffers depends on this
>      */
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 28739da..44e8db6 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -803,6 +803,8 @@ shader_source(struct gl_context *ctx, GLuint shader, const GLchar *source)
>     if (!sh)
>        return;
>  
> +   _mesa_glsl_lazy_recompile_ir(ctx, sh);
> +
>     /* free old shader source string and install new one */
>     free((void *)sh->Source);
>     sh->Source = source;
> @@ -812,6 +814,23 @@ shader_source(struct gl_context *ctx, GLuint shader, const GLchar *source)
>  #endif
>  }
>  
> +/**
> + * Used to ensure that there is a valid set of IR in the program when we might
> + * have freed it after glLinkProgram() on the assumption that nobody would go
> + * looking for it again.
> +*/
> +void
> +_mesa_glsl_lazy_recompile_ir(struct gl_context *ctx, struct gl_shader *sh)
> +{
> +   if (sh->ir || !sh->CompileStatus)
> +      return;
> +
> +   _mesa_glsl_compile_shader(ctx, sh, false, false);
> +   /* The recompile had better have succeeded! */
> +   assert(sh->CompileStatus);
> +
> +   ctx->RecompiledAnyShaders = true;
> +}
>  
>  /**
>   * Compile a shader.
> diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h
> index 17b05b3..4e6c31f 100644
> --- a/src/mesa/main/shaderapi.h
> +++ b/src/mesa/main/shaderapi.h
> @@ -228,6 +228,9 @@ extern GLuint GLAPIENTRY
>  _mesa_CreateShaderProgramv(GLenum type, GLsizei count,
>                             const GLchar* const *strings);
>  
> +void
> +_mesa_glsl_lazy_recompile_ir(struct gl_context *ctx, struct gl_shader *sh);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
> index 59cf123..d4b66bf 100644
> --- a/src/mesa/program/ir_to_mesa.cpp
> +++ b/src/mesa/program/ir_to_mesa.cpp
> @@ -3079,8 +3079,14 @@ _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
>     prog->LinkStatus = GL_TRUE;
>  
>     for (i = 0; i < prog->NumShaders; i++) {
> -      if (!prog->Shaders[i]->CompileStatus) {
> +      struct gl_shader *sh = prog->Shaders[i];
> +      if (!sh->CompileStatus) {
>  	 linker_error(prog, "linking with uncompiled shader");
> +      } else {
> +         /* If we'd freed the compiled IR after a previous link in order to
> +          * save memory, recreate it.
> +          */
> +         _mesa_glsl_lazy_recompile_ir(ctx, sh);
>        }
>     }
>  
> @@ -3104,6 +3110,21 @@ _mesa_glsl_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
>  	 fprintf(stderr, "%s\n", prog->InfoLog);
>        }
>     }
> +
> +   /* Free the compiled shaders' IR now that they've been linked into a
> +    * program.  This saves memory in the common case of people using each
> +    * shader in one program, at the cost of needing to lazily recompile one
> +    * set of them if somebody does actually use the same gl_shader in multiple
> +    * gl_shader_programs.
> +    */
> +   if (prog->LinkStatus && !ctx->RecompiledAnyShaders) {
> +      for (i = 0; i < prog->NumShaders; i++) {
> +         struct gl_shader *sh = prog->Shaders[i];
> +
> +         ralloc_free(sh->ir);
> +         sh->ir = NULL;
> +      }
> +   }
>  }
>  
>  } /* extern "C" */
Ian Romanick <idr@freedesktop.org> writes:

> On 05/28/2014 01:57 PM, Eric Anholt wrote:
>> If the shader compiled once, then we can compile it again.  Compiled
>> shaders almost always get used in just one program, so holding that
>> compiled IR until the program is freed is just a waste of memory.
>
> Would this work with some madness like:
>
>    glAttachShader(prog, sh1);
>    glAttachShader(prog, sh2);
>    glLinkProgram(prog);
>
>    GLchar *empty = "";
>    glShaderSource(sh1, 1, &empty, NULL);

When we get the shadersource call here on a previously linked-and-freed
shader, the lazy recompile call present in shadersource ensures that we
have the right IR.

>    glBindAttribLocation(prog, 0, "foo");
>    glLinkProgram(prog);

> There are some things like this that I think would work very nicely with
> glCreateShaderProgramv, but it may be difficult to get all the corner
> cases right otherwise.
>
>> On the other hand, if they are either reusing shader objects to compile
>> multiple times, or linking the same shader into multiple programs, we turn
>> off this memory savings hack to avoid spending CPU on recompiling.
>> 
>> Reduces peak memory allocation of glretrace of a trace of dolphin-emu by
>> 5.5MB.  It seems like this should be a big deal to DOTA2, but it was
>> triggering RecompiledAnyShader, and I failed to see a benefit even if I
>> removed the RecompiledAnyShader check (which confuses me).
>
> I think they use the same vertex shader with multiple fragment
> shaders... but it does seem weird that disabling the check didn't change
> the memory usage.  Did it change anything (bad rendering)?

Not that I noticed, not that I was paying close attention when it took
so long to run.
On 05/31/2014 02:39 PM, Eric Anholt wrote:
> Ian Romanick <idr@freedesktop.org> writes:
> 
>> On 05/28/2014 01:57 PM, Eric Anholt wrote:
>>> If the shader compiled once, then we can compile it again.  Compiled
>>> shaders almost always get used in just one program, so holding that
>>> compiled IR until the program is freed is just a waste of memory.
>>
>> Would this work with some madness like:
>>
>>    glAttachShader(prog, sh1);
>>    glAttachShader(prog, sh2);
>>    glLinkProgram(prog);
>>
>>    GLchar *empty = "";
>>    glShaderSource(sh1, 1, &empty, NULL);
> 
> When we get the shadersource call here on a previously linked-and-freed
> shader, the lazy recompile call present in shadersource ensures that we
> have the right IR.

Ah.. I did miss that on the first read.  Maybe add a comment there?
Either way,

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

>>    glBindAttribLocation(prog, 0, "foo");
>>    glLinkProgram(prog);
> 
>> There are some things like this that I think would work very nicely with
>> glCreateShaderProgramv, but it may be difficult to get all the corner
>> cases right otherwise.
>>
>>> On the other hand, if they are either reusing shader objects to compile
>>> multiple times, or linking the same shader into multiple programs, we turn
>>> off this memory savings hack to avoid spending CPU on recompiling.
>>>
>>> Reduces peak memory allocation of glretrace of a trace of dolphin-emu by
>>> 5.5MB.  It seems like this should be a big deal to DOTA2, but it was
>>> triggering RecompiledAnyShader, and I failed to see a benefit even if I
>>> removed the RecompiledAnyShader check (which confuses me).
>>
>> I think they use the same vertex shader with multiple fragment
>> shaders... but it does seem weird that disabling the check didn't change
>> the memory usage.  Did it change anything (bad rendering)?
> 
> Not that I noticed, not that I was paying close attention when it took
> so long to run.
On 05/28/2014 01:57 PM, Eric Anholt wrote:
> If the shader compiled once, then we can compile it again.  Compiled
> shaders almost always get used in just one program, so holding that
> compiled IR until the program is freed is just a waste of memory.
> 
> On the other hand, if they are either reusing shader objects to compile
> multiple times, or linking the same shader into multiple programs, we turn
> off this memory savings hack to avoid spending CPU on recompiling.
> 
> Reduces peak memory allocation of glretrace of a trace of dolphin-emu by
> 5.5MB.  It seems like this should be a big deal to DOTA2, but it was
> triggering RecompiledAnyShader, and I failed to see a benefit even if I
> removed the RecompiledAnyShader check (which confuses me).

Does dota2 DeleteShader after AttachShader / LinkProgram?  That could
explain why it doesn't help there...