From e47042fe2604625fffdba015c2a9bdaed30e1f0c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Feb 2011 20:05:41 -0500 Subject: [PATCH 1/3] Optimize the case where we reinsert an existing timeout --- event.c | 36 ++++++++++++++++++++++++++++++++++-- minheap-internal.h | 18 ++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/event.c b/event.c index fa8c722d..63927085 100644 --- a/event.c +++ b/event.c @@ -133,6 +133,8 @@ static inline int event_del_internal(struct event *ev); static void event_queue_insert(struct event_base *, struct event *, int); static void event_queue_remove(struct event_base *, struct event *, int); +static void event_queue_reinsert(struct event_base *,struct event *ev,int); + static int event_haveevents(struct event_base *); static int event_process_active(struct event_base *); @@ -146,6 +148,9 @@ static inline void event_persist_closure(struct event_base *, struct event *ev); static int evthread_notify_base(struct event_base *base); +static void insert_common_timeout_inorder(struct common_timeout_list *ctl, + struct event *ev); + #ifndef _EVENT_DISABLE_DEBUG_MODE /* These functions implement a hashtable of which 'struct event *' structures * have been setup or added. We don't want to trust the content of the struct @@ -2030,7 +2035,7 @@ event_add_internal(struct event *ev, const struct timeval *tv, /* XXX I believe this is needless. */ if (min_heap_elt_is_top(ev)) notify = 1; - event_queue_remove(base, ev, EVLIST_TIMEOUT); + /*event_queue_remove(base, ev, EVLIST_TIMEOUT);*/ } /* Check if it is active due to a timeout. Rescheduling @@ -2070,7 +2075,8 @@ event_add_internal(struct event *ev, const struct timeval *tv, "event_add: timeout in %d seconds, call %p", (int)tv->tv_sec, ev->ev_callback)); - event_queue_insert(base, ev, EVLIST_TIMEOUT); + event_queue_reinsert(base, ev, EVLIST_TIMEOUT); + if (common_timeout) { struct common_timeout_list *ctl = get_common_timeout_list(base, &ev->ev_timeout); @@ -2458,6 +2464,32 @@ event_queue_remove(struct event_base *base, struct event *ev, int queue) } } +/* Remove and reinsert 'ev' into the appropriate queue. Only EVLIST_TIMEOUT + * is supported. */ +static void +event_queue_reinsert(struct event_base *base, struct event *ev, int queue) +{ + if (!(ev->ev_flags & queue)) { + event_queue_insert(base, ev, queue); + return; + } + + if (queue != EVLIST_TIMEOUT) { + event_errx(1, "%s: Unsupported queue %x", __func__, queue); + return; /* unreached */ + } + + if (is_common_timeout(&ev->ev_timeout, base)) { + struct common_timeout_list *ctl = + get_common_timeout_list(base, &ev->ev_timeout); + TAILQ_REMOVE(&ctl->events, ev, + ev_timeout_pos.ev_next_with_common_timeout); + insert_common_timeout_inorder(ctl, ev); + } else { + min_heap_adjust(&base->timeheap, ev); + } +} + /* Add 'ev' to the common timeout list in 'ev'. */ static void insert_common_timeout_inorder(struct common_timeout_list *ctl, diff --git a/minheap-internal.h b/minheap-internal.h index 8055e903..39e3946a 100644 --- a/minheap-internal.h +++ b/minheap-internal.h @@ -53,6 +53,7 @@ static inline struct event* min_heap_top(min_heap_t* s); static inline int min_heap_reserve(min_heap_t* s, unsigned n); static inline int min_heap_push(min_heap_t* s, struct event* e); static inline struct event* min_heap_pop(min_heap_t* s); +static inline int min_heap_adjust(min_heap_t *s, struct event* e); static inline int min_heap_erase(min_heap_t* s, struct event* e); static inline void min_heap_shift_up_(min_heap_t* s, unsigned hole_index, struct event* e); static inline void min_heap_shift_down_(min_heap_t* s, unsigned hole_index, struct event* e); @@ -115,6 +116,23 @@ int min_heap_erase(min_heap_t* s, struct event* e) return -1; } +int min_heap_adjust(min_heap_t *s, struct event *e) +{ + if (-1 == e->ev_timeout_pos.min_heap_idx) { + return min_heap_push(s, e); + } else { + unsigned parent = (e->ev_timeout_pos.min_heap_idx - 1) / 2; + /* The position of e has changed; we shift it up or down + * as needed. We can't need to do both. */ + if (e->ev_timeout_pos.min_heap_idx > 0 && min_heap_elem_greater(s->p[parent], e)) + min_heap_shift_up_(s, e->ev_timeout_pos.min_heap_idx, e); + else + min_heap_shift_down_(s, e->ev_timeout_pos.min_heap_idx, e); + return 0; + } + return -1; +} + int min_heap_reserve(min_heap_t* s, unsigned n) { if (s->a < n) From 77a96fd90186251f5e2ffeafefbf1e401aefd69a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Feb 2011 20:09:16 -0500 Subject: [PATCH 2/3] Remove a needless base-notify when rescheduling the first timeout We don't need to wake up the base when rescheduling the timeout that will expire first: the base will already wake up, see that nothing is ready, and go back to sleep. --- event.c | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/event.c b/event.c index 63927085..b6a5f078 100644 --- a/event.c +++ b/event.c @@ -2027,17 +2027,6 @@ event_add_internal(struct event *ev, const struct timeval *tv, if (ev->ev_closure == EV_CLOSURE_PERSIST && !tv_is_absolute) ev->ev_io_timeout = *tv; - /* - * we already reserved memory above for the case where we - * are not replacing an existing timeout. - */ - if (ev->ev_flags & EVLIST_TIMEOUT) { - /* XXX I believe this is needless. */ - if (min_heap_elt_is_top(ev)) - notify = 1; - /*event_queue_remove(base, ev, EVLIST_TIMEOUT);*/ - } - /* Check if it is active due to a timeout. Rescheduling * this timeout before the callback can be executed * removes it from the active list. */ From dd5189b1c3f904e15f60198dcd2800c4014993e5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 2 Feb 2011 20:23:23 -0500 Subject: [PATCH 3/3] Save a needless comparison when removing/adjusting timeouts --- minheap-internal.h | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/minheap-internal.h b/minheap-internal.h index 39e3946a..53b88886 100644 --- a/minheap-internal.h +++ b/minheap-internal.h @@ -46,7 +46,6 @@ static inline void min_heap_ctor(min_heap_t* s); static inline void min_heap_dtor(min_heap_t* s); static inline void min_heap_elem_init(struct event* e); static inline int min_heap_elt_is_top(const struct event *e); -static inline int min_heap_elem_greater(struct event *a, struct event *b); static inline int min_heap_empty(min_heap_t* s); static inline unsigned min_heap_size(min_heap_t* s); static inline struct event* min_heap_top(min_heap_t* s); @@ -56,12 +55,11 @@ static inline struct event* min_heap_pop(min_heap_t* s); static inline int min_heap_adjust(min_heap_t *s, struct event* e); static inline int min_heap_erase(min_heap_t* s, struct event* e); static inline void min_heap_shift_up_(min_heap_t* s, unsigned hole_index, struct event* e); +static inline void min_heap_shift_up_unconditional_(min_heap_t* s, unsigned hole_index, struct event* e); static inline void min_heap_shift_down_(min_heap_t* s, unsigned hole_index, struct event* e); -int min_heap_elem_greater(struct event *a, struct event *b) -{ - return evutil_timercmp(&a->ev_timeout, &b->ev_timeout, >); -} +#define min_heap_elem_greater(a, b) \ + (evutil_timercmp(&(a)->ev_timeout, &(b)->ev_timeout, >)) void min_heap_ctor(min_heap_t* s) { s->p = 0; s->n = 0; s->a = 0; } void min_heap_dtor(min_heap_t* s) { if (s->p) mm_free(s->p); } @@ -107,7 +105,7 @@ int min_heap_erase(min_heap_t* s, struct event* e) to be less than the parent, it can't need to shift both up and down. */ if (e->ev_timeout_pos.min_heap_idx > 0 && min_heap_elem_greater(s->p[parent], last)) - min_heap_shift_up_(s, e->ev_timeout_pos.min_heap_idx, last); + min_heap_shift_up_unconditional_(s, e->ev_timeout_pos.min_heap_idx, last); else min_heap_shift_down_(s, e->ev_timeout_pos.min_heap_idx, last); e->ev_timeout_pos.min_heap_idx = -1; @@ -125,7 +123,7 @@ int min_heap_adjust(min_heap_t *s, struct event *e) /* The position of e has changed; we shift it up or down * as needed. We can't need to do both. */ if (e->ev_timeout_pos.min_heap_idx > 0 && min_heap_elem_greater(s->p[parent], e)) - min_heap_shift_up_(s, e->ev_timeout_pos.min_heap_idx, e); + min_heap_shift_up_unconditional_(s, e->ev_timeout_pos.min_heap_idx, e); else min_heap_shift_down_(s, e->ev_timeout_pos.min_heap_idx, e); return 0; @@ -149,6 +147,18 @@ int min_heap_reserve(min_heap_t* s, unsigned n) return 0; } +void min_heap_shift_up_unconditional_(min_heap_t* s, unsigned hole_index, struct event* e) +{ + unsigned parent = (hole_index - 1) / 2; + do + { + (s->p[hole_index] = s->p[parent])->ev_timeout_pos.min_heap_idx = hole_index; + hole_index = parent; + parent = (hole_index - 1) / 2; + } while (hole_index && min_heap_elem_greater(s->p[parent], e)); + (s->p[hole_index] = e)->ev_timeout_pos.min_heap_idx = hole_index; +} + void min_heap_shift_up_(min_heap_t* s, unsigned hole_index, struct event* e) { unsigned parent = (hole_index - 1) / 2;