Davi, thanks a lot for taking your time to review.
I recommitted following your suggestion here.
You'll also notice couple of casts that I added after Win64 compiling and
testing to silence warnings.
An interesting piece that merits explanation is casting from SOCKET to int
(libevent works with int only). It looks unsafe, but it is not - even so the
sizeof(SOCKET) is 8, the actual value is limited to 26 bits (except for
INVALID_SOCKET which is 0xffffffffffffffff)
(26 bits is a limitation for HANDLES - handle is an index to the 2^24 array,
and 2 extra tag bits are used by Windows for own purposes)
There was a discussion about it some time ago on OpenSSL list
http://markmail.org/message/u26dkagwdbund6a5
Other comments inline
> -----Original Message-----
> From: Davi.Arnaut@stripped [mailto:Davi.Arnaut@stripped]
> Sent: Monday, December 15, 2008 11:19 AM
> To: Vladislav Vaintroub
> Cc: commits@stripped
> Subject: Re: bzr commit into mysql-6.0 branch (vvaintroub:2681)
> Bug#41218
>
> 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."
>
Right.
> > === added file 'extra/libevent/WIN32-Code/misc.c'
>
> 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.
>
I took it from libevent 1.4.3 distribution (seems to be the one checked in).
You are right, the latest libevent (1.4.8) does not have it anymore.
> [..]
>
> > === 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"
Thanks for catching it, changed.
>
> 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..
>
Sounds better, suggestion accepted
> >
> > /*
> > 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..
Yep
> > + return true;
>
> Coding style mandates TRUE/FALSE instead of true/false.
I went back to long ORed expression. Returning TRUE from a C++ function that
is declared to return bool is beyond me...
(I also pretend not noticing returning false on success and true on error -
this convention damages my fragile mind;)
> [..]
>
> Otherwise seems okay. Please fix the coding style throughout the file.
>
> Regards,
>
> -- Davi
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
