[dconf: 2/4] engine: Limit the number of in-flight requests to one

[Daniel Playfair Cal <danielplayfaircal src gnome org>]

commit 62033732542d7effaf8b40fbe9df79f69a3e59d3
Author: Tomasz Miąsko <tomasz miasko gmail com>
Date:   Sun Nov 11 17:14:30 2018 +0100

    engine: Limit the number of in-flight requests to one
    
    Reduce the number of in-flight requests to one, so as to increase
    chances of merging pending requests. Drop the in-flight queue since
    it is no longer useful, replacing it with optional changeset.

 engine/dconf-engine.c | 54 ++++++++++++---------------------------------------
 tests/client.c        | 35 +++++++++++++--------------------
 2 files changed, 25 insertions(+), 64 deletions(-)
---
diff --git a/engine/dconf-engine.c b/engine/dconf-engine.c
index bb0cf9f..c0ad181 100644
--- a/engine/dconf-engine.c
+++ b/engine/dconf-engine.c
@@ -154,8 +154,6 @@
  * sources_lock or queue_lock
  */
 
-#define MAX_IN_FLIGHT 2
-
 static GSList *dconf_engine_global_list;
 static GMutex  dconf_engine_global_lock;
 
@@ -173,7 +171,7 @@ struct _DConfEngine
   GMutex              queue_lock;    /* This lock is for pending, in_flight, queue_cond */
   GCond               queue_cond;    /* Signalled when the queues empty */
   DConfChangeset     *pending;       /* DConfChangeset */
-  GQueue              in_flight;     /* DConfChangeset */
+  DConfChangeset     *in_flight;     /* DConfChangeset */
 
   gchar              *last_handled;  /* reply tag from last item in in_flight */
 
@@ -403,9 +401,7 @@ dconf_engine_unref (DConfEngine *engine)
       g_free (engine->last_handled);
 
       g_clear_pointer (&engine->pending, dconf_changeset_unref);
-
-      while (!g_queue_is_empty (&engine->in_flight))
-        dconf_changeset_unref ((DConfChangeset *) g_queue_pop_head (&engine->in_flight));
+      g_clear_pointer (&engine->in_flight, dconf_changeset_unref);
 
       for (i = 0; i < engine->n_sources; i++)
         dconf_engine_source_free (engine->sources[i]);
@@ -737,8 +733,8 @@ dconf_engine_read (DConfEngine    *engine,
           if (engine->pending != NULL)
             found_key = dconf_changeset_get (engine->pending, key, &value);
 
-          if (!found_key)
-            found_key = dconf_engine_find_key_in_queue (&engine->in_flight, key, &value);
+          if (!found_key && engine->in_flight != NULL)
+            found_key = dconf_changeset_get (engine->in_flight, key, &value);
 
           dconf_engine_unlock_queues (engine);
         }
@@ -1174,36 +1170,12 @@ dconf_engine_change_completed (DConfEngine  *engine,
                                const GError *error)
 {
   OutstandingChange *oc = handle;
+  DConfChangeset *expected;
 
   dconf_engine_lock_queues (engine);
 
-  /* D-Bus guarantees ordered delivery of messages.
-   *
-   * The dconf-service handles requests in-order.
-   *
-   * The reply we just received should therefore be at the head of
-   * our 'in flight' queue.
-   *
-   * Due to https://bugs.freedesktop.org/show_bug.cgi?id=59780 it is
-   * possible that we receive an out-of-sequence error message, however,
-   * so only assume that messages are in-order for positive replies.
-   */
-  if (reply)
-    {
-      DConfChangeset *expected;
-
-      expected = g_queue_pop_head (&engine->in_flight);
-      g_assert (expected && oc->change == expected);
-    }
-  else
-    {
-      gboolean found;
-
-      g_assert (error != NULL);
-
-      found = g_queue_remove (&engine->in_flight, oc->change);
-      g_assert (found);
-    }
+  expected = g_steal_pointer (&engine->in_flight);
+  g_assert (expected && oc->change == expected);
 
   /* We just popped a change from the in-flight queue, possibly
    * making room for another to be added.  Check that.
@@ -1251,7 +1223,7 @@ dconf_engine_change_completed (DConfEngine  *engine,
 static void
 dconf_engine_manage_queue (DConfEngine *engine)
 {
-  if (engine->pending != NULL && g_queue_get_length (&engine->in_flight) < MAX_IN_FLIGHT)
+  if (engine->pending != NULL && engine->in_flight == NULL)
     {
       OutstandingChange *oc;
       GVariant *parameters;
@@ -1259,7 +1231,7 @@ dconf_engine_manage_queue (DConfEngine *engine)
       oc = dconf_engine_call_handle_new (engine, dconf_engine_change_completed,
                                          G_VARIANT_TYPE ("(s)"), sizeof (OutstandingChange));
 
-      oc->change = g_steal_pointer (&engine->pending);
+      oc->change = engine->in_flight = g_steal_pointer (&engine->pending);
 
       parameters = dconf_engine_prepare_change (engine, oc->change);
 
@@ -1268,11 +1240,9 @@ dconf_engine_manage_queue (DConfEngine *engine)
                                          engine->sources[0]->object_path,
                                          "ca.desrt.dconf.Writer", "Change",
                                          parameters, &oc->handle, NULL);
-
-      g_queue_push_tail (&engine->in_flight, oc->change);
     }
 
-  if (g_queue_is_empty (&engine->in_flight))
+  if (engine->in_flight == NULL)
     {
       /* The in-flight queue should not be empty if we have changes
        * pending...
@@ -1543,7 +1513,7 @@ dconf_engine_has_outstanding (DConfEngine *engine)
    * also empty, so we only really need to check one of them...
    */
   dconf_engine_lock_queues (engine);
-  has = !g_queue_is_empty (&engine->in_flight);
+  has = engine->in_flight != NULL;
   dconf_engine_unlock_queues (engine);
 
   return has;
@@ -1554,7 +1524,7 @@ dconf_engine_sync (DConfEngine *engine)
 {
   g_debug ("sync");
   dconf_engine_lock_queues (engine);
-  while (!g_queue_is_empty (&engine->in_flight))
+  while (engine->in_flight != NULL)
     g_cond_wait (&engine->queue_cond, &engine->queue_lock);
   dconf_engine_unlock_queues (engine);
 }
diff --git a/tests/client.c b/tests/client.c
index 25f99f3..1773ed1 100644
--- a/tests/client.c
+++ b/tests/client.c
@@ -58,7 +58,7 @@ queue_up_100_writes (DConfClient *client)
   gint i;
 
   /* We send 100 writes, letting them pile up.
-   * At no time should there be more than 2 writes on the wire.
+   * At no time should there be more than one write on the wire.
    */
   for (i = 0; i < 100; i++)
     {
@@ -71,7 +71,7 @@ queue_up_100_writes (DConfClient *client)
       check_and_free (dconf_client_read_full (client, "/test/value", DCONF_READ_DEFAULT_VALUE, NULL), NULL);
     }
 
-  g_assert_cmpint (g_queue_get_length (&dconf_mock_dbus_outstanding_call_handles), ==, 2);
+  g_assert_cmpint (g_queue_get_length (&dconf_mock_dbus_outstanding_call_handles), ==, 1);
 }
 
 static void
@@ -108,7 +108,6 @@ static void
 test_fast (void)
 {
   DConfClient *client;
-  gint i;
 
   g_log_set_writer_func (log_writer_cb, NULL, NULL);
 
@@ -119,30 +118,23 @@ test_fast (void)
 
   /* Start indicating that the writes failed.
    *
-   * For the first failures, we should continue to see the most recently
-   * written value (99).
-   *
-   * After we fail that last one, we should see NULL returned.
+   * Because of the pending-merge logic, we should only have had to fail two calls.
    *
    * Each time, we should see a change notify.
    */
 
-  for (i = 0; g_queue_get_length (&dconf_mock_dbus_outstanding_call_handles) > 1; i++)
-    {
-      changed_was_called = FALSE;
-      fail_one_call ();
-      g_assert (changed_was_called);
+  g_assert_cmpint (g_queue_get_length (&dconf_mock_dbus_outstanding_call_handles), == , 1);
 
-      check_and_free (dconf_client_read (client, "/test/value"), g_variant_new_int32 (99));
-      check_and_free (dconf_client_read_full (client, "/test/value", DCONF_READ_DEFAULT_VALUE, NULL), NULL);
-    }
+  changed_was_called = FALSE;
+  fail_one_call ();
+  g_assert (changed_was_called);
 
-  /* Because of the pending-merging logic, we should only have had to
-   * fail two calls.
-   */
-  g_assert (i == 2);
+  /* For the first failure, we should continue to see the most recently written value (99) */
+  check_and_free (dconf_client_read (client, "/test/value"), g_variant_new_int32 (99));
+  check_and_free (dconf_client_read_full (client, "/test/value", DCONF_READ_DEFAULT_VALUE, NULL), NULL);
+
+  g_assert_cmpint (g_queue_get_length (&dconf_mock_dbus_outstanding_call_handles), == , 1);
 
-  /* Fail the last call. */
   changed_was_called = FALSE;
   fail_one_call ();
   g_assert (changed_was_called);
@@ -228,9 +220,8 @@ test_coalesce (void)
 
   dconf_mock_dbus_async_reply (g_variant_new ("(s)", "1"), NULL);
   dconf_mock_dbus_async_reply (g_variant_new ("(s)", "2"), NULL);
-  dconf_mock_dbus_async_reply (g_variant_new ("(s)", "3"), NULL);
 
-  /* There should be no more requests since all but first two have been
+  /* There should be no more requests since all but first have been
    * coalesced together. */
   dconf_mock_dbus_assert_no_async ();