[Mesa-dev] [PATCH 02/26] glsl: Rebuild the symbol table without unreachable symbols

[Kenneth Graunke]

On Monday, July 14, 2014 03:48:34 PM Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> Previously we had to keep unreachable global symbols in the symbol table
> because the symbol table is used during linking.  Having the symbol
> table retain pointers to freed memory... what could possibly go wrong?
> At the same time, this meant that we kept live references to tons of
> memory that was no longer needed.
> 
> New strategy:  destroy the old symbol table, and make a new one from the
> reachable symbols.
> 
> Valgrind massif results for a trimmed apitrace of dota2:
> 
>                   n        time(i)         total(B)   useful-heap(B) extra-
heap(B)    stacks(B)
> Before (32-bit): 59 40,642,425,451       76,337,968       69,720,886     
6,617,082            0
> After  (32-bit): 46 40,661,487,174       75,116,800       68,854,065     
6,262,735            0
> 
> Before (64-bit): 79 37,179,441,771      106,986,512       98,112,095     
8,874,417            0
> After  (64-bit): 64 37,200,329,700      104,872,672       96,514,546     
8,358,126            0
> 
> A real savings of 846KiB on 32-bit and 1.5MiB on 64-bit.
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/glsl_parser_extras.cpp | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/glsl_parser_extras.cpp 
b/src/glsl/glsl_parser_extras.cpp
> index b327c2b..2962407 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -1485,7 +1485,7 @@ _mesa_glsl_compile_shader(struct gl_context *ctx, 
struct gl_shader *shader,
>     if (shader->InfoLog)
>        ralloc_free(shader->InfoLog);
>  
> -   shader->symbols = state->symbols;
> +   shader->symbols = new(shader->ir) glsl_symbol_table;
>     shader->CompileStatus = !state->error;
>     shader->InfoLog = state->info_log;
>     shader->Version = state->language_version;
> @@ -1498,6 +1498,41 @@ _mesa_glsl_compile_shader(struct gl_context *ctx, 
struct gl_shader *shader,
>     /* Retain any live IR, but trash the rest. */
>     reparent_ir(shader->ir, shader->ir);
>  
> +   /* Destroy the symbol table.  Create a new symbol table that contains 
only
> +    * the variables and functions that still exist in the IR.  The symbol
> +    * table will be used later during linking.
> +    *
> +    * There must NOT be any freed objects still referenced by the symbol
> +    * table.  That could cause the linker to dereference freed memory.
> +    *
> +    * We don't have to worry about types or interface-types here because 
those
> +    * are fly-weights that are looked up by glsl_type.
> +    */
> +   foreach_in_list (ir_instruction, ir, shader->ir) {
> +      switch (ir->ir_type) {
> +      case ir_type_function: {
> +         /* If the function is a built-in that is partially overridden in 
the
> +          * shader, the ir_function stored in the symbol table may not be 
the
> +          * same as the one that appears in the shader.  The one in the 
shader
> +          * will only include definitions from inside the shader.  We need 
the
> +          * one from the symbol table because it will include built-in
> +          * defintions and definitions from the shader.
> +          */
> +         ir_function *const def = (ir_function *) ir;
> +         ir_function *const decl = state->symbols->get_function(def->name);

Huh.  This doesn't match my understanding of the code.  I believe there 
should only be one ir_function with a given name, and it should be both in the 
shader's instruction stream and in the symbol table.

That single ir_function should contain any definitions and prototypes created 
in the shader's GLSL code, and also prototypes (but not definitions) for any 
built-in signatures called.

If you look at match_function_by_name(), it always uses the existing function, 
if there is one, or creates a new one and adds it in both places...walking 
through a couple scenarios, I haven't seen what's going wrong.

I suppose I should try the obvious

   shader->symobls->add_function((ir_function *) ir);

and see what breaks.  It sure doesn't seem like anything should.  I'll try to 
look into it soon.

> +         shader->symbols->add_function(decl);
> +         break;
> +      }
> +      case ir_type_variable:
> +         shader->symbols->add_variable((ir_variable *) ir);
> +         break;
> +      default:
> +         break;
> +      }
> +   }
> +
> +   delete state->symbols;
>     ralloc_free(state);
>  }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140715/7e0f3720/attachment.sig>