Hi Vladislav,
A few comments below.
On 12/4/08 7:21 AM, Vladislav Vaintroub wrote:
> #At file:///G:/bzr/mysql-6.0-wtf-threadpool/
>
> 2681 Vladislav Vaintroub 2008-12-04
> Bug#41218 - Windows port for thread pooling
>
> * add porting stuff (Win32-Code subdirectory )from libevent distribution
Typo: "subdirectory )from" -> "subdirectory) from"
> * add libevent library to CMake build
> * adopt code in scheduler
> Replace pipes that were used as communication mechanism with libevent with
> sockets.
"with socket pairs."
[..]
> added:
> extra/libevent/CMakeLists.txt
> extra/libevent/WIN32-Code/
> extra/libevent/WIN32-Code/config.h
> extra/libevent/WIN32-Code/misc.c
> extra/libevent/WIN32-Code/misc.h
> extra/libevent/WIN32-Code/tree.h
> extra/libevent/WIN32-Code/win32.c
> modified:
> CMakeLists.txt
> extra/libevent/Makefile.am
> extra/libevent/evutil.h
> include/config-win.h
> sql/CMakeLists.txt
> sql/mysqld.cc
> sql/scheduler.cc
>
> per-file messages:
> CMakeLists.txt
> compile libevent on Windows
> extra/libevent/CMakeLists.txt
> compile libevent on Windows
> extra/libevent/Makefile.am
> compile libevent on Windows
> extra/libevent/WIN32-Code
> compile libevent on Windows
> extra/libevent/WIN32-Code/config.h
> compile libevent on Windows
> extra/libevent/WIN32-Code/misc.c
> compile libevent on Windows
> extra/libevent/WIN32-Code/misc.h
> compile libevent on Windows
> extra/libevent/WIN32-Code/tree.h
> compile libevent on Windows
> extra/libevent/WIN32-Code/win32.c
> compile libevent on Windows
> extra/libevent/evutil.h
> compile libevent on Windows
> include/config-win.h
> compile libevent on Windows
> sql/CMakeLists.txt
> link mysqld with libevent
> sql/mysqld.cc
> pool of threads only works with sockets on Windows.
> Abort if named pipe or shared memory protocol used.
> sql/scheduler.cc
> Use sockets as communication mechanism with libevent instead of pipes
> Pipes do not work on windows - a limitation of win32 libevent , underlying
> select() does not support anything but sockets.
> === modified file 'CMakeLists.txt'
> --- a/CMakeLists.txt 2008-11-05 10:19:19 +0000
> +++ b/CMakeLists.txt 2008-12-04 09:21:32 +0000
> @@ -269,6 +269,7 @@ ADD_SUBDIRECTORY(mysys)
> ADD_SUBDIRECTORY(scripts)
> ADD_SUBDIRECTORY(zlib)
> ADD_SUBDIRECTORY(extra/yassl)
> +ADD_SUBDIRECTORY(extra/libevent)
> ADD_SUBDIRECTORY(extra/yassl/taocrypt)
> ADD_SUBDIRECTORY(extra)
> ADD_SUBDIRECTORY(storage/heap)
>
> === added file 'extra/libevent/CMakeLists.txt'
> --- a/extra/libevent/CMakeLists.txt 1970-01-01 00:00:00 +0000
> +++ b/extra/libevent/CMakeLists.txt 2008-12-04 09:21:32 +0000
> @@ -0,0 +1,34 @@
> +INCLUDE_DIRECTORIES(
> + ${CMAKE_SOURCE_DIR}/extra/libevent
> + ${CMAKE_SOURCE_DIR}/extra/libevent/compat
> + ${CMAKE_SOURCE_DIR}/extra/libevent/WIN32-Code)
> +
> +IF(MSVC)
> + ADD_DEFINITIONS("-DWIN32 -DHAVE_CONFIG_H")
> +ENDIF(MSVC)
> +
> +SET(LIBEVENT_SOURCES
> + buffer.c
> + evbuffer.c
> + event.c
> + evutil.c
> + log.c
> + signal.c
> + strlcpy.c
> + WIN32-Code/win32.c
> + WIN32-Code/config.h
> + WIN32-Code/misc.c
> + WIN32-Code/misc.h
> + event-internal.h
> + event.h
> + evsignal.h
> + evutil.h
> + log.h
> + min_heap.h
> + strlcpy-internal.h
> + )
> +
> +CONFIGURE_FILE(WIN32-Code/config.h event-config.h COPYONLY)
> +IF(NOT SOURCE_SUBLIBS)
> + ADD_LIBRARY(libevent ${LIBEVENT_SOURCES})
> +ENDIF(NOT SOURCE_SUBLIBS)
>
> === modified file 'extra/libevent/Makefile.am'
> --- a/extra/libevent/Makefile.am 2008-04-29 03:50:47 +0000
> +++ b/extra/libevent/Makefile.am 2008-12-04 09:21:32 +0000
> @@ -5,7 +5,9 @@ EXTRA_DIST = README compat/sys
> signal.c devpoll.c epoll_sub.c evdns.c event_tagging.c evrpc.c http.c \
> log.c select.c strlcpy.c \
> evdns.h event.h evrpc-internal.h evsignal.h http-internal.h log.h \
> - min_heap.h event-internal.h evhttp.h evrpc.h evutil.h strlcpy-internal.h
> + min_heap.h event-internal.h evhttp.h evrpc.h evutil.h strlcpy-internal.h \
> + WIN32-Code/misc.c WIN32-Code/misc.h WIN32-Code/config.h WIN32-Code/tree.h \
> + WIN32-Code/win32.c CMakeLists.txt
>
>
> DISTCLEANFILES = event-config.h
>
> === added directory 'extra/libevent/WIN32-Code'
> === added file 'extra/libevent/WIN32-Code/config.h'
> --- a/extra/libevent/WIN32-Code/config.h 1970-01-01 00:00:00 +0000
> +++ b/extra/libevent/WIN32-Code/config.h 2008-12-04 09:21:32 +0000
> @@ -0,0 +1,247 @@
[..]
>
> === added file 'extra/libevent/WIN32-Code/misc.c'
> --- a/extra/libevent/WIN32-Code/misc.c 1970-01-01 00:00:00 +0000
> +++ b/extra/libevent/WIN32-Code/misc.c 2008-12-04 09:21:32 +0000
> @@ -0,0 +1,38 @@
> +#include <stdio.h>
> +#include <string.h>
> +#include <windows.h>
> +#include <sys/timeb.h>
> +#include <time.h>
> +
> +#ifdef __GNUC__
> +/*our prototypes for timeval and timezone are in here, just in case the above
> + headers don't have them*/
> +#include "misc.h"
> +#endif
> +
> +/****************************************************************************
> + *
> + * Function: gettimeofday(struct timeval *, struct timezone *)
> + *
> + * Purpose: Get current time of day.
> + *
> + * Arguments: tv => Place to store the curent time of day.
> + * tz => Ignored.
> + *
> + * Returns: 0 => Success.
> + *
> + ****************************************************************************/
> +
> +#ifndef HAVE_GETTIMEOFDAY
> +int gettimeofday(struct timeval *tv, struct timezone *tz) {
> + struct _timeb tb;
> +
> + if(tv == NULL)
> + return -1;
> +
> + _ftime(&tb);
> + tv->tv_sec = (long) tb.time;
> + tv->tv_usec = ((int) tb.millitm) * 1000;
> + return 0;
> +}
> +#endif
Where does this file come from? I don't see it in the standard libevent
tarball and livevent already defines a WIN32 wrapper for gettimeofday.
[..]
> === modified file 'extra/libevent/evutil.h'
> --- a/extra/libevent/evutil.h 2008-04-28 17:52:20 +0000
> +++ b/extra/libevent/evutil.h 2008-12-04 09:21:32 +0000
> @@ -55,8 +55,8 @@ extern "C" {
> #define ev_uint64_t uint64_t
> #define ev_int64_t int64_t
> #elif defined(WIN32)
> -#define ev_uint64_t __uint64_t
> -#define ev_int64_t __int64_t
> +#define ev_uint64_t unsigned __int64
> +#define ev_int64_t __int64
> #elif _EVENT_SIZEOF_LONG_LONG == 8
> #define ev_uint64_t unsigned long long
> #define ev_int64_t long long
>
> === modified file 'include/config-win.h'
> --- a/include/config-win.h 2008-07-23 08:52:08 +0000
> +++ b/include/config-win.h 2008-12-04 09:21:32 +0000
> @@ -433,3 +433,7 @@ inline double ulonglong2double(ulonglong
> #define HAVE_CHARSET_utf8mb3 1
> #define HAVE_UCA_COLLATIONS 1
> #define HAVE_BOOL 1
> +#ifndef EMBEDDED_LIBRARY
> +#define HAVE_LIBEVENT 1
> +#define HAVE_POOL_OF_THREADS 1
> +#endif
>
> === modified file 'sql/CMakeLists.txt'
> --- a/sql/CMakeLists.txt 2008-11-12 01:37:42 +0000
> +++ b/sql/CMakeLists.txt 2008-12-04 09:21:32 +0000
> @@ -20,6 +20,7 @@ INCLUDE_DIRECTORIES(${CMAKE_SOURCE_DIR}/
> ${CMAKE_SOURCE_DIR}/regex
> ${CMAKE_SOURCE_DIR}/zlib
> ${CMAKE_SOURCE_DIR}/sql/backup
> + ${CMAKE_SOURCE_DIR}/extra/libevent
> )
>
> SET_SOURCE_FILES_PROPERTIES(${CMAKE_SOURCE_DIR}/sql/sql_yacc.h
> @@ -82,7 +83,7 @@ ADD_EXECUTABLE(mysqld
>
> TARGET_LINK_LIBRARIES(mysqld
> heap myisam myisammrg mysys yassl zlib dbug yassl
> - taocrypt strings vio regex)
> + taocrypt strings vio regex libevent)
>
> TARGET_LINK_LIBRARIES(mysqld backup)
>
>
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc 2008-11-15 17:02:01 +0000
> +++ b/sql/mysqld.cc 2008-12-04 09:21:32 +0000
> @@ -4381,6 +4381,13 @@ static void handle_connections_methods()
> sql_print_error("TCP/IP, --shared-memory, or --named-pipe should be configured
> on NT OS");
> unireg_abort(1); // Will not return
> }
> + if((hPipe != INVALID_HANDLE_VALUE || opt_enable_shared_memory) &&
Please add space after the "if". "if((hPipe" -> "if ((hPipe"
> + global_system_variables.thread_handling== SCHEDULER_POOL_OF_THREADS)
"thread_handling== SCHEDULER_POOL_OF_THREADS" ->
"thread_handling == SCHEDULER_POOL_OF_THREADS"
> + {
> + sql_print_error("thread_handling=pool-of-threads cannot be used with "\
> + "shared memory or named pipe protocols");
sql_print_error("thread_handling=pool-of-threads cannot be used "
"with shared memory or named pipe protocols");
> + unireg_abort(1);
> + }
> #endif
>
> pthread_mutex_lock(&LOCK_thread_count);
>
> === modified file 'sql/scheduler.cc'
> --- a/sql/scheduler.cc 2008-10-17 17:47:16 +0000
> +++ b/sql/scheduler.cc 2008-12-04 09:21:32 +0000
> @@ -105,8 +105,8 @@ 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 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 */
> +static int thd_add_socketpair[2]; /* pipe to signal add a connection to libevent*/
> +static int thd_kill_socketpair[2]; /* pipe to signal kill a connection in libevent
> */
May I suggest to drop the socket part of the name? Something like:
static int thd_add_pair[2];
static int thd_kill_pair[2];
Should be enough..
>
> /*
> LOCK_event_loop protects the non-thread safe libevent calls (event_add and
> @@ -132,16 +132,26 @@ void libevent_kill_thd_callback(int Fd,
> Returns TRUE if there is an error.
> */
>
> -static bool init_pipe(int pipe_fds[])
> +static bool init_socketpair(int sock_pair[])
> {
> - int flags;
> - return pipe(pipe_fds)< 0 ||
> - (flags= fcntl(pipe_fds[0], F_GETFL)) == -1 ||
> - fcntl(pipe_fds[0], F_SETFL, flags | O_NONBLOCK) == -1 ||
> - (flags= fcntl(pipe_fds[1], F_GETFL)) == -1 ||
> - fcntl(pipe_fds[1], F_SETFL, flags | O_NONBLOCK) == -1;
> + sock_pair[0]= sock_pair[1]= -1;
> + if (evutil_socketpair( AF_UNIX, SOCK_STREAM,0, sock_pair)< 0)
> + return true;
> + if(evutil_make_socket_nonblocking(sock_pair[0]) == -1)
Space after the if in "if(evutil_", remove space "socketpair( AF_UNIX",
add space "SOCK_STREAM,0,", etc..
> + return true;
Coding style mandates TRUE/FALSE instead of true/false.
[..]
Otherwise seems okay. Please fix the coding style throughout the file.
Regards,
-- Davi
