[glib/glib-2-28] GDBusWorker: combine num_writes_pending with flush_pending

[David Zeuthen <davidz src gnome org>]

commit 7bbc7727d4d14c9a7ab1cc760c7c01425ca2585e
Author: Simon McVittie <simon mcvittie collabora co uk>
Date:   Tue Sep 13 16:30:33 2011 +0100

    GDBusWorker: combine num_writes_pending with flush_pending
    
    num_writes_pending was a counter, but it only took values 0 or 1, so make
    it a boolean: it would never make sense to be trying to write out two
    messages at the same time (they'd get interleaved).
    
    Similarly, we can never be writing and flushing at the same time (that'd
    mean we were flushing halfway through a message, which would be pointless)
    so combine it with flush_pending too, calling the result output_pending.
    
    Also assert that it takes the expected value whenever we change it,
    and document the locking discipline used for it, including a subtle
    case in write_message_in_idle_cb where it's not obvious at first glance
    why we don't need the lock.
    
    (Having the combined boolean at the top of the block of write-related
    struct members improves struct packing on 64-bit platforms, by packing
    read_num_ancillary_messages and output_pending into one word.)
    
    Bug: https://bugzilla.gnome.org/show_bug.cgi?id=651268
    Bug-NB: NB#271520
    Signed-off-by: Simon McVittie <simon mcvittie collabora co uk>
    Signed-off-by: David Zeuthen <davidz redhat com>

 gio/gdbusprivate.c |   36 +++++++++++++++++++++++++-----------
 1 files changed, 25 insertions(+), 11 deletions(-)
---
diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c
index 5f8f1f4..ace0800 100644
--- a/gio/gdbusprivate.c
+++ b/gio/gdbusprivate.c
@@ -424,13 +424,17 @@ struct GDBusWorker
   GSocketControlMessage             **read_ancillary_messages;
   gint                                read_num_ancillary_messages;
 
+  /* TRUE if an async write or flush is pending.
+   * Only the worker thread may change its value, and only with the write_lock.
+   * Other threads may read its value when holding the write_lock.
+   * The worker thread may read its value at any time.
+   */
+  gboolean                            output_pending;
   /* used for writing */
   GMutex                             *write_lock;
   GQueue                             *write_queue;
-  gint                                num_writes_pending;
   guint64                             write_num_messages_written;
   GList                              *write_pending_flushes;
-  gboolean                            flush_pending;
 };
 
 /* ---------------------------------------------------------------------------------------------------- */
@@ -1159,7 +1163,8 @@ ostream_flush_cb (GObject      *source_object,
   /* Make sure we tell folks that we don't have additional
      flushes pending */
   g_mutex_lock (data->worker->write_lock);
-  data->worker->flush_pending = FALSE;
+  g_assert (data->worker->output_pending);
+  data->worker->output_pending = FALSE;
   g_mutex_unlock (data->worker->write_lock);
 
   /* OK, cool, finally kick off the next write */
@@ -1216,7 +1221,8 @@ message_written (GDBusWorker *worker,
     }
   if (flushers != NULL)
     {
-      worker->flush_pending = TRUE;
+      g_assert (!worker->output_pending);
+      worker->output_pending = TRUE;
     }
   g_mutex_unlock (worker->write_lock);
 
@@ -1250,7 +1256,8 @@ write_message_cb (GObject       *source_object,
   GError *error;
 
   g_mutex_lock (data->worker->write_lock);
-  data->worker->num_writes_pending -= 1;
+  g_assert (data->worker->output_pending);
+  data->worker->output_pending = FALSE;
   g_mutex_unlock (data->worker->write_lock);
 
   error = NULL;
@@ -1277,15 +1284,17 @@ maybe_write_next_message (GDBusWorker *worker)
   MessageToWriteData *data;
 
  write_next:
+  /* we mustn't try to write two things at once */
+  g_assert (!worker->output_pending);
 
   g_mutex_lock (worker->write_lock);
   data = g_queue_pop_head (worker->write_queue);
   if (data != NULL)
-    worker->num_writes_pending += 1;
+    worker->output_pending = TRUE;
   g_mutex_unlock (worker->write_lock);
 
   /* Note that write_lock is only used for protecting the @write_queue
-   * and @num_writes_pending fields of the GDBusWorker struct ... which we
+   * and @output_pending fields of the GDBusWorker struct ... which we
    * need to modify from arbitrary threads in _g_dbus_worker_send_message().
    *
    * Therefore, it's fine to drop it here when calling back into user
@@ -1309,7 +1318,7 @@ maybe_write_next_message (GDBusWorker *worker)
         {
           /* filters dropped message */
           g_mutex_lock (worker->write_lock);
-          worker->num_writes_pending -= 1;
+          worker->output_pending = FALSE;
           g_mutex_unlock (worker->write_lock);
           message_to_write_data_free (data);
           goto write_next;
@@ -1352,8 +1361,13 @@ static gboolean
 write_message_in_idle_cb (gpointer user_data)
 {
   GDBusWorker *worker = user_data;
-  if (worker->num_writes_pending == 0 && !worker->flush_pending)
+
+  /* Because this is the worker thread, we can read this struct member
+   * without holding the lock: no other thread ever modifies it.
+   */
+  if (!worker->output_pending)
     maybe_write_next_message (worker);
+
   return FALSE;
 }
 
@@ -1380,7 +1394,7 @@ _g_dbus_worker_send_message (GDBusWorker    *worker,
 
   g_mutex_lock (worker->write_lock);
   g_queue_push_tail (worker->write_queue, data);
-  if (worker->num_writes_pending == 0)
+  if (!worker->output_pending)
     {
       GSource *idle_source;
       idle_source = g_idle_source_new ();
@@ -1435,7 +1449,7 @@ _g_dbus_worker_new (GIOStream                              *stream,
   worker->stream = g_object_ref (stream);
   worker->capabilities = capabilities;
   worker->cancellable = g_cancellable_new ();
-  worker->flush_pending = FALSE;
+  worker->output_pending = FALSE;
 
   worker->frozen = initially_frozen;
   worker->received_messages_while_frozen = g_queue_new ();