On Monday, July 14, 2014 03:48:34 PM Ian Romanick wrote:
> From: Ian Romanick <ian.d.roman...@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.roman...@intel.com>
> Cc: Kenneth Graunke <kenn...@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); > }
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
