Attachment 137353 Details for Bug 104884 – Simple patch that changes glsl_type to manage its own memory

[patch] Simple patch that changes glsl_type to manage its own memory

0001-glsl-Fix-memory-leak-with-known-glsl_type-instances.patch (text/plain), 10.02 KB, created by Simon Hausmann on 2018-02-14 12:01:11 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Hausmann

Created: 2018-02-14 12:01:11 UTC

Size: 10.02 KB

patch

obsolete

>From 31fb2b846b1d0dfde79dfb81734f71ce0d014a47 Mon Sep 17 00:00:00 2001
>From: Simon Hausmann <simon.hausmann@qt.io>
>Date: Wed, 14 Feb 2018 12:51:11 +0100
>Subject: [PATCH] glsl: Fix memory leak with known glsl_type instances
>
>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.
>
>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104884
>Signed-off-by: Simon Hausmann <simon.hausmann@qt.io>
>---
> src/compiler/glsl_types.cpp | 79 +++++++++++++++++++--------------------------
> src/compiler/glsl_types.h   | 44 +++----------------------
> 2 files changed, 37 insertions(+), 86 deletions(-)
>
>diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp
>index 3cc5eb0495..2c35b42e54 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
>@@ -419,37 +401,41 @@ const glsl_type *glsl_type::get_scalar_type() const
> void
> _mesa_glsl_release_types(void)
> {
>+    const auto hash_free_type_function = [](struct hash_entry *entry) {
>+        delete (glsl_type *)entry->data;
>+    };
>+
>    /* Should only be called during atexit (either when unloading shared
>     * object, or if process terminates), so no mutex-locking should be
>     * necessary.
>     */
>    if (glsl_type::array_types != NULL) {
>-      _mesa_hash_table_destroy(glsl_type::array_types, NULL);
>+      _mesa_hash_table_destroy(glsl_type::array_types, [](struct hash_entry *entry){
>+          ::free((void *)entry->key);
>+          delete (glsl_type *)entry->data;
>+      });
>       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 +459,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 +957,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..90a4654790 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);
>-
>-      mtx_lock(&glsl_type::mem_mutex);
>-
>-      /* mem_ctx should have been created by the static members */
>-      assert(glsl_type::mem_ctx != NULL);
>-
>-      void *type;
>-
>-      type = ralloc_size(glsl_type::mem_ctx, size);
>-      assert(type != NULL);
>-
>-      mtx_unlock(&glsl_type::mem_mutex);
>-
>-      return type;
>-   }
>-
>-   /* If the user *does* call delete, that's OK, we will just
>-    * ralloc_free in that case. */
>-   static void operator delete(void *type)
>-   {
>-      mtx_lock(&glsl_type::mem_mutex);
>-      ralloc_free(type);
>-      mtx_unlock(&glsl_type::mem_mutex);
>-   }
>-
>    /**
>     * \name Vector and matrix element counts
>     *
>@@ -875,17 +842,12 @@ public:
> 
> private:
> 
>-   static mtx_t mem_mutex;
>    static mtx_t hash_mutex;
> 
>    /**
>-    * ralloc context for all glsl_type allocations
>-    *
>-    * Set on the first call to \c glsl_type::new.
>+    * ralloc context for the type itself.
>     */
>-   static void *mem_ctx;
>-
>-   void init_ralloc_type_ctx(void);
>+   void *mem_ctx;
> 
>    /** Constructor for vector and matrix types */
>    glsl_type(GLenum gl_type,
>@@ -916,6 +878,8 @@ private:
>    /** Constructor for subroutine types */
>    glsl_type(const char *name);
> 
>+   ~glsl_type();
>+
>    /** Hash table containing the known array types. */
>    static struct hash_table *array_types;
> 
>-- 
>2.14.1
>

Actions: View | Splinter Review

Attachments on bug 104884: 137292 | 137294 | 137353 | 137522 | 137720 | 137858

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.