On 6 March 2018 at 07:32, Tapani Pälli <tapani.pa...@intel.com> 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 <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. > > > Any suggestions for this arbitrary location? It seems to me that new() was > not so arbitrary, it was kind of convinient, now we should have them in > every constructor (?) > Of the of the glsl_type ctors already has a bunch of STATIC_ASSERTs. I'd stick it in there?
>> Other than that, and the strdup issue Emil noticed, > > > That was no-issue, see my reply. > Indeed - pardon for the noise. -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev