[glib/th/gobject-new-parameter-list] gobject: limit required stack by heap allocating parameter list in g_object_new_valist()




commit fb3977e9b4fb0b731d01bb4cff99a8f6be6e75c8
Author: Thomas Haller <thaller redhat com>
Date:   Thu Dec 12 12:28:01 2019 +0100

    gobject: limit required stack by heap allocating parameter list in g_object_new_valist()
    
    The previous code consumed a larger additional amount of stack space.
    That is because it would allocate the temporary buffer for GValues on
    the stack with "g_newa (GValue, 1)" and thus the required stack
    space grew with the number of arguments. Granted, this is already
    a variadic C function, so the caller already placed that many elements
    on the stack. For example, on the stack there are the property names
    and the pointers to the arguments, which should amount to roughly
    O(n_args * 16) (on 64 bit, with pointers being 8 bytes large).
    
    That is not bad, because it means in the previous version the stack space
    would grow linear with the already used stack space. However, a GValue is
    an additional 24 bytes (on 64 bit), which probably more than doubles the
    required stack space. Let's avoid that, by allocating the temporary list
    on the heap after a certain threshold. This probably more than doubles the
    number of possible arguments before the stack overflows.
    
    Also, previously the heap allocated "params" array only grew one element
    per iteration. Of course, it is likely that libc anyway reallocates
    the buffers by growing the space exponentially. So realloc(ptr, 1)
    probably does not O() scale worse than doubling the buffer sizes ourselves.
    However, it seems clearer to keep track of the allocated sizes ourself, and
    only call realloc() when we determine that we are out of space.
    Especially because we need to update the value pointers on reallocation.
    
    Note that we now require a heap allocation both for the "params" and the
    "values" list. Theoretically that could be combined by using one buffer
    for both. But that would make the code more complicated.
    
    Now we pre-allocate buffers for 16 elements on the stack. That
    is (16 * (16 + 24) bytes (or 640 bytes) on the stack. I think that
    is still acceptable.

 gobject/gobject.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)
---
diff --git a/gobject/gobject.c b/gobject/gobject.c
index 3e860da26..32d554874 100644
--- a/gobject/gobject.c
+++ b/gobject/gobject.c
@@ -2218,13 +2218,15 @@ g_object_new_valist (GType        object_type,
 
   if (first_property_name)
     {
-      GObjectConstructParam stack_params[16];
-      GObjectConstructParam *params;
+      GObjectConstructParam params_stack[16];
+      GValue values_stack[G_N_ELEMENTS (params_stack)];
       const gchar *name;
-      gint n_params = 0;
+      GObjectConstructParam *params = params_stack;
+      GValue *values = values_stack;
+      guint n_params = 0;
+      guint n_params_alloc = G_N_ELEMENTS (params_stack);
 
       name = first_property_name;
-      params = stack_params;
 
       do
         {
@@ -2236,24 +2238,39 @@ g_object_new_valist (GType        object_type,
           if (!g_object_new_is_valid_property (object_type, pspec, name, params, n_params))
             break;
 
-          if (n_params == 16)
+          if (G_UNLIKELY (n_params == n_params_alloc))
             {
-              params = g_new (GObjectConstructParam, n_params + 1);
-              memcpy (params, stack_params, sizeof stack_params);
+              guint i;
+
+              if (n_params_alloc == G_N_ELEMENTS (params_stack))
+                {
+                  n_params_alloc = G_N_ELEMENTS (params_stack) * 2u;
+                  params = g_new (GObjectConstructParam, n_params_alloc);
+                  values = g_new (GValue, n_params_alloc);
+                  memcpy (params, params_stack, sizeof (GObjectConstructParam) * n_params);
+                  memcpy (values, values_stack, sizeof (GValue) * n_params);
+                }
+              else
+                {
+                  n_params_alloc *= 2u;
+                  params = g_realloc (params, sizeof (GObjectConstructParam) * n_params_alloc);
+                  values = g_realloc (values, sizeof (GValue) * n_params_alloc);
+                }
+
+              for (i = 0; i < n_params; i++)
+                params[i].value = &values[i];
             }
-          else if (n_params > 16)
-            params = g_renew (GObjectConstructParam, params, n_params + 1);
 
           params[n_params].pspec = pspec;
-          params[n_params].value = g_newa (GValue, 1);
-          memset (params[n_params].value, 0, sizeof (GValue));
+          params[n_params].value = &values[n_params];
+          memset (&values[n_params], 0, sizeof (GValue));
 
-          G_VALUE_COLLECT_INIT (params[n_params].value, pspec->value_type, var_args, 0, &error);
+          G_VALUE_COLLECT_INIT (&values[n_params], pspec->value_type, var_args, 0, &error);
 
           if (error)
             {
               g_critical ("%s: %s", G_STRFUNC, error);
-              g_value_unset (params[n_params].value);
+              g_value_unset (&values[n_params]);
               g_free (error);
               break;
             }
@@ -2267,8 +2284,11 @@ g_object_new_valist (GType        object_type,
       while (n_params--)
         g_value_unset (params[n_params].value);
 
-      if (params != stack_params)
-        g_free (params);
+      if (G_UNLIKELY (n_params_alloc != G_N_ELEMENTS (params_stack)))
+        {
+          g_free (params);
+          g_free (values);
+        }
     }
   else
     /* Fast case: no properties passed in. */


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]