[Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
Emil Velikov
emil.l.velikov at gmail.com
Tue Mar 6 12:07:33 UTC 2018
On 6 March 2018 at 07:32, Tapani Pälli <tapani.palli at 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.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 (?)
>
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
More information about the mesa-dev
mailing list