Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances

Tapani Pälli Mon, 05 Mar 2018 23:33:14 -0800

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 themin every constructor (?)

Other than that, and the strdup issue Emil noticed,


That was no-issue, see my reply.

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.


// Tapani
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glsl: Fix memory leak with known glsl_type instances

Reply via email to