From: Nick Mathewson Date: Thu, 14 Jan 2010 21:30:40 +0000 (-0500) Subject: Changelist code to defer event changes until just before dispatch X-Git-Tag: release-2.0.4-alpha~79 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=27308aae4d921ecc286ab7a734693c2e27737a99;p=libevent Changelist code to defer event changes until just before dispatch This is necessary or useful for a few reasons: 1) Sometimes applications will add and delete the same event more than once between calls to dispatch. Processing these changes immediately is needless, and potentially expensive (especially if we're on a system that makes one syscall per changed event). Yes, this actually happens in practice for nonpathological code, such as in cases where the user's callback conditionally re-adds a non-persistent event, or where draining a buffer turns off writing and invokes a user callback which adds more data which in turn re-enabled writing. 2) Sometimes we can coalesce multiple changes on the same fd into a single syscall if we know about them in advance. For example, epoll can do an add and a delete at the same time, but only if we have found out about both of them before we tell epoll. 3) Sometimes adding an event that we immediately delete can cause unintended consequences: in kqueue, this makes pending events get reported spuriously. --- diff --git a/Makefile.am b/Makefile.am index 52b15023..275087e7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -137,6 +137,7 @@ noinst_HEADERS = util-internal.h mm-internal.h ipv6-internal.h \ bufferevent-internal.h http-internal.h event-internal.h \ evthread-internal.h ht-internal.h defer-internal.h \ minheap-internal.h log-internal.h evsignal-internal.h evmap-internal.h \ + changelist-internal.h \ ratelim-internal.h include_HEADERS = event.h evhttp.h evdns.h evrpc.h evutil.h diff --git a/event-internal.h b/event-internal.h index 68329761..fe6027fa 100644 --- a/event-internal.h +++ b/event-internal.h @@ -114,6 +114,14 @@ struct common_timeout_list { struct event_base *base; }; +struct event_change; + +struct event_changelist { + struct event_change *changes; + int n_changes; + int changes_size; +}; + struct event_base { /** Function pointers and other data to describe this event_base's * backend. */ @@ -121,6 +129,10 @@ struct event_base { /** Pointer to backend-specific data. */ void *evbase; + /** List of changes to tell backend about at next dispatch. Only used + * by the O(1) backends. */ + struct event_changelist changelist; + /* signal handling info */ const struct eventop *evsigsel; void *evsigbase; @@ -151,7 +163,7 @@ struct event_base { struct deferred_cb_queue defer_queue; /** Mapping from file descriptors to enabled events */ - struct event_io_map io; + struct event_io_map io; /** Mapping from signal numbers to enabled events. */ struct event_signal_map sigmap; diff --git a/event.c b/event.c index 2c830cee..94ae3231 100644 --- a/event.c +++ b/event.c @@ -65,6 +65,7 @@ #include "log-internal.h" #include "evmap-internal.h" #include "iocp-internal.h" +#include "changelist-internal.h" #ifdef _EVENT_HAVE_EVENT_PORTS extern const struct eventop evportops; @@ -318,6 +319,7 @@ event_base_new_with_config(struct event_config *cfg) evmap_io_initmap(&base->io); evmap_signal_initmap(&base->sigmap); + event_changelist_init(&base->changelist); base->evbase = NULL; @@ -492,6 +494,7 @@ event_base_free(struct event_base *base) evmap_io_clear(&base->io); evmap_signal_clear(&base->sigmap); + event_changelist_freemem(&base->changelist); EVTHREAD_FREE_LOCK(base->th_base_lock, EVTHREAD_LOCKTYPE_RECURSIVE); EVTHREAD_FREE_LOCK(base->current_event_lock, @@ -534,6 +537,7 @@ event_reinit(struct event_base *base) return (-1); } + event_changelist_freemem(&base->changelist); /* XXX */ evmap_io_clear(&base->io); evmap_signal_clear(&base->sigmap); diff --git a/evmap.c b/evmap.c index 00cb80e2..18db0909 100644 --- a/evmap.c +++ b/evmap.c @@ -49,6 +49,7 @@ #include "event-internal.h" #include "evmap-internal.h" #include "mm-internal.h" +#include "changelist-internal.h" /** An entry for an evmap_io list: notes all the events that want to read or write on a given fd, and the number of each. @@ -409,7 +410,8 @@ evmap_signal_add(struct event_base *base, int sig, struct event *ev) map, sig, sizeof(struct evmap_signal *)) == -1) return (-1); } - GET_SIGNAL_SLOT_AND_CTOR(ctx, map, sig, evmap_signal, evmap_signal_init, 0); + GET_SIGNAL_SLOT_AND_CTOR(ctx, map, sig, evmap_signal, evmap_signal_init, + base->evsigsel->fdinfo_len); if (TAILQ_EMPTY(&ctx->events)) { if (evsel->add(base, EVENT_SIGNAL(ev), 0, EV_SIGNAL, NULL) == -1) @@ -467,3 +469,231 @@ evmap_io_get_fdinfo(struct event_io_map *map, evutil_socket_t fd) else return NULL; } + +/** Per-fd structure for use with changelists. It keeps track, for each fd or + * signal using the changelist, of where its entry in the changelist is. + */ +struct event_changelist_fdinfo { + int idxplus1; /* this is the index +1, so that memset(0) will make it + * a no-such-element */ +}; + +void +event_changelist_init(struct event_changelist *changelist) +{ + changelist->changes = NULL; + changelist->changes_size = 0; + changelist->n_changes = 0; +} + +/** Helper: return the changelist_fdinfo corresponding to a given change. */ +static inline struct event_changelist_fdinfo * +event_change_get_fdinfo(struct event_base *base, + const struct event_change *change) +{ + char *ptr; + if (change->read_change & EV_CHANGE_SIGNAL) { + struct evmap_signal *ctx; + GET_SIGNAL_SLOT(ctx, &base->sigmap, change->fd, evmap_signal); + ptr = ((char*)ctx) + sizeof(struct evmap_signal); + } else { + struct evmap_io *ctx; + GET_IO_SLOT(ctx, &base->io, change->fd, evmap_io); + ptr = ((char*)ctx) + sizeof(struct evmap_io); + } + return (void*)ptr; +} + +#ifdef DEBUG_CHANGELIST +/** Make sure that the changelist is consistent with the evmap structures. */ +static void +event_changelist_check(struct event_base *base) +{ + int i; + struct event_changelist *changelist = &base->changelist; + + EVUTIL_ASSERT(changelist->changes_size >= changelist->n_changes); + for (i = 0; i < changelist->n_changes; ++i) { + struct event_change *c = &changelist->changes[i]; + struct event_changelist_fdinfo *f; + EVUTIL_ASSERT(c->fd >= 0); + f = event_change_get_fdinfo(base, c); + EVUTIL_ASSERT(f); + EVUTIL_ASSERT(f->idxplus1 == i + 1); + } + + for (i = 0; i < base->io.nentries; ++i) { + struct evmap_io *io = base->io.entries[i]; + struct event_changelist_fdinfo *f; + if (!io) + continue; + f = (void*) + ( ((char*)io) + sizeof(struct evmap_io) ); + if (f->idxplus1) { + struct event_change *c = &changelist->changes[f->idxplus1 - 1]; + EVUTIL_ASSERT(c->fd == i); + } + } +} +#else +#define event_changelist_check(base) ((void)0) +#endif + +void +event_changelist_remove_all(struct event_changelist *changelist, + struct event_base *base) +{ + int i; + + event_changelist_check(base); + + for (i = 0; i < changelist->n_changes; ++i) { + struct event_change *ch = &changelist->changes[i]; + struct event_changelist_fdinfo *fdinfo = + event_change_get_fdinfo(base, ch); + EVUTIL_ASSERT(fdinfo->idxplus1 == i + 1); + fdinfo->idxplus1 = 0; + } + + changelist->n_changes = 0; + + event_changelist_check(base); +} + +void +event_changelist_freemem(struct event_changelist *changelist) +{ + if (changelist->changes) + mm_free(changelist->changes); + event_changelist_init(changelist); /* zero it all out. */ +} + +/** Increase the size of 'changelist' to hold more changes. */ +static int +event_changelist_grow(struct event_changelist *changelist) +{ + int new_size; + struct event_change *new_changes; + if (changelist->changes_size < 64) + new_size = 64; + else + new_size = changelist->changes_size * 2; + + new_changes = mm_realloc(changelist->changes, + new_size * sizeof(struct event_change)); + + if (EVUTIL_UNLIKELY(new_changes == NULL)) + return (-1); + + changelist->changes = new_changes; + changelist->changes_size = new_size; + + return (0); +} + +/** Return a pointer to the changelist entry for the file descriptor or signal + * 'fd', whose fdinfo is 'fdinfo'. If none exists, construct it, setting its + * old_events field to old_events. + */ +static struct event_change * +event_changelist_get_or_construct(struct event_changelist *changelist, + evutil_socket_t fd, + short old_events, + struct event_changelist_fdinfo *fdinfo) +{ + struct event_change *change; + + if (fdinfo->idxplus1 == 0) { + int idx; + EVUTIL_ASSERT(changelist->n_changes <= changelist->changes_size); + + if (changelist->n_changes == changelist->changes_size) { + if (event_changelist_grow(changelist) < 0) + return NULL; + } + + idx = changelist->n_changes++; + change = &changelist->changes[idx]; + fdinfo->idxplus1 = idx + 1; + + memset(change, 0, sizeof(struct event_change)); + change->fd = fd; + change->old_events = old_events; + } else { + change = &changelist->changes[fdinfo->idxplus1 - 1]; + EVUTIL_ASSERT(change->fd == fd); + } + return change; +} + +int +event_changelist_add(struct event_base *base, int fd, short old, short events, + void *p) +{ + struct event_changelist *changelist = &base->changelist; + struct event_changelist_fdinfo *fdinfo = p; + struct event_change *change; + + event_changelist_check(base); + + change = event_changelist_get_or_construct(changelist, fd, old, fdinfo); + if (!change) + return -1; + + /* An add replaces any previous delete, but doesn't result in a no-op, + * since the delete might fail (because the fd had been closed since + * the last add, for instance. */ + + if (events & (EV_READ|EV_SIGNAL)) { + change->read_change = EV_CHANGE_ADD | + (events & (EV_ET|EV_PERSIST|EV_SIGNAL)); + } + if (events & EV_WRITE) { + change->write_change = EV_CHANGE_ADD | + (events & (EV_ET|EV_PERSIST|EV_SIGNAL)); + } + + event_changelist_check(base); + return (0); +} + +int +event_changelist_del(struct event_base *base, int fd, short old, short events, + void *p) +{ + struct event_changelist *changelist = &base->changelist; + struct event_changelist_fdinfo *fdinfo = p; + struct event_change *change; + + event_changelist_check(base); + change = event_changelist_get_or_construct(changelist, fd, old, fdinfo); + event_changelist_check(base); + if (!change) + return -1; + + /* A delete removes any previous add, rather than replacing it: + on those platforms where "add, delete, dispatch" is not the same + as "no-op" dispatch, we want the no-op behavior. + + If we have a no-op item, we could it from the list entirely, but + really there's not much point: skipping the no-op change when we do + the dispatch later is far cheaper than rejuggling the array now. + */ + + if (events & (EV_READ|EV_SIGNAL)) { + if (change->read_change & EV_CHANGE_ADD) + change->read_change = 0; + else + change->read_change = EV_CHANGE_DEL; + } + if (events & EV_WRITE) { + if (change->write_change & EV_CHANGE_ADD) + change->write_change = 0; + else + change->write_change = EV_CHANGE_DEL; + } + + event_changelist_check(base); + return (0); +} +