On Thursday, February 15, 2018 1:12:54 AM PST Tapani Pälli wrote: > From: Simon Hausmann <simon.hausm...@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.hausm...@qt.io> > Signed-off-by: Tapani Pälli <tapani.pa...@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 <kenn...@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.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
