[Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances
Tapani Pälli
tapani.palli at intel.com
Tue Mar 6 13:23:05 UTC 2018
On 06.03.2018 14:07, Emil Velikov wrote:
> 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?
Yeah this is fine for me. I noticed that when adding
ASSERT_BITFIELD_SIZE back I need to also initialize mem_ctx as NULL
because ASSERT_BITFIELD_SIZE uses the 'dummy ctor' in header and our
dtor now calls ralloc_free. If this addition is fine, I'll run CI one
more time and push it in like that.
>>> 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