libostree: Rework OstreeAsyncProgress to use GVariants internally

OstreeAsyncProgress currently does some contortions to try and avoid
allocating space for guints and guint64s (on 64-bit platforms), but this
means it uses two GHashTables. A GHashTable allocates 8 buckets even
when empty. Given that the largest usage of OstreeAsyncProgress in
libostree puts 13 uints and 5 uint64s in it, this optimisation does not
save significant (if any) memory.

Instead, change OstreeAsyncProgress to store values internally as
GVariants, and expose this with some new API:
 • ostree_async_progress_get_variant()
 • ostree_async_progress_set_variant()
Each GVariant is allocated on the heap. As they are immutable, they are
thread-safe once returned by a getter.

The existing API continues to work as before, except in the case where a
key is set/got as both a uint and a uint64 — there will now be a
collision (and a GVariant type checking failure) whereas previously
there was no collision. Nothing in OSTree uses OstreeAsyncProgress this
way though.

The new API can be used to share more complex data via the progress API.

Signed-off-by: Philip Withnall <[email protected]>

Closes: #819
Approved by: cgwalters

apidoc/ostree-sections.txt

@@ -4,9 +4,11 @@ OstreeAsyncProgress
 ostree_async_progress_new
 ostree_async_progress_new_and_connect
 ostree_async_progress_get_status
+ostree_async_progress_get_variant
 ostree_async_progress_get_uint
 ostree_async_progress_get_uint64
 ostree_async_progress_set_status
+ostree_async_progress_set_variant
 ostree_async_progress_set_uint
 ostree_async_progress_set_uint64
 ostree_async_progress_finish

src/libostree/libostree.sym

@@ -394,13 +394,11 @@ global:
  *                         NOTE NOTE NOTE
  */
 
-/* Uncomment when adding the first new symbol */
-/*
-LIBOSTREE_2017.$NEWVERSION {
+LIBOSTREE_2017.6 {
 global:
-	someostree_symbol_deleteme;
+  ostree_async_progress_get_variant;
+  ostree_async_progress_set_variant;
 } LIBOSTREE_2017.4;
-*/
 
 /* Stub section for the stable release *after* this development one; don't
  * edit this other than to update the last number.  This is just a copy/paste

src/libostree/ostree-async-progress.c

@@ -22,6 +22,8 @@
 
 #include "ostree-async-progress.h"
 
+#include "libglnx.h"
+
 /**
  * SECTION:ostree-async-progress
  * @title: Progress notification system for asynchronous operations
@@ -39,12 +41,6 @@
  * operation.
  */
 
-#if GLIB_SIZEOF_VOID_P == 8
-#define _OSTREE_HAVE_LP64 1
-#else
-#define _OSTREE_HAVE_LP64 0
-#endif
-
 enum {
   CHANGED,
   LAST_SIGNAL
@@ -59,8 +55,7 @@ struct OstreeAsyncProgress
   GMutex lock;
   GMainContext *maincontext;
   GSource *idle_source;
-  GHashTable *uint_values;
-  GHashTable *uint64_values;
+  GHashTable *values;  /* (element-type uint GVariant) */
 
   gboolean dead;
 
@@ -79,8 +74,7 @@ ostree_async_progress_finalize (GObject *object)
   g_mutex_clear (&self->lock);
   g_clear_pointer (&self->maincontext, g_main_context_unref);
   g_clear_pointer (&self->idle_source, g_source_unref);
-  g_hash_table_unref (self->uint_values);
-  g_hash_table_unref (self->uint64_values);
+  g_hash_table_unref (self->values);
   g_free (self->status);
 
   G_OBJECT_CLASS (ostree_async_progress_parent_class)->finalize (object);
@@ -114,46 +108,53 @@ ostree_async_progress_init (OstreeAsyncProgress *self)
 {
   g_mutex_init (&self->lock);
   self->maincontext = g_main_context_ref_thread_default ();
-  self->uint_values = g_hash_table_new (NULL, NULL);
-#if _OSTREE_HAVE_LP64
-  self->uint64_values = g_hash_table_new (NULL, NULL);
-#else
-  self->uint64_values = g_hash_table_new_full (NULL, NULL,
-                                               NULL, g_free);
-#endif
+  self->values = g_hash_table_new_full (NULL, NULL, NULL, (GDestroyNotify) g_variant_unref);
 }
 
-guint
-ostree_async_progress_get_uint (OstreeAsyncProgress       *self,
-                                const char                *key)
+/**
+ * ostree_async_progress_get_variant:
+ * @self: an #OstreeAsyncProgress
+ * @key: a key to look up
+ *
+ * Look up a key in the #OstreeAsyncProgress and return the #GVariant associated
+ * with it. The lookup is thread-safe.
+ *
+ * Returns: (transfer full) (nullable): value for the given @key, or %NULL if
+ *    it was not set
+ * Since: 2017.6
+ */
+GVariant *
+ostree_async_progress_get_variant (OstreeAsyncProgress *self,
+                                   const char          *key)
 {
-  guint rval;
+  GVariant *rval;
+
+  g_return_val_if_fail (OSTREE_IS_ASYNC_PROGRESS (self), NULL);
+  g_return_val_if_fail (key != NULL, NULL);
+
   g_mutex_lock (&self->lock);
-  rval = GPOINTER_TO_UINT (g_hash_table_lookup (self->uint_values,
-                                                GUINT_TO_POINTER (g_quark_from_string (key))));
+  rval = g_hash_table_lookup (self->values, GUINT_TO_POINTER (g_quark_from_string (key)));
+  if (rval != NULL)
+    g_variant_ref (rval);
   g_mutex_unlock (&self->lock);
+
   return rval;
 }
 
+guint
+ostree_async_progress_get_uint (OstreeAsyncProgress       *self,
+                                const char                *key)
+{
+  g_autoptr(GVariant) rval = ostree_async_progress_get_variant (self, key);
+  return (rval != NULL) ? g_variant_get_uint32 (rval) : 0;
+}
+
 guint64
 ostree_async_progress_get_uint64 (OstreeAsyncProgress       *self,
                                   const char                *key)
 {
-#if _OSTREE_HAVE_LP64
-  guint64 rval;
-  g_mutex_lock (&self->lock);
-  rval = (guint64) g_hash_table_lookup (self->uint64_values, GUINT_TO_POINTER (g_quark_from_string (key)));
-  g_mutex_unlock (&self->lock);
-  return rval;
-#else
-  guint64 *rval;
-  g_mutex_lock (&self->lock);
-  rval = g_hash_table_lookup (self->uint64_values, (gpointer)g_quark_from_string (key));
-  g_mutex_unlock (&self->lock);
-  if (rval)
-    return *rval;
-  return 0;
-#endif
+  g_autoptr(GVariant) rval = ostree_async_progress_get_variant (self, key);
+  return (rval != NULL) ? g_variant_get_uint64 (rval) : 0;
 }
 
 static gboolean
@@ -204,26 +205,45 @@ ostree_async_progress_get_status (OstreeAsyncProgress       *self)
   return ret;
 }
 
-static void
-update_key (OstreeAsyncProgress   *self,
-            GHashTable            *hash,
-            const char            *key,
-            gpointer               value)
+/**
+ * ostree_async_progress_set_variant:
+ * @self: an #OstreeAsyncProgress
+ * @key: a key to set
+ * @value: the value to assign to @key
+ *
+ * Assign a new @value to the given @key, replacing any existing value. The
+ * operation is thread-safe. @value may be a floating reference;
+ * g_variant_ref_sink() will be called on it.
+ *
+ * Any watchers of the #OstreeAsyncProgress will be notified of the change if
+ * @value differs from the existing value for @key.
+ *
+ * Since: 2017.6
+ */
+void
+ostree_async_progress_set_variant (OstreeAsyncProgress *self,
+                                   const char          *key,
+                                   GVariant            *value)
 {
-  gpointer orig_value;
+  GVariant *orig_value;
+  g_autoptr(GVariant) new_value = g_variant_ref_sink (value);
   gpointer qkey = GUINT_TO_POINTER (g_quark_from_string (key));
 
+  g_return_if_fail (OSTREE_IS_ASYNC_PROGRESS (self));
+  g_return_if_fail (key != NULL);
+  g_return_if_fail (value != NULL);
+
   g_mutex_lock (&self->lock);
 
   if (self->dead)
     goto out;
 
-  if (g_hash_table_lookup_extended (hash, qkey, NULL, &orig_value))
+  if (g_hash_table_lookup_extended (self->values, qkey, NULL, (gpointer *) &orig_value))
     {
-      if (orig_value == value)
+      if (g_variant_equal (orig_value, new_value))
         goto out;
     }
-  g_hash_table_replace (hash, qkey, value);
+  g_hash_table_replace (self->values, qkey, g_steal_pointer (&new_value));
   ensure_callback_locked (self);
 
  out:
@@ -235,26 +255,15 @@ ostree_async_progress_set_uint (OstreeAsyncProgress       *self,
                                 const char                *key,
                                 guint                      value)
 {
-  update_key (self, self->uint_values, key, GUINT_TO_POINTER (value));
+  ostree_async_progress_set_variant (self, key, g_variant_new_uint32 (value));
 }
 
 void
 ostree_async_progress_set_uint64 (OstreeAsyncProgress       *self,
                                   const char                *key,
                                   guint64                    value)
 {
-  gpointer valuep;
-
-#if _OSTREE_HAVE_LP64
-  valuep = (gpointer)value;
-#else
-  {
-    guint64 *boxed = g_malloc (sizeof (guint64));
-    *boxed = value;
-    valuep = boxed;
-  }
-#endif
-  update_key (self, self->uint64_values, key, valuep);
+  ostree_async_progress_set_variant (self, key, g_variant_new_uint64 (value));
 }
 
 /**

src/libostree/ostree-async-progress.h

@@ -59,6 +59,9 @@ guint ostree_async_progress_get_uint (OstreeAsyncProgress       *self,
 _OSTREE_PUBLIC
 guint64 ostree_async_progress_get_uint64 (OstreeAsyncProgress       *self,
                                           const char                *key);
+_OSTREE_PUBLIC
+GVariant *ostree_async_progress_get_variant (OstreeAsyncProgress *self,
+                                             const char          *key);
 
 _OSTREE_PUBLIC
 void ostree_async_progress_set_status (OstreeAsyncProgress       *self,
@@ -72,6 +75,10 @@ _OSTREE_PUBLIC
 void ostree_async_progress_set_uint64 (OstreeAsyncProgress       *self,
                                        const char                *key,
                                        guint64                    value);
+_OSTREE_PUBLIC
+void ostree_async_progress_set_variant (OstreeAsyncProgress *self,
+                                        const char          *key,
+                                        GVariant            *value);
 
 _OSTREE_PUBLIC
 void ostree_async_progress_finish (OstreeAsyncProgress *self);