On Mon, May 17, 2010 at 2:16 AM, Mike McCormack
<[email protected]> wrote:
> Hey Guys,
>
> Here's a patch to support epoll in ecore.
>
> Probably needs a bit more testing, but let me know what you think.
Hi Mike, code looks great. I don't agree with Vincent about having yet
another file for it, but I'll suggest some changes in the organization
below:
> +#ifdef HAVE_EPOLL
> +static int epoll_fd = -1;
> +
> +void _ecore_main_loop_init(void)
> +{
> + epoll_fd = epoll_create(1);
> + if (epoll_fd < 0)
> + ERR("Failed to create epoll fd!");
make it a CRIT(), so it will abort when eina is set to abort on
critical. This will make the application not work, so it is critical
IMO.
> +static inline int _ecore_epoll_add(Ecore_Fd_Handler *fdh)
...
> +static inline void _ecore_epoll_remove(Ecore_Fd_Handler *fdh)
...
> +static inline int ecore_epoll_modify(Ecore_Fd_Handler *fdh)
you could make these more generically named: ecore_fdpoller_ or
something like this (not epoll) and have the #ifdef inside them. It is
more similar to what other Ecore code does.
The following I'd make a function ecore_fdpoller_process() or
ecore_fdpoller_post_process() that would be in charge to flag the fdh
structures properly, moving the code out of this main function.
> +#ifndef HAVE_EPOLL
> EINA_INLIST_FOREACH(fd_handlers, fdh)
> if (!fdh->delete_me)
> {
> @@ -560,6 +648,54 @@
> #endif
> return 1;
> }
> +#else /* HAVE_EPOLL */
> + if (_ecore_signal_count_get()) return -1;
> +
> + do
> + {
> + const int num_epoll_fds = 10;
> + struct epoll_event ev[num_epoll_fds];
isn't 10 too short? Better to make it 100 or so? It will not take that
much more memory, and should avoid some syscalls when system is doing
some networking (ie: loading couple of resources in parallel should
activate dozen of fds by itself, then we have X11 and possible more
fds that app use if doing threads/ecore_thread).
> + ret = epoll_wait(epoll_fd, ev, num_epoll_fds, timeout);
ouch, this breaks ecore_main_loop_glib_integrate() as there is no way
to override the poller anymore. In my opinion it is hard to have just
a select() and map it to epoll, it will not be efficient. As we're not
1.0 yet, maybe we could just replace ecore_main_loop_select_func_set()
to receive a different parameter, or receive 2 functions, one for
select() and another for epoll? This needs thinking.
> --- src/lib/ecore/ecore.c (revision 48932)
> +++ src/lib/ecore/ecore.c (working copy)
> @@ -118,6 +118,7 @@
> _ecore_glib_init();
> _ecore_job_init();
> _ecore_loop_time = ecore_time_get();
> + _ecore_main_loop_init();
As someone else said, we better have a shutdown to close() the epoll_fd.
--
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: [email protected]
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202
------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
