[Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
Kenneth Graunke
kenneth at whitecape.org
Thu Mar 1 01:12:13 UTC 2018
On Thursday, February 15, 2018 1:12:54 AM PST Tapani Pälli wrote:
> From: Simon Hausmann <simon.hausmann at qt.io>
>
> 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 <simon.hausmann at qt.io>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
> 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;
> }
> -
> - mtx_unlock(&glsl_type::mem_mutex);
> }
>
> glsl_type::glsl_type(const char *subroutine_name) :
> @@ -197,12 +175,16 @@ glsl_type::glsl_type(const char *subroutine_name) :
> vector_elements(1), matrix_columns(1),
> length(0)
> {
> - mtx_lock(&glsl_type::mem_mutex);
> + this->mem_ctx = ralloc_context(NULL);
> + assert(this->mem_ctx != NULL);
>
> - init_ralloc_type_ctx();
> assert(subroutine_name != NULL);
> this->name = ralloc_strdup(this->mem_ctx, subroutine_name);
> - mtx_unlock(&glsl_type::mem_mutex);
> +}
> +
> +glsl_type::~glsl_type()
> +{
> + ralloc_free(this->mem_ctx);
> }
>
> bool
> @@ -416,6 +398,17 @@ const glsl_type *glsl_type::get_scalar_type() const
> }
>
>
> +static void
> +hash_free_type_function(struct hash_entry *entry)
> +{
> + glsl_type *type = (glsl_type *) entry->data;
> +
> + if (type->is_array())
> + free((void*)entry->key);
> +
> + delete type;
> +}
> +
> void
> _mesa_glsl_release_types(void)
> {
> @@ -424,32 +417,29 @@ _mesa_glsl_release_types(void)
> * necessary.
> */
> if (glsl_type::array_types != NULL) {
> - _mesa_hash_table_destroy(glsl_type::array_types, NULL);
> + _mesa_hash_table_destroy(glsl_type::array_types, hash_free_type_function);
> glsl_type::array_types = NULL;
> }
>
> if (glsl_type::record_types != NULL) {
> - _mesa_hash_table_destroy(glsl_type::record_types, NULL);
> + _mesa_hash_table_destroy(glsl_type::record_types, hash_free_type_function);
> glsl_type::record_types = NULL;
> }
>
> if (glsl_type::interface_types != NULL) {
> - _mesa_hash_table_destroy(glsl_type::interface_types, NULL);
> + _mesa_hash_table_destroy(glsl_type::interface_types, hash_free_type_function);
> glsl_type::interface_types = NULL;
> }
>
> if (glsl_type::function_types != NULL) {
> - _mesa_hash_table_destroy(glsl_type::function_types, NULL);
> + _mesa_hash_table_destroy(glsl_type::function_types, hash_free_type_function);
> glsl_type::function_types = NULL;
> }
>
> if (glsl_type::subroutine_types != NULL) {
> - _mesa_hash_table_destroy(glsl_type::subroutine_types, NULL);
> + _mesa_hash_table_destroy(glsl_type::subroutine_types, hash_free_type_function);
> glsl_type::subroutine_types = NULL;
> }
> -
> - ralloc_free(glsl_type::mem_ctx);
> - glsl_type::mem_ctx = NULL;
> }
>
>
> @@ -473,9 +463,10 @@ glsl_type::glsl_type(const glsl_type *array, unsigned length) :
> */
> const unsigned name_length = strlen(array->name) + 10 + 3;
>
> - mtx_lock(&glsl_type::mem_mutex);
> + this->mem_ctx = ralloc_context(NULL);
> + assert(this->mem_ctx != NULL);
> +
> char *const n = (char *) ralloc_size(this->mem_ctx, name_length);
> - mtx_unlock(&glsl_type::mem_mutex);
>
> if (length == 0)
> snprintf(n, name_length, "%s[]", array->name);
> @@ -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);
> }
>
> diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
> index ab0b263764..e016835695 100644
> --- a/src/compiler/glsl_types.h
> +++ b/src/compiler/glsl_types.h
> @@ -169,39 +169,6 @@ private:
> }
>
> public:
> - /* Callers of this ralloc-based new need not call delete. It's
> - * easier to just ralloc_free 'mem_ctx' (or any of its ancestors). */
> - static void* operator new(size_t size)
> - {
> - ASSERT_BITFIELD_SIZE(glsl_type, base_type, GLSL_TYPE_ERROR);
> - ASSERT_BITFIELD_SIZE(glsl_type, sampled_type, GLSL_TYPE_ERROR);
> - ASSERT_BITFIELD_SIZE(glsl_type, sampler_dimensionality,
> - GLSL_SAMPLER_DIM_SUBPASS_MS);
These asserts should stick around. They don't actually have anything to
do with memory allocation - it looks like somebody just stuffed them
here arbitrarily. Moving them to some other arbitrary location would
be fine.
Other than that, and the strdup issue Emil noticed,
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
I wanted to try shader-db on ogopogo, just to stress test the
concurrency, but I can't get that working today for some reason.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180228/6c84dda5/attachment.sig>
More information about the mesa-dev
mailing list