[Mesa-dev] [PATCH] i965: Fix slow leak of brw->wm.compile_data->store

[Kenneth Graunke]

On 11/06/2012 10:47 AM, Eric Anholt wrote:
> We were successfully freeing our compile data at context destroy, but until
> then we were allocating a new store every compile without freeing it.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=56019
> ---
>   src/mesa/drivers/dri/i965/brw_context.h |    1 -
>   src/mesa/drivers/dri/i965/brw_vtbl.c    |    2 --
>   src/mesa/drivers/dri/i965/brw_wm.c      |   23 ++++++-----------------
>   3 files changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index 97556cb..5520ca8 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -977,7 +977,6 @@ struct brw_context
>
>      struct {
>         struct brw_wm_prog_data *prog_data;
> -      struct brw_wm_compile *compile_data;
>
>         /** Input sizes, calculated from active vertex program.
>          * One bit per fragment program input attribute.
> diff --git a/src/mesa/drivers/dri/i965/brw_vtbl.c b/src/mesa/drivers/dri/i965/brw_vtbl.c
> index 3709777..0da6070 100644
> --- a/src/mesa/drivers/dri/i965/brw_vtbl.c
> +++ b/src/mesa/drivers/dri/i965/brw_vtbl.c
> @@ -72,8 +72,6 @@ static void brw_destroy_context( struct intel_context *intel )
>      brw_destroy_state(brw);
>      brw_draw_destroy( brw );
>
> -   ralloc_free(brw->wm.compile_data);
> -
>      dri_bo_release(&brw->curbe.curbe_bo);
>      dri_bo_release(&brw->vs.const_bo);
>      dri_bo_release(&brw->wm.const_bo);
> diff --git a/src/mesa/drivers/dri/i965/brw_wm.c b/src/mesa/drivers/dri/i965/brw_wm.c
> index c273cd8..2c9a6a0 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm.c
> @@ -215,20 +215,7 @@ bool do_wm_prog(struct brw_context *brw,
>      if (prog)
>         fs = prog->_LinkedShaders[MESA_SHADER_FRAGMENT];
>
> -   c = brw->wm.compile_data;
> -   if (c == NULL) {
> -      brw->wm.compile_data = rzalloc(NULL, struct brw_wm_compile);
> -      c = brw->wm.compile_data;
> -      if (c == NULL) {
> -         /* Ouch - big out of memory problem.  Can't continue
> -          * without triggering a segfault, no way to signal,
> -          * so just return.
> -          */
> -         return false;
> -      }
> -   } else {
> -      memset(c, 0, sizeof(*brw->wm.compile_data));
> -   }
> +   c = rzalloc(NULL, struct brw_wm_compile);
>
>      /* Allocate the references to the uniforms that will end up in the
>       * prog_data associated with the compiled program, and which will be freed
> @@ -239,14 +226,14 @@ bool do_wm_prog(struct brw_context *brw,
>         /* The backend also sometimes adds params for texture size. */
>         param_count += 2 * BRW_MAX_TEX_UNIT;
>
> -      c->prog_data.param = rzalloc_array(c, const float *, param_count);
> -      c->prog_data.pull_param = rzalloc_array(c, const float *, param_count);
> +      c->prog_data.param = rzalloc_array(NULL, const float *, param_count);
> +      c->prog_data.pull_param = rzalloc_array(NULL, const float *, param_count);
>      } else {
>         /* brw_wm_pass0.c will also add references to 0.0 and 1.0 which are
>          * uploaded as push parameters.
>          */
>         int param_count = (fp->program.Base.Parameters->NumParameters + 2) * 4;
> -      c->prog_data.param = rzalloc_array(c, const float *, param_count);
> +      c->prog_data.param = rzalloc_array(NULL, const float *, param_count);
>         /* The old backend never does pull constants. */
>         c->prog_data.pull_param = NULL;
>      }

This code mentions the old backend.  It either needs to die or get 
relabeled.

Otherwise, this looks fine to me.  You really don't need to be using 
ralloc for any of this...it's just adding overhead.  (Then again, I 
might file that in the 'meh' department...)

> @@ -288,6 +275,8 @@ bool do_wm_prog(struct brw_context *brw,
>   		    &c->prog_data, sizeof(c->prog_data),
>   		    &brw->wm.prog_offset, &brw->wm.prog_data);
>
> +   ralloc_free(c);
> +
>      return true;
>   }