On Mon, Feb 04, 2013 at 05:56:43PM +0100, Laszlo Ersek wrote:
> one remark below:
>
> On 02/04/13 13:12, Stefan Hajnoczi wrote:
> > Convert glib file descriptor polling from rfds/wfds/xfds to GPollFD.
> >
> > The Windows code still needs poll_fds[] and n_poll_fds but they can now
> > become local variables.
> >
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > main-loop.c | 71
> > +++++++++++++++++++------------------------------------------
> > 1 file changed, 22 insertions(+), 49 deletions(-)
> >
> > diff --git a/main-loop.c b/main-loop.c
> > index f1dcd14..12b0213 100644
> > --- a/main-loop.c
> > +++ b/main-loop.c
> > @@ -145,8 +145,6 @@ int qemu_init_main_loop(void)
> >
> > static fd_set rfds, wfds, xfds;
> > static int nfds;
> > -static GPollFD poll_fds[1024 * 2]; /* this is probably overkill */
> > -static int n_poll_fds;
> > static int max_priority;
> >
> > /* Load rfds/wfds/xfds into gpollfds. Will be removed a few commits
> > later. */
> > @@ -206,65 +204,39 @@ static void gpollfds_to_select(int ret)
> > }
> >
> > #ifndef _WIN32
> > -static void glib_select_fill(int *max_fd, fd_set *rfds, fd_set *wfds,
> > - fd_set *xfds, uint32_t *cur_timeout)
> > +static int glib_pollfds_idx;
> > +static int glib_n_poll_fds;
> > +
> > +static void glib_pollfds_fill(uint32_t *cur_timeout)
> > {
> > GMainContext *context = g_main_context_default();
> > - int i;
> > int timeout = 0;
> > + int n;
> >
> > g_main_context_prepare(context, &max_priority);
> >
> > - n_poll_fds = g_main_context_query(context, max_priority, &timeout,
> > - poll_fds, ARRAY_SIZE(poll_fds));
> > - g_assert(n_poll_fds <= ARRAY_SIZE(poll_fds));
> > -
> > - for (i = 0; i < n_poll_fds; i++) {
> > - GPollFD *p = &poll_fds[i];
> > -
> > - if ((p->events & G_IO_IN)) {
> > - FD_SET(p->fd, rfds);
> > - *max_fd = MAX(*max_fd, p->fd);
> > - }
> > - if ((p->events & G_IO_OUT)) {
> > - FD_SET(p->fd, wfds);
> > - *max_fd = MAX(*max_fd, p->fd);
> > - }
> > - if ((p->events & G_IO_ERR)) {
> > - FD_SET(p->fd, xfds);
> > - *max_fd = MAX(*max_fd, p->fd);
> > - }
> > - }
> > + glib_pollfds_idx = gpollfds->len;
> > + n = glib_n_poll_fds;
> > + do {
> > + GPollFD *pfds;
> > + glib_n_poll_fds = n;
> > + g_array_set_size(gpollfds, glib_pollfds_idx + glib_n_poll_fds);
> > + pfds = &g_array_index(gpollfds, GPollFD, glib_pollfds_idx);
> > + n = g_main_context_query(context, max_priority, &timeout, pfds,
> > + glib_n_poll_fds);
> > + } while (n != glib_n_poll_fds);
>
> This seems clever, but I'm not sure about all the use cases.
>
> When it runs for the very first time, it goes in with
> glib_n_poll_fds==0, ie. it queries the number of records needed. Then it
> resizes the array so that there are that many records from
> glib_pollfds_idx onwards. The second query is expected to succeed.
>
> I wonder if you could just issue a query call up-front with n_fds==0 (to
> retrieve the number of records needed) and eliminate the loop. Also,
> although the "gpollfds" array is emptied for each iteration in
> main_loop_wait() -- and consequently "glib_pollfds_idx" is re-set --,
> "glib_n_poll_fds" seems to survive the iterations. Do you intend that as
> some kind of caching?
Yes and this is the approach that glib itself uses when it calls
g_main_context_query().
g_main_context_query() takes O(n) time in the number of GPollFDs. I had
hoped n_fds==0 would be a O(1) operation but that is not the case.
Because of this I figured it is better to keep the last n_fds stored
between iterations - we'll guess right most of the time.
> Again, I wonder if you could just set glib_n_poll_fds=0 right beside
> where you re-set "glib_pollfds_idx". If so, then you could eliminate the
> loop I believe, just pre-query the number of records needed without
> actually storing any.
The most common scenario is where glib_n_poll_fds stays the same between
calls. Querying with n_fds==0 would iterate over all GPollFDs
needlessly. We need glib_n_poll_fds again in the glib_pollfds_poll()
function (it cannot be dropped) so I feel the complexity trade-off is
worth it here.
Stefan
