Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
On 06.03.2018 14:07, Emil Velikov wrote: On 6 March 2018 at 07:32, Tapani Pälli wrote: Hi; On 01.03.2018 03:12, Kenneth Graunke wrote: On Thursday, February 15, 2018 1:12:54 AM PST Tapani Pälli wrote: From: Simon Hausmann When looking up known glsl_type instances in the various hash tables, we end up leaking the key instances used for the lookup, as the glsl_type constructor allocates memory on the global mem_ctx. This patch changes glsl_type to manage its own memory, which fixes the leak and also allows getting rid of the global mem_ctx and its mutex. v2: remove lambda usage (Tapani) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 Signed-off-by: Simon Hausmann Signed-off-by: Tapani Pälli --- src/compiler/glsl_types.cpp | 83 - src/compiler/glsl_types.h | 44 +++- 2 files changed, 41 insertions(+), 86 deletions(-) diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 3cc5eb0495..81ff97b1f7 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -28,23 +28,12 @@ #include "util/hash_table.h" -mtx_t glsl_type::mem_mutex = _MTX_INITIALIZER_NP; mtx_t glsl_type::hash_mutex = _MTX_INITIALIZER_NP; hash_table *glsl_type::array_types = NULL; hash_table *glsl_type::record_types = NULL; hash_table *glsl_type::interface_types = NULL; hash_table *glsl_type::function_types = NULL; hash_table *glsl_type::subroutine_types = NULL; -void *glsl_type::mem_ctx = NULL; - -void -glsl_type::init_ralloc_type_ctx(void) -{ - if (glsl_type::mem_ctx == NULL) { - glsl_type::mem_ctx = ralloc_context(NULL); - assert(glsl_type::mem_ctx != NULL); - } -} glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, unsigned vector_elements, @@ -63,14 +52,12 @@ glsl_type::glsl_type(GLenum gl_type, STATIC_ASSERT((unsigned(GLSL_TYPE_INT) & 3) == unsigned(GLSL_TYPE_INT)); STATIC_ASSERT((unsigned(GLSL_TYPE_FLOAT) & 3) == unsigned(GLSL_TYPE_FLOAT)); - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); - mtx_unlock(&glsl_type::mem_mutex); - /* Neither dimension is zero or both dimensions are zero. */ assert((vector_elements == 0) == (matrix_columns == 0)); @@ -86,14 +73,12 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, sampler_array(array), interface_packing(0), interface_row_major(0), length(0) { - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); - mtx_unlock(&glsl_type::mem_mutex); - memset(& fields, 0, sizeof(fields)); matrix_columns = vector_elements = 1; @@ -110,9 +95,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); this->fields.structure = ralloc_array(this->mem_ctx, @@ -123,8 +108,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, this->fields.structure[i].name = ralloc_strdup(this->fields.structure, fields[i].name); } - - mtx_unlock(&glsl_type::mem_mutex); } glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, @@ -140,9 +123,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); this->fields.structure = rzalloc_array(this->mem_ctx, @@ -152,8 +135,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, this->fields.structure[i].name = ralloc_strdup(this->fields.structure, fields[i].name); } - - mtx_unlock(&glsl_type::mem_mutex); } glsl_type::glsl_type(const glsl_type *return_type, @@ -167,9 +148,8 @@ glsl_type::glsl_type(const glsl_type *return_type, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); - - init_ralloc_type_ctx(); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); this->fields.parameters = rzalloc_array(this->mem_ctx, glsl_function_param, num_params + 1); @@ -185,8 +165,6
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
On 6 March 2018 at 07:32, Tapani Pälli wrote: > Hi; > > > On 01.03.2018 03:12, Kenneth Graunke wrote: >> >> On Thursday, February 15, 2018 1:12:54 AM PST Tapani Pälli wrote: >>> >>> From: Simon Hausmann >>> >>> When looking up known glsl_type instances in the various hash tables, we >>> end up leaking the key instances used for the lookup, as the glsl_type >>> constructor allocates memory on the global mem_ctx. This patch changes >>> glsl_type to manage its own memory, which fixes the leak and also allows >>> getting rid of the global mem_ctx and its mutex. >>> >>> v2: remove lambda usage (Tapani) >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 >>> Signed-off-by: Simon Hausmann >>> Signed-off-by: Tapani Pälli >>> --- >>> src/compiler/glsl_types.cpp | 83 >>> - >>> src/compiler/glsl_types.h | 44 +++- >>> 2 files changed, 41 insertions(+), 86 deletions(-) >>> >>> diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp >>> index 3cc5eb0495..81ff97b1f7 100644 >>> --- a/src/compiler/glsl_types.cpp >>> +++ b/src/compiler/glsl_types.cpp >>> @@ -28,23 +28,12 @@ >>> #include "util/hash_table.h" >>> -mtx_t glsl_type::mem_mutex = _MTX_INITIALIZER_NP; >>> mtx_t glsl_type::hash_mutex = _MTX_INITIALIZER_NP; >>> hash_table *glsl_type::array_types = NULL; >>> hash_table *glsl_type::record_types = NULL; >>> hash_table *glsl_type::interface_types = NULL; >>> hash_table *glsl_type::function_types = NULL; >>> hash_table *glsl_type::subroutine_types = NULL; >>> -void *glsl_type::mem_ctx = NULL; >>> - >>> -void >>> -glsl_type::init_ralloc_type_ctx(void) >>> -{ >>> - if (glsl_type::mem_ctx == NULL) { >>> - glsl_type::mem_ctx = ralloc_context(NULL); >>> - assert(glsl_type::mem_ctx != NULL); >>> - } >>> -} >>> glsl_type::glsl_type(GLenum gl_type, >>>glsl_base_type base_type, unsigned >>> vector_elements, >>> @@ -63,14 +52,12 @@ glsl_type::glsl_type(GLenum gl_type, >>> STATIC_ASSERT((unsigned(GLSL_TYPE_INT) & 3) == >>> unsigned(GLSL_TYPE_INT)); >>> STATIC_ASSERT((unsigned(GLSL_TYPE_FLOAT) & 3) == >>> unsigned(GLSL_TYPE_FLOAT)); >>> - mtx_lock(&glsl_type::mem_mutex); >>> + this->mem_ctx = ralloc_context(NULL); >>> + assert(this->mem_ctx != NULL); >>> - init_ralloc_type_ctx(); >>> assert(name != NULL); >>> this->name = ralloc_strdup(this->mem_ctx, name); >>> - mtx_unlock(&glsl_type::mem_mutex); >>> - >>> /* Neither dimension is zero or both dimensions are zero. >>> */ >>> assert((vector_elements == 0) == (matrix_columns == 0)); >>> @@ -86,14 +73,12 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type >>> base_type, >>> sampler_array(array), interface_packing(0), >>> interface_row_major(0), length(0) >>> { >>> - mtx_lock(&glsl_type::mem_mutex); >>> + this->mem_ctx = ralloc_context(NULL); >>> + assert(this->mem_ctx != NULL); >>> - init_ralloc_type_ctx(); >>> assert(name != NULL); >>> this->name = ralloc_strdup(this->mem_ctx, name); >>> - mtx_unlock(&glsl_type::mem_mutex); >>> - >>> memset(& fields, 0, sizeof(fields)); >>>matrix_columns = vector_elements = 1; >>> @@ -110,9 +95,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, >>> unsigned num_fields, >>> { >>> unsigned int i; >>> - mtx_lock(&glsl_type::mem_mutex); >>> + this->mem_ctx = ralloc_context(NULL); >>> + assert(this->mem_ctx != NULL); >>> - init_ralloc_type_ctx(); >>> assert(name != NULL); >>> this->name = ralloc_strdup(this->mem_ctx, name); >>> this->fields.structure = ralloc_array(this->mem_ctx, >>> @@ -123,8 +108,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, >>> unsigned num_fields, >>> this->fields.structure[i].name = >>> ralloc_strdup(this->fields.structure, >>>fields[i].name); >>> } >>> - >>> - mtx_unlock(&glsl_type::mem_mutex); >>> } >>> glsl_type::glsl_type(const glsl_struct_field *fields, unsigned >>> num_fields, >>> @@ -140,9 +123,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, >>> unsigned num_fields, >>> { >>> unsigned int i; >>> - mtx_lock(&glsl_type::mem_mutex); >>> + this->mem_ctx = ralloc_context(NULL); >>> + assert(this->mem_ctx != NULL); >>> - init_ralloc_type_ctx(); >>> assert(name != NULL); >>> this->name = ralloc_strdup(this->mem_ctx, name); >>> this->fields.structure = rzalloc_array(this->mem_ctx, >>> @@ -152,8 +135,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, >>> unsigned num_fields, >>> this->fields.structure[i].name = >>> ralloc_strdup(this->fields.structure, >>>fields[i].name); >>> } >>> - >>> - mtx_unlock(&glsl_type::mem_mutex); >>> } >>> glsl_type::glsl_type(const glsl_type *return_type, >>> @@ -167,9 +148,8 @@ gls
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
Hi; On 01.03.2018 03:12, Kenneth Graunke wrote: On Thursday, February 15, 2018 1:12:54 AM PST Tapani Pälli wrote: From: Simon Hausmann When looking up known glsl_type instances in the various hash tables, we end up leaking the key instances used for the lookup, as the glsl_type constructor allocates memory on the global mem_ctx. This patch changes glsl_type to manage its own memory, which fixes the leak and also allows getting rid of the global mem_ctx and its mutex. v2: remove lambda usage (Tapani) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 Signed-off-by: Simon Hausmann Signed-off-by: Tapani Pälli --- src/compiler/glsl_types.cpp | 83 - src/compiler/glsl_types.h | 44 +++- 2 files changed, 41 insertions(+), 86 deletions(-) diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 3cc5eb0495..81ff97b1f7 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -28,23 +28,12 @@ #include "util/hash_table.h" -mtx_t glsl_type::mem_mutex = _MTX_INITIALIZER_NP; mtx_t glsl_type::hash_mutex = _MTX_INITIALIZER_NP; hash_table *glsl_type::array_types = NULL; hash_table *glsl_type::record_types = NULL; hash_table *glsl_type::interface_types = NULL; hash_table *glsl_type::function_types = NULL; hash_table *glsl_type::subroutine_types = NULL; -void *glsl_type::mem_ctx = NULL; - -void -glsl_type::init_ralloc_type_ctx(void) -{ - if (glsl_type::mem_ctx == NULL) { - glsl_type::mem_ctx = ralloc_context(NULL); - assert(glsl_type::mem_ctx != NULL); - } -} glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, unsigned vector_elements, @@ -63,14 +52,12 @@ glsl_type::glsl_type(GLenum gl_type, STATIC_ASSERT((unsigned(GLSL_TYPE_INT) & 3) == unsigned(GLSL_TYPE_INT)); STATIC_ASSERT((unsigned(GLSL_TYPE_FLOAT) & 3) == unsigned(GLSL_TYPE_FLOAT)); - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); - mtx_unlock(&glsl_type::mem_mutex); - /* Neither dimension is zero or both dimensions are zero. */ assert((vector_elements == 0) == (matrix_columns == 0)); @@ -86,14 +73,12 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, sampler_array(array), interface_packing(0), interface_row_major(0), length(0) { - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); - mtx_unlock(&glsl_type::mem_mutex); - memset(& fields, 0, sizeof(fields)); matrix_columns = vector_elements = 1; @@ -110,9 +95,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); this->fields.structure = ralloc_array(this->mem_ctx, @@ -123,8 +108,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, this->fields.structure[i].name = ralloc_strdup(this->fields.structure, fields[i].name); } - - mtx_unlock(&glsl_type::mem_mutex); } glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, @@ -140,9 +123,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); this->fields.structure = rzalloc_array(this->mem_ctx, @@ -152,8 +135,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, this->fields.structure[i].name = ralloc_strdup(this->fields.structure, fields[i].name); } - - mtx_unlock(&glsl_type::mem_mutex); } glsl_type::glsl_type(const glsl_type *return_type, @@ -167,9 +148,8 @@ glsl_type::glsl_type(const glsl_type *return_type, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); - - init_ralloc_type_ctx(); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); this->fields.parameters = rzalloc_array(this->mem_ctx, glsl_function_param, num_params + 1); @@ -185,8 +165,6 @@ glsl_type::glsl_type(const glsl_type *return_type, this->fields.parameters[i + 1].in = params[i].in;
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
On Thursday, February 15, 2018 1:12:54 AM PST Tapani Pälli wrote: > From: Simon Hausmann > > When looking up known glsl_type instances in the various hash tables, we > end up leaking the key instances used for the lookup, as the glsl_type > constructor allocates memory on the global mem_ctx. This patch changes > glsl_type to manage its own memory, which fixes the leak and also allows > getting rid of the global mem_ctx and its mutex. > > v2: remove lambda usage (Tapani) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 > Signed-off-by: Simon Hausmann > Signed-off-by: Tapani Pälli > --- > src/compiler/glsl_types.cpp | 83 > - > src/compiler/glsl_types.h | 44 +++- > 2 files changed, 41 insertions(+), 86 deletions(-) > > diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp > index 3cc5eb0495..81ff97b1f7 100644 > --- a/src/compiler/glsl_types.cpp > +++ b/src/compiler/glsl_types.cpp > @@ -28,23 +28,12 @@ > #include "util/hash_table.h" > > > -mtx_t glsl_type::mem_mutex = _MTX_INITIALIZER_NP; > mtx_t glsl_type::hash_mutex = _MTX_INITIALIZER_NP; > hash_table *glsl_type::array_types = NULL; > hash_table *glsl_type::record_types = NULL; > hash_table *glsl_type::interface_types = NULL; > hash_table *glsl_type::function_types = NULL; > hash_table *glsl_type::subroutine_types = NULL; > -void *glsl_type::mem_ctx = NULL; > - > -void > -glsl_type::init_ralloc_type_ctx(void) > -{ > - if (glsl_type::mem_ctx == NULL) { > - glsl_type::mem_ctx = ralloc_context(NULL); > - assert(glsl_type::mem_ctx != NULL); > - } > -} > > glsl_type::glsl_type(GLenum gl_type, > glsl_base_type base_type, unsigned vector_elements, > @@ -63,14 +52,12 @@ glsl_type::glsl_type(GLenum gl_type, > STATIC_ASSERT((unsigned(GLSL_TYPE_INT) & 3) == unsigned(GLSL_TYPE_INT)); > STATIC_ASSERT((unsigned(GLSL_TYPE_FLOAT) & 3) == > unsigned(GLSL_TYPE_FLOAT)); > > - mtx_lock(&glsl_type::mem_mutex); > + this->mem_ctx = ralloc_context(NULL); > + assert(this->mem_ctx != NULL); > > - init_ralloc_type_ctx(); > assert(name != NULL); > this->name = ralloc_strdup(this->mem_ctx, name); > > - mtx_unlock(&glsl_type::mem_mutex); > - > /* Neither dimension is zero or both dimensions are zero. > */ > assert((vector_elements == 0) == (matrix_columns == 0)); > @@ -86,14 +73,12 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type > base_type, > sampler_array(array), interface_packing(0), > interface_row_major(0), length(0) > { > - mtx_lock(&glsl_type::mem_mutex); > + this->mem_ctx = ralloc_context(NULL); > + assert(this->mem_ctx != NULL); > > - init_ralloc_type_ctx(); > assert(name != NULL); > this->name = ralloc_strdup(this->mem_ctx, name); > > - mtx_unlock(&glsl_type::mem_mutex); > - > memset(& fields, 0, sizeof(fields)); > > matrix_columns = vector_elements = 1; > @@ -110,9 +95,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, > unsigned num_fields, > { > unsigned int i; > > - mtx_lock(&glsl_type::mem_mutex); > + this->mem_ctx = ralloc_context(NULL); > + assert(this->mem_ctx != NULL); > > - init_ralloc_type_ctx(); > assert(name != NULL); > this->name = ralloc_strdup(this->mem_ctx, name); > this->fields.structure = ralloc_array(this->mem_ctx, > @@ -123,8 +108,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, > unsigned num_fields, >this->fields.structure[i].name = ralloc_strdup(this->fields.structure, > fields[i].name); > } > - > - mtx_unlock(&glsl_type::mem_mutex); > } > > glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, > @@ -140,9 +123,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, > unsigned num_fields, > { > unsigned int i; > > - mtx_lock(&glsl_type::mem_mutex); > + this->mem_ctx = ralloc_context(NULL); > + assert(this->mem_ctx != NULL); > > - init_ralloc_type_ctx(); > assert(name != NULL); > this->name = ralloc_strdup(this->mem_ctx, name); > this->fields.structure = rzalloc_array(this->mem_ctx, > @@ -152,8 +135,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, > unsigned num_fields, >this->fields.structure[i].name = ralloc_strdup(this->fields.structure, > fields[i].name); > } > - > - mtx_unlock(&glsl_type::mem_mutex); > } > > glsl_type::glsl_type(const glsl_type *return_type, > @@ -167,9 +148,8 @@ glsl_type::glsl_type(const glsl_type *return_type, > { > unsigned int i; > > - mtx_lock(&glsl_type::mem_mutex); > - > - init_ralloc_type_ctx(); > + this->mem_ctx = ralloc_context(NULL); > + assert(this->mem_ctx != NULL); > > this->fields.parameters = rzalloc_array(this->mem_ctx, > glsl_function_param, num
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
On 27 February 2018 at 05:43, Tapani Pälli wrote: > > > On 02/26/2018 07:55 PM, Emil Velikov wrote: >> >> On 15 February 2018 at 09:12, Tapani Pälli wrote: >>> >>> From: Simon Hausmann >>> >>> When looking up known glsl_type instances in the various hash tables, we >>> end up leaking the key instances used for the lookup, as the glsl_type >>> constructor allocates memory on the global mem_ctx. This patch changes >>> glsl_type to manage its own memory, which fixes the leak and also allows >>> getting rid of the global mem_ctx and its mutex. >>> >>> v2: remove lambda usage (Tapani) >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 >>> Signed-off-by: Simon Hausmann >>> Signed-off-by: Tapani Pälli >>> --- >>> src/compiler/glsl_types.cpp | 83 >>> - >>> src/compiler/glsl_types.h | 44 +++- >>> 2 files changed, 41 insertions(+), 86 deletions(-) >>> >> Great overall result, there's a small question below. >> >> Can you include the mesa-stable tag and/or the commit that introduced >> this shared memory pool. >> ... Gut feeling says it was before the file started using ralloc. > > > Sure, can add this. > >> >>> @@ -970,7 +961,7 @@ glsl_type::get_array_instance(const glsl_type *base, >>> unsigned array_size) >>> const glsl_type *t = new glsl_type(base, array_size); >>> >>> entry = _mesa_hash_table_insert(array_types, >>> - ralloc_strdup(mem_ctx, key), >>> + strdup(key), >>> (void *) t); >>> } >>> >> >> Are you sure we need the strdup here? AFAICT nothing clears it so it >> will be leaked. >> > > Yes I believe we do need it, see comment about base type name some rows > before. It is being freed during _mesa_glsl_release_types() by the > hash_free_type_function when array_types hash is iterated. > Right - could swear I saw _mesa_hash_table_insert duplicating the key. That's not the case, so everything will be torn as you mentioned. Thanks Tapani! With a stable tag. the patch is Reviewed-by: Emil Velikov -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
On 02/26/2018 07:55 PM, Emil Velikov wrote: On 15 February 2018 at 09:12, Tapani Pälli wrote: From: Simon Hausmann When looking up known glsl_type instances in the various hash tables, we end up leaking the key instances used for the lookup, as the glsl_type constructor allocates memory on the global mem_ctx. This patch changes glsl_type to manage its own memory, which fixes the leak and also allows getting rid of the global mem_ctx and its mutex. v2: remove lambda usage (Tapani) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 Signed-off-by: Simon Hausmann Signed-off-by: Tapani Pälli --- src/compiler/glsl_types.cpp | 83 - src/compiler/glsl_types.h | 44 +++- 2 files changed, 41 insertions(+), 86 deletions(-) Great overall result, there's a small question below. Can you include the mesa-stable tag and/or the commit that introduced this shared memory pool. ... Gut feeling says it was before the file started using ralloc. Sure, can add this. @@ -970,7 +961,7 @@ glsl_type::get_array_instance(const glsl_type *base, unsigned array_size) const glsl_type *t = new glsl_type(base, array_size); entry = _mesa_hash_table_insert(array_types, - ralloc_strdup(mem_ctx, key), + strdup(key), (void *) t); } Are you sure we need the strdup here? AFAICT nothing clears it so it will be leaked. Yes I believe we do need it, see comment about base type name some rows before. It is being freed during _mesa_glsl_release_types() by the hash_free_type_function when array_types hash is iterated. // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
On 15 February 2018 at 09:12, Tapani Pälli wrote: > From: Simon Hausmann > > When looking up known glsl_type instances in the various hash tables, we > end up leaking the key instances used for the lookup, as the glsl_type > constructor allocates memory on the global mem_ctx. This patch changes > glsl_type to manage its own memory, which fixes the leak and also allows > getting rid of the global mem_ctx and its mutex. > > v2: remove lambda usage (Tapani) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 > Signed-off-by: Simon Hausmann > Signed-off-by: Tapani Pälli > --- > src/compiler/glsl_types.cpp | 83 > - > src/compiler/glsl_types.h | 44 +++- > 2 files changed, 41 insertions(+), 86 deletions(-) > Great overall result, there's a small question below. Can you include the mesa-stable tag and/or the commit that introduced this shared memory pool. ... Gut feeling says it was before the file started using ralloc. > @@ -970,7 +961,7 @@ glsl_type::get_array_instance(const glsl_type *base, > unsigned array_size) >const glsl_type *t = new glsl_type(base, array_size); > >entry = _mesa_hash_table_insert(array_types, > - ralloc_strdup(mem_ctx, key), > + strdup(key), >(void *) t); > } > Are you sure we need the strdup here? AFAICT nothing clears it so it will be leaked. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
ping .. On 02/15/2018 11:12 AM, Tapani Pälli wrote: From: Simon Hausmann When looking up known glsl_type instances in the various hash tables, we end up leaking the key instances used for the lookup, as the glsl_type constructor allocates memory on the global mem_ctx. This patch changes glsl_type to manage its own memory, which fixes the leak and also allows getting rid of the global mem_ctx and its mutex. v2: remove lambda usage (Tapani) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884 Signed-off-by: Simon Hausmann Signed-off-by: Tapani Pälli --- src/compiler/glsl_types.cpp | 83 - src/compiler/glsl_types.h | 44 +++- 2 files changed, 41 insertions(+), 86 deletions(-) diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 3cc5eb0495..81ff97b1f7 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -28,23 +28,12 @@ #include "util/hash_table.h" -mtx_t glsl_type::mem_mutex = _MTX_INITIALIZER_NP; mtx_t glsl_type::hash_mutex = _MTX_INITIALIZER_NP; hash_table *glsl_type::array_types = NULL; hash_table *glsl_type::record_types = NULL; hash_table *glsl_type::interface_types = NULL; hash_table *glsl_type::function_types = NULL; hash_table *glsl_type::subroutine_types = NULL; -void *glsl_type::mem_ctx = NULL; - -void -glsl_type::init_ralloc_type_ctx(void) -{ - if (glsl_type::mem_ctx == NULL) { - glsl_type::mem_ctx = ralloc_context(NULL); - assert(glsl_type::mem_ctx != NULL); - } -} glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, unsigned vector_elements, @@ -63,14 +52,12 @@ glsl_type::glsl_type(GLenum gl_type, STATIC_ASSERT((unsigned(GLSL_TYPE_INT) & 3) == unsigned(GLSL_TYPE_INT)); STATIC_ASSERT((unsigned(GLSL_TYPE_FLOAT) & 3) == unsigned(GLSL_TYPE_FLOAT)); - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); - mtx_unlock(&glsl_type::mem_mutex); - /* Neither dimension is zero or both dimensions are zero. */ assert((vector_elements == 0) == (matrix_columns == 0)); @@ -86,14 +73,12 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, sampler_array(array), interface_packing(0), interface_row_major(0), length(0) { - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); - mtx_unlock(&glsl_type::mem_mutex); - memset(& fields, 0, sizeof(fields)); matrix_columns = vector_elements = 1; @@ -110,9 +95,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); this->fields.structure = ralloc_array(this->mem_ctx, @@ -123,8 +108,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, this->fields.structure[i].name = ralloc_strdup(this->fields.structure, fields[i].name); } - - mtx_unlock(&glsl_type::mem_mutex); } glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, @@ -140,9 +123,9 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); - init_ralloc_type_ctx(); assert(name != NULL); this->name = ralloc_strdup(this->mem_ctx, name); this->fields.structure = rzalloc_array(this->mem_ctx, @@ -152,8 +135,6 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, this->fields.structure[i].name = ralloc_strdup(this->fields.structure, fields[i].name); } - - mtx_unlock(&glsl_type::mem_mutex); } glsl_type::glsl_type(const glsl_type *return_type, @@ -167,9 +148,8 @@ glsl_type::glsl_type(const glsl_type *return_type, { unsigned int i; - mtx_lock(&glsl_type::mem_mutex); - - init_ralloc_type_ctx(); + this->mem_ctx = ralloc_context(NULL); + assert(this->mem_ctx != NULL); this->fields.parameters = rzalloc_array(this->mem_ctx, glsl_function_param, num_params + 1); @@ -185,8 +165,6 @@ glsl_type::glsl_type(const glsl_type *return_type, this->fields.parameters[i + 1].in = params[i].in; this->fields.parameters[i + 1].out = params[i].out; }