gmain: don't pass the same fd to g_poll() multiple times
authorDan Winship <danw@gnome.org>
Tue, 13 Mar 2012 15:43:38 +0000 (11:43 -0400)
committerDan Winship <danw@gnome.org>
Wed, 29 Oct 2014 21:19:20 +0000 (17:19 -0400)
If a given fd is being polled by multiple sources, we used to pass it
multiple times to g_poll(), which is technically illegal (and not
supported by the select()-based fallback implementation of poll() in
gpoll.c), and also made it more likely that we'd exceed the maximum
number of pollfds.

Fix it to merge together "duplicate" GPollFDs. The easiest way to do
this involves re-sorting context->poll_records into fd order rather
than priority order. This means we now have to walk the entire pollrec
list for every g_main_context_query() and g_main_context_poll(),
rather than only walking the list up to the current max_priority.
However, this will only have a noticeable effect if you have tons of
GPollFDs, and we're already too slow in that case anyway because of
other O(n) operations that happen too often. So this shouldn't change
much (and the new poll API will eventually let us be cleverer).

Remove some win32-specific code which did the same thing (but was
O(n^2)).

https://bugzilla.gnome.org/show_bug.cgi?id=11059

glib/gmain.c
glib/gpoll.c
glib/tests/mainloop.c

index 47e7749..e2b6ca3 100644 (file)
@@ -249,7 +249,7 @@ struct _GMainContext
   GList *source_lists;
   gint in_check_or_prepare;
 
-  GPollRec *poll_records, *poll_records_tail;
+  GPollRec *poll_records;
   guint n_poll_records;
   GPollFD *cached_poll_array;
   guint cached_poll_array_size;
@@ -3506,33 +3506,43 @@ g_main_context_query (GMainContext *context,
                      gint          n_fds)
 {
   gint n_poll;
-  GPollRec *pollrec;
+  GPollRec *pollrec, *lastpollrec;
+  gushort events;
   
   LOCK_CONTEXT (context);
 
-  pollrec = context->poll_records;
   n_poll = 0;
-  while (pollrec && max_priority >= pollrec->priority)
+  lastpollrec = NULL;
+  for (pollrec = context->poll_records; pollrec; pollrec = pollrec->next)
     {
-      /* We need to include entries with fd->events == 0 in the array because
-       * otherwise if the application changes fd->events behind our back and 
-       * makes it non-zero, we'll be out of sync when we check the fds[] array.
-       * (Changing fd->events after adding an FD wasn't an anticipated use of 
-       * this API, but it occurs in practice.) */
-      if (n_poll < n_fds)
-       {
-         fds[n_poll].fd = pollrec->fd->fd;
-         /* In direct contradiction to the Unix98 spec, IRIX runs into
-          * difficulty if you pass in POLLERR, POLLHUP or POLLNVAL
-          * flags in the events field of the pollfd while it should
-          * just ignoring them. So we mask them out here.
-          */
-         fds[n_poll].events = pollrec->fd->events & ~(G_IO_ERR|G_IO_HUP|G_IO_NVAL);
-         fds[n_poll].revents = 0;
-       }
+      if (pollrec->priority > max_priority)
+        continue;
+
+      /* In direct contradiction to the Unix98 spec, IRIX runs into
+       * difficulty if you pass in POLLERR, POLLHUP or POLLNVAL
+       * flags in the events field of the pollfd while it should
+       * just ignoring them. So we mask them out here.
+       */
+      events = pollrec->fd->events & ~(G_IO_ERR|G_IO_HUP|G_IO_NVAL);
+
+      if (lastpollrec && pollrec->fd->fd == lastpollrec->fd->fd)
+        {
+          if (n_poll - 1 < n_fds)
+            fds[n_poll - 1].events |= events;
+        }
+      else
+        {
+          if (n_poll < n_fds)
+            {
+              fds[n_poll].fd = pollrec->fd->fd;
+              fds[n_poll].events = events;
+              fds[n_poll].revents = 0;
+            }
+
+          n_poll++;
+        }
 
-      pollrec = pollrec->next;
-      n_poll++;
+      lastpollrec = pollrec;
     }
 
   context->poll_changed = FALSE;
@@ -3597,15 +3607,21 @@ g_main_context_check (GMainContext *context,
       UNLOCK_CONTEXT (context);
       return FALSE;
     }
-  
+
   pollrec = context->poll_records;
   i = 0;
-  while (i < n_fds)
+  while (pollrec && i < n_fds)
     {
-      if (pollrec->fd->events)
-       pollrec->fd->revents = fds[i].revents;
+      while (pollrec && pollrec->fd->fd == fds[i].fd)
+        {
+          if (pollrec->priority <= max_priority)
+            {
+              pollrec->fd->revents =
+                fds[i].revents & (pollrec->fd->events | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
+            }
+          pollrec = pollrec->next;
+        }
 
-      pollrec = pollrec->next;
       i++;
     }
 
@@ -4185,12 +4201,14 @@ g_main_context_add_poll_unlocked (GMainContext *context,
   newrec->fd = fd;
   newrec->priority = priority;
 
-  prevrec = context->poll_records_tail;
-  nextrec = NULL;
-  while (prevrec && priority < prevrec->priority)
+  prevrec = NULL;
+  nextrec = context->poll_records;
+  while (nextrec)
     {
-      nextrec = prevrec;
-      prevrec = prevrec->prev;
+      if (nextrec->fd > fd)
+        break;
+      prevrec = nextrec;
+      nextrec = nextrec->next;
     }
 
   if (prevrec)
@@ -4203,8 +4221,6 @@ g_main_context_add_poll_unlocked (GMainContext *context,
 
   if (nextrec)
     nextrec->prev = newrec;
-  else 
-    context->poll_records_tail = newrec;
 
   context->n_poll_records++;
 
@@ -4258,8 +4274,6 @@ g_main_context_remove_poll_unlocked (GMainContext *context,
 
          if (nextrec != NULL)
            nextrec->prev = prevrec;
-         else
-           context->poll_records_tail = prevrec;
 
          g_slice_free (GPollRec, pollrec);
 
index 2620c9a..adc6782 100644 (file)
@@ -271,30 +271,17 @@ g_poll (GPollFD *fds,
       }
     else if (f->fd > 0)
       {
-       /* Don't add the same handle several times into the array, as
-        * docs say that is not allowed, even if it actually does seem
-        * to work.
-        */
-       gint i;
-
-       for (i = 0; i < nhandles; i++)
-         if (handles[i] == (HANDLE) f->fd)
-           break;
-
-       if (i == nhandles)
-         {
-           if (nhandles == MAXIMUM_WAIT_OBJECTS)
-             {
-               g_warning ("Too many handles to wait for!\n");
-               break;
-             }
-           else
-             {
-               if (_g_main_poll_debug)
-                 g_print (" %p", (HANDLE) f->fd);
-               handles[nhandles++] = (HANDLE) f->fd;
-             }
-         }
+        if (nhandles == MAXIMUM_WAIT_OBJECTS)
+          {
+            g_warning ("Too many handles to wait for!\n");
+            break;
+          }
+        else
+          {
+            if (_g_main_poll_debug)
+              g_print (" %p", (HANDLE) f->fd);
+            handles[nhandles++] = (HANDLE) f->fd;
+          }
       }
 
   if (_g_main_poll_debug)
index 8abd262..f5d672a 100644 (file)
@@ -22,6 +22,7 @@
 
 #include <glib.h>
 #include "glib-private.h"
+#include <stdio.h>
 #include <string.h>
 
 static gboolean cb (gpointer data)
@@ -1564,6 +1565,165 @@ test_mainloop_wait (void)
   g_main_context_unref (context);
 }
 
+static gboolean
+nfds_in_cb (GIOChannel   *io,
+            GIOCondition  condition,
+            gpointer      user_data)
+{
+  gboolean *in_cb_ran = user_data;
+
+  *in_cb_ran = TRUE;
+  g_assert_cmpint (condition, ==, G_IO_IN);
+  return FALSE;
+}
+
+static gboolean
+nfds_out_cb (GIOChannel   *io,
+             GIOCondition  condition,
+             gpointer      user_data)
+{
+  gboolean *out_cb_ran = user_data;
+
+  *out_cb_ran = TRUE;
+  g_assert_cmpint (condition, ==, G_IO_OUT);
+  return FALSE;
+}
+
+static gboolean
+nfds_out_low_cb (GIOChannel   *io,
+                 GIOCondition  condition,
+                 gpointer      user_data)
+{
+  g_assert_not_reached ();
+  return FALSE;
+}
+
+static void
+test_nfds (void)
+{
+  GMainContext *ctx;
+  GPollFD out_fds[3];
+  gint fd, nfds;
+  GIOChannel *io;
+  GSource *source1, *source2, *source3;
+  gboolean source1_ran = FALSE, source3_ran = FALSE;
+  gchar *tmpfile;
+  GError *error = NULL;
+
+  ctx = g_main_context_new ();
+  nfds = g_main_context_query (ctx, G_MAXINT, NULL,
+                               out_fds, G_N_ELEMENTS (out_fds));
+  /* An "empty" GMainContext will have a single GPollFD, for its
+   * internal GWakeup.
+   */
+  g_assert_cmpint (nfds, ==, 1);
+
+  fd = g_file_open_tmp (NULL, &tmpfile, &error);
+  g_assert_no_error (error);
+
+  io = g_io_channel_unix_new (fd);
+#ifdef G_OS_WIN32
+  /* The fd in the pollfds won't be the same fd we passed in */
+  g_io_channel_win32_make_pollfd (io, G_IO_IN, out_fds);
+  fd = out_fds[0].fd;
+#endif
+
+  /* Add our first pollfd */
+  source1 = g_io_create_watch (io, G_IO_IN);
+  g_source_set_priority (source1, G_PRIORITY_DEFAULT);
+  g_source_set_callback (source1, (GSourceFunc) nfds_in_cb,
+                         &source1_ran, NULL);
+  g_source_attach (source1, ctx);
+
+  nfds = g_main_context_query (ctx, G_MAXINT, NULL,
+                               out_fds, G_N_ELEMENTS (out_fds));
+  g_assert_cmpint (nfds, ==, 2);
+  if (out_fds[0].fd == fd)
+    g_assert_cmpint (out_fds[0].events, ==, G_IO_IN);
+  else if (out_fds[1].fd == fd)
+    g_assert_cmpint (out_fds[1].events, ==, G_IO_IN);
+  else
+    g_assert_not_reached ();
+
+  /* Add a second pollfd with the same fd but different event, and
+   * lower priority.
+   */
+  source2 = g_io_create_watch (io, G_IO_OUT);
+  g_source_set_priority (source2, G_PRIORITY_LOW);
+  g_source_set_callback (source2, (GSourceFunc) nfds_out_low_cb,
+                         NULL, NULL);
+  g_source_attach (source2, ctx);
+
+  /* g_main_context_query() should still return only 2 pollfds,
+   * one of which has our fd, and a combined events field.
+   */
+  nfds = g_main_context_query (ctx, G_MAXINT, NULL,
+                               out_fds, G_N_ELEMENTS (out_fds));
+  g_assert_cmpint (nfds, ==, 2);
+  if (out_fds[0].fd == fd)
+    g_assert_cmpint (out_fds[0].events, ==, G_IO_IN | G_IO_OUT);
+  else if (out_fds[1].fd == fd)
+    g_assert_cmpint (out_fds[1].events, ==, G_IO_IN | G_IO_OUT);
+  else
+    g_assert_not_reached ();
+
+  /* But if we query with a max priority, we won't see the
+   * lower-priority one.
+   */
+  nfds = g_main_context_query (ctx, G_PRIORITY_DEFAULT, NULL,
+                               out_fds, G_N_ELEMENTS (out_fds));
+  g_assert_cmpint (nfds, ==, 2);
+  if (out_fds[0].fd == fd)
+    g_assert_cmpint (out_fds[0].events, ==, G_IO_IN);
+  else if (out_fds[1].fd == fd)
+    g_assert_cmpint (out_fds[1].events, ==, G_IO_IN);
+  else
+    g_assert_not_reached ();
+
+  /* Third pollfd */
+  source3 = g_io_create_watch (io, G_IO_OUT);
+  g_source_set_priority (source3, G_PRIORITY_DEFAULT);
+  g_source_set_callback (source3, (GSourceFunc) nfds_out_cb,
+                         &source3_ran, NULL);
+  g_source_attach (source3, ctx);
+
+  nfds = g_main_context_query (ctx, G_MAXINT, NULL,
+                               out_fds, G_N_ELEMENTS (out_fds));
+  g_assert_cmpint (nfds, ==, 2);
+  if (out_fds[0].fd == fd)
+    g_assert_cmpint (out_fds[0].events, ==, G_IO_IN | G_IO_OUT);
+  else if (out_fds[1].fd == fd)
+    g_assert_cmpint (out_fds[1].events, ==, G_IO_IN | G_IO_OUT);
+  else
+    g_assert_not_reached ();
+
+  /* Now actually iterate the loop; the fd should be readable and
+   * writable, so source1 and source3 should be triggered, but *not*
+   * source2, since it's lower priority than them. (Though on
+   * G_OS_WIN32, source3 doesn't get triggered, probably because of
+   * giowin32 weirdness...)
+   */
+  g_main_context_iteration (ctx, FALSE);
+
+  g_assert (source1_ran);
+#ifndef G_OS_WIN32
+  g_assert (source3_ran);
+#endif
+
+  g_source_destroy (source1);
+  g_source_unref (source1);
+  g_source_destroy (source2);
+  g_source_unref (source2);
+  g_source_destroy (source3);
+  g_source_unref (source3);
+
+  g_io_channel_unref (io);
+  remove (tmpfile);
+  g_free (tmpfile);
+
+  g_main_context_unref (ctx);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -1592,6 +1752,7 @@ main (int argc, char *argv[])
   g_test_add_func ("/mainloop/wait", test_mainloop_wait);
   g_test_add_func ("/mainloop/unix-file-poll", test_unix_file_poll);
 #endif
+  g_test_add_func ("/mainloop/nfds", test_nfds);
 
   return g_test_run ();
 }