gmain: don't pass the same fd to g_poll() multiple times

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
This commit is contained in:
Dan Winship 2012-03-13 11:43:38 -04:00
parent 66fc112c74
commit b3e3ed7386
3 changed files with 222 additions and 60 deletions

View 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;
pollrec = pollrec->next;
n_poll++;
/* 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++;
}
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);

View 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)

View 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 ();
}