[PATCH] event-loop: Use two-step destruction of event loop sources.
Kristian Høgsberg
krh at bitplanet.net
Tue Mar 20 07:40:55 PDT 2012
On Tue, Mar 20, 2012 at 9:21 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> Instead of directly destroying an event source upon removal only destroy
> it in case its event loop is not dispatching. If dispatching the event
> source is marked for destruction and added to the check list and
> destroyed in post_dispatch_check(); if not it is destroyed immediately.
Good catch and thanks for tracking it down. The patch is pretty good,
but there's just a couple of changes I'd like to see: 1) don't
distinguish between dispatching or not, just always defer the destroy,
2) don't introduce a function pointer when all callers just need free
and 3) move the int fd from all the source subclasses into
wl_event_source and set it to -1 in wl_event_source_destroy and put if
on a new loop->destroy_list (can't use check list for destroying, not
all sources are checked) and 4) don't dispatch epoll events for
sources with fd = -1 and then add a loop after the epoll for loop to
destroy sources on the destroy list.
thanks,
Kristian
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
> src/event-loop.c | 73 ++++++++++++++++++++++++++++++++++-------------------
> 1 files changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/src/event-loop.c b/src/event-loop.c
> index 2dfe0ae..7f2ef80 100644
> --- a/src/event-loop.c
> +++ b/src/event-loop.c
> @@ -39,12 +39,15 @@ struct wl_event_loop {
> int epoll_fd;
> struct wl_list check_list;
> struct wl_list idle_list;
> +
> + int dispatching;
> };
>
> struct wl_event_source_interface {
> int (*dispatch)(struct wl_event_source *source,
> struct epoll_event *ep);
> int (*remove)(struct wl_event_source *source);
> + int (*destroy)(struct wl_event_source *source);
> };
>
> struct wl_event_source {
> @@ -52,6 +55,7 @@ struct wl_event_source {
> struct wl_event_loop *loop;
> struct wl_list link;
> void *data;
> + int destroy;
> };
>
> struct wl_event_source_fd {
> @@ -61,6 +65,13 @@ struct wl_event_source_fd {
> };
>
> static int
> +wl_event_source_generic_destroy(struct wl_event_source *source)
> +{
> + free(source);
> + return 0;
> +}
> +
> +static int
> wl_event_source_fd_dispatch(struct wl_event_source *source,
> struct epoll_event *ep)
> {
> @@ -82,17 +93,14 @@ wl_event_source_fd_remove(struct wl_event_source *source)
> struct wl_event_source_fd *fd_source =
> (struct wl_event_source_fd *) source;
> struct wl_event_loop *loop = source->loop;
> - int fd;
> -
> - fd = fd_source->fd;
> - free(source);
>
> - return epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, fd, NULL);
> + return epoll_ctl(loop->epoll_fd, EPOLL_CTL_DEL, fd_source->fd, NULL);
> }
>
> struct wl_event_source_interface fd_source_interface = {
> wl_event_source_fd_dispatch,
> - wl_event_source_fd_remove
> + wl_event_source_fd_remove,
> + wl_event_source_generic_destroy
> };
>
> WL_EXPORT struct wl_event_source *
> @@ -104,7 +112,7 @@ wl_event_loop_add_fd(struct wl_event_loop *loop,
> struct wl_event_source_fd *source;
> struct epoll_event ep;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -179,13 +187,13 @@ wl_event_source_timer_remove(struct wl_event_source *source)
> (struct wl_event_source_timer *) source;
>
> close(timer_source->fd);
> - free(source);
> return 0;
> }
>
> struct wl_event_source_interface timer_source_interface = {
> wl_event_source_timer_dispatch,
> - wl_event_source_timer_remove
> + wl_event_source_timer_remove,
> + wl_event_source_generic_destroy
> };
>
> WL_EXPORT struct wl_event_source *
> @@ -196,7 +204,7 @@ wl_event_loop_add_timer(struct wl_event_loop *loop,
> struct wl_event_source_timer *source;
> struct epoll_event ep;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -278,13 +286,13 @@ wl_event_source_signal_remove(struct wl_event_source *source)
> (struct wl_event_source_signal *) source;
>
> close(signal_source->fd);
> - free(source);
> return 0;
> }
>
> struct wl_event_source_interface signal_source_interface = {
> wl_event_source_signal_dispatch,
> - wl_event_source_signal_remove
> + wl_event_source_signal_remove,
> + wl_event_source_generic_destroy
> };
>
> WL_EXPORT struct wl_event_source *
> @@ -297,7 +305,7 @@ wl_event_loop_add_signal(struct wl_event_loop *loop,
> struct epoll_event ep;
> sigset_t mask;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -337,17 +345,10 @@ struct wl_event_source_idle {
> wl_event_loop_idle_func_t func;
> };
>
> -static int
> -wl_event_source_idle_remove(struct wl_event_source *source)
> -{
> - free(source);
> -
> - return 0;
> -}
> -
> struct wl_event_source_interface idle_source_interface = {
> NULL,
> - wl_event_source_idle_remove
> + NULL,
> + wl_event_source_generic_destroy
> };
>
> WL_EXPORT struct wl_event_source *
> @@ -357,7 +358,7 @@ wl_event_loop_add_idle(struct wl_event_loop *loop,
> {
> struct wl_event_source_idle *source;
>
> - source = malloc(sizeof *source);
> + source = calloc(1, sizeof *source);
> if (source == NULL)
> return NULL;
>
> @@ -381,11 +382,22 @@ wl_event_source_check(struct wl_event_source *source)
> WL_EXPORT int
> wl_event_source_remove(struct wl_event_source *source)
> {
> + struct wl_event_loop *loop = source->loop;
> +
> if (!wl_list_empty(&source->link))
> wl_list_remove(&source->link);
>
> source->interface->remove(source);
>
> + if (loop->dispatching) {
> + source->destroy = 1;
> +
> + if (!wl_list_empty(&source->link))
> + wl_list_remove(&source->link);
> + wl_event_source_check(source);
> + } else
> + source->interface->destroy(source);
> +
> return 0;
> }
>
> @@ -425,8 +437,13 @@ post_dispatch_check(struct wl_event_loop *loop)
>
> ep.events = 0;
> n = 0;
> - wl_list_for_each_safe(source, next, &loop->check_list, link)
> - n += source->interface->dispatch(source, &ep);
> + wl_list_for_each_safe(source, next, &loop->check_list, link) {
> + if (source->destroy) {
> + wl_list_remove(&source->link);
> + source->interface->destroy(source);
> + } else
> + n += source->interface->dispatch(source, &ep);
> + }
>
> return n;
> }
> @@ -456,11 +473,15 @@ wl_event_loop_dispatch(struct wl_event_loop *loop, int timeout)
> count = epoll_wait(loop->epoll_fd, ep, ARRAY_LENGTH(ep), timeout);
> if (count < 0)
> return -1;
> +
> + loop->dispatching = 1;
> n = 0;
> for (i = 0; i < count; i++) {
> source = ep[i].data.ptr;
> - n += source->interface->dispatch(source, &ep[i]);
> + if (!source->destroy)
> + n += source->interface->dispatch(source, &ep[i]);
> }
> + loop->dispatching = 0;
>
> while (n > 0)
> n = post_dispatch_check(loop);
> --
> 1.7.5.4
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list