[PATCH] event-loop: Use two-step destruction of event loop sources.

[Andreas Ericsson]

On 03/20/2012 02:21 PM, Jonas Ådahl 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.
> 

I prefer my commit message (well doh) since it alsogh explains why this is
a necessary change. That helps future coders who fiddle with it realize
what is necessary to test and what is necessary to protect from.

Some minor things below as well.

> 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;
>   };
> 

It's a good practice to keep types of different sizes listed after
each other in structs, since it means the compiler can squeeze them
together and save memory when building for embedded devices. Not
that most embedded devices are 64-bit yet, but I assume they will be
before long. Safe to ignore for now though.

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

This I'm not too fond of. See below.

>   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)
>   {
> @@ -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
>   };
> 

And here's the fallout. Now not all source->interface structs have a
valid remove() function, and future people will have to check for that
in order to not break stuff horribly if they want to write generic code.
I'd rather you just left the remove function as an empty "return 0"
thing. That will let the compiler optimize away all local calls to it
while we needn't bother with keeping check of which type we're dealing
with.

> @@ -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);
> 

And here is the place where 'idle' sources can never go while they
lack a remove() function.

> +	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);
> +	}
> 

This just adds a lot of redirection to something that really should be
a simple free() call. Getting rid of the in-interface destroy() function
and just using free() here will save memory and make it run a (tiny bit)
faster.


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


Otherwise, it looks neat, and you can probably at least compile-test
your patches straight away, so +1 for this.

-- 
Andreas Ericsson                   andreas.ericsson at op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.