On 06.03.2018 14:07, Emil Velikov wrote:
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?

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

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

Reply via email to