[Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
Tapani Pälli
tapani.palli at intel.com
Tue Mar 6 07:32:55 UTC 2018
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.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.
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 (?)
> Other than that, and the strdup issue Emil noticed,
That was no-issue, see my reply.
> 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.
>
// Tapani
More information about the mesa-dev
mailing list