Hello Damien!
I have only a few comments about your patch. Please note that I am
commenting on version which doesn't include fix for broken per-session
idebug variable and bug #32156 (I will provide separate review for it).
* damien@stripped <damien@stripped> [07/11/09 16:14]:
> ChangeSet@stripped, 2007-11-09 07:53:37-05:00, dkatz@stripped +2 -0
> WL441:
> - use a list instead of pipe when adding THD for libevent processing
> - now we reset THD::mysys_var when we disassociate THD from physical thread to
> avoid problems with other threads when killing threads waiting for I/O
> - changed how threads are killed: use single pipe for awaking libevent instead of
> using one pipe for each THD
>
In addition to WL number it makes sense to mention its title here since
it is not always convinient to look it up (and not all WLs are public).
I would also mentioned here that in this ChangeSet you do some cleanups
in order to make code conform to our Coding Style.
...
> diff -Nrup a/sql/scheduler.cc b/sql/scheduler.cc
> --- a/sql/scheduler.cc 2007-11-02 15:39:00 -04:00
> +++ b/sql/scheduler.cc 2007-11-09 07:53:32 -05:00
> @@ -94,34 +94,47 @@ void one_thread_per_connection_scheduler
> #include "event.h"
>
> static uint created_threads, killed_threads;
> -static int thd_add_pipe[2]; /*pipe to use for adding connections to libevent*/
> -static pthread_mutex_t LOCK_event_loop;
> -bool kill_pool_threads= FALSE;
> +static bool kill_pool_threads;
> +
> +static struct event thd_add_event;
> +static struct event thd_kill_event;
> +
> +static pthread_mutex_t LOCK_thd_add; /* protects thds_need_adding */
> +static LIST *thds_need_adding; /* list of thds to add to libevent queue */
>
> -static LIST *thds_need_processing;
> +static int thd_add_pipe[2]; /* pipe to signal add a connection to libevent*/
> +static int thd_kill_pipe[2]; /* pipe to signal kill a connection in libevent */
> +
> +/*
> + LOCK_event_loop protects the non-thread safe libevent calls (event_add and
> + event_del) and thds_need_processing and thds_need_adding.
> +*/
The above comment is not quite correct. LOCK_event_loop protects
thds_waiting_for_io and not thds_need_adding.
...
Altough this patch does not address all problems with WL#441
implementation (e.g. Windows build, general portability, broken
per-session debug variable) I still think that it is step in
the right direction and thus should be pushed after considering/
fixing above issues.
--
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
