List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:October 1 2010 1:19am
Subject:Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)
View as plain text  
Hi Wlad,

On 9/27/10 4:38 PM, Vladislav Vaintroub wrote:
> #At file:///H:/bzr-new/55bf/ based on
> revid:mats.kindahl@stripped
>
>   3204 Vladislav Vaintroub	2010-09-27
>        Use native windows conditions and rwlocks i mysys if available.
>
>        This patch is intended to be applied  on top of
>        dlenev's prlock patch.
>
>        rwlocks used here do not implement extra properties required by runtime
>        team, commonly knows as "readers preference" ( which in detail means
>        starving writer, recursive read acquires, serialization of unlock
>        calls and perhaps more).
>
>        Also , since this patch is using pthread_once heavily, optimize it to save
>        and interlocked operation in case once routine has already finished.

Some comments and questions below.

>      modified:
>        include/my_pthread.h
>        mysys/my_wincond.c
>        mysys/my_winthread.c
>        mysys/thr_rwlock.c
> === modified file 'include/my_pthread.h'
> --- a/include/my_pthread.h	2010-08-10 21:12:01 +0000
> +++ b/include/my_pthread.h	2010-09-27 19:38:44 +0000
> @@ -49,6 +49,7 @@ typedef struct st_pthread_link {
>   } pthread_link;
>
>   typedef struct {
> +  CONDITION_VARIABLE native_cond; /*native condition (Vista and later) */

Add space after *.

>     uint32 waiting;
>     CRITICAL_SECTION lock_waiting;
>
> @@ -651,6 +652,10 @@ extern int rw_pr_init(struct st_my_rw_lo
>     read/write locks which prefer readers we have to use own implementation.
>   */
>   typedef struct st_my_rw_lock_t {
> +#ifdef _WIN32
> +	SRWLOCK srwlock;            /* native rwlock (Vista and later) */
> +	BOOL have_exclusive_srwlock;   /* used for unlock */
> +#endif

[..]

>   	pthread_mutex_t lock;		/* lock for structure		*/
>   	pthread_cond_t	readers;	/* waiting readers		*/
>   	pthread_cond_t	writers;	/* waiting writers		*/
>
> === modified file 'mysys/my_wincond.c'
> --- a/mysys/my_wincond.c	2009-11-23 17:08:37 +0000
> +++ b/mysys/my_wincond.c	2010-09-27 19:38:44 +0000
> @@ -24,7 +24,107 @@
>   #include<process.h>
>   #include<sys/timeb.h>
>
> -int pthread_cond_init(pthread_cond_t *cond, const pthread_condattr_t *attr)
> +
> +/* Windows native condition variables. We use runtime loading / function
> +  pointers, because they are not available on XP */
> +
> +/* Prototypes and function pointers for condition variable functions */
> +typedef VOID (WINAPI * InitializeConditionVariableProc)
> +  (PCONDITION_VARIABLE ConditionVariable);
> +static InitializeConditionVariableProc my_InitializeConditionVariable;

I suggest to separate the typedefs with the variables in order to make 
this easier to read.

> +
> +typedef BOOL (WINAPI * SleepConditionVariableCSProc)
> +  (PCONDITION_VARIABLE ConditionVariable,
> +  PCRITICAL_SECTION CriticalSection,
> +  DWORD dwMilliseconds);
> +static SleepConditionVariableCSProc my_SleepConditionVariableCS;
> +
> +typedef VOID (WINAPI * WakeAllConditionVariableProc)
> + (PCONDITION_VARIABLE ConditionVariable);
> +static WakeAllConditionVariableProc my_WakeAllConditionVariable;
> +
> +typedef VOID (WINAPI * WakeConditionVariableProc)
> +  (PCONDITION_VARIABLE ConditionVariable);
> +static WakeConditionVariableProc my_WakeConditionVariable;
> +
> +
> +/**
> + Indicates if we have native condition variables,
> + initialized first time pthread_cond_init is called.
> +*/
> +static BOOL have_native_conditions= FALSE;
> +
> +/* Old condition implementation (pre-Vista) */
> +static int legacy_cond_init(pthread_cond_t *, const pthread_condattr_t *);
> +static int legacy_cond_destroy(pthread_cond_t *);
> +static int legacy_cond_signal(pthread_cond_t *);
> +static int legacy_cond_broadcast(pthread_cond_t *);
> +static int legacy_cond_timedwait(pthread_cond_t *, pthread_mutex_t *,
> +  struct timespec *);
> +
> +
> +/**
> +  Check if native conditions can be used, load function pointers
> +*/
> +static void check_native_cond_availability(void)
> +{
> +  HMODULE module= GetModuleHandle("kernel32");
> +
> +  my_InitializeConditionVariable = (InitializeConditionVariableProc)
> +    GetProcAddress(module, "InitializeConditionVariable");
> +  my_SleepConditionVariableCS= (SleepConditionVariableCSProc)
> +    GetProcAddress(module, "SleepConditionVariableCS");
> +  my_WakeAllConditionVariable= (WakeAllConditionVariableProc)
> +    GetProcAddress(module, "WakeAllConditionVariable");
> +  my_WakeConditionVariable= (WakeConditionVariableProc)
> +    GetProcAddress(module, "WakeConditionVariable");
> +
> +  if(my_InitializeConditionVariable)

Run through the entire patch: s/if(/if (/

> +    have_native_conditions = TRUE;

Remove space before =.

> +}
> +
> +/**
> +  Convert abstime to milliseconds
> +*/
> +static DWORD get_milliseconds(const struct timespec *abstime)
> +{
> +  long long millis;
> +  union ft64 now;
> +
> +  if(abstime == NULL)
> +   return INFINITE;
> +
> +  GetSystemTimeAsFileTime(&now.ft);
> +
> +  /*
> +    Calculate time left to abstime
> +    - subtract start time from current time(values are in 100ns units)
> +    - convert to millisec by dividing with 10000
> +  */
> +  millis = (abstime->tv.i64 - now.i64) / 10000;
> +
> +  /* Don't allow the timeout to be negative */
> +  if (millis < 0)
> +    return 0;
> +
> +  /*
> +    Make sure the calucated timeout does not exceed original timeout

calucated -> calculated.

[..]

> +/*
> + Posix API functions just chose between native and legacy implementation
> +*/
> +
> +int pthread_cond_init(pthread_cond_t *cond, const pthread_condattr_t *attr)
> +{
> +  /* check if native conditions are available on the first call */
> +  static my_pthread_once_t once_control = MY_PTHREAD_ONCE_INIT;
> +  my_pthread_once(&once_control, check_native_cond_availability);

Why not just check at my_thread_init time? No need for once control.

> +  if(have_native_conditions)
> +  {
> +	my_InitializeConditionVariable(&cond->native_cond);
> +	return 0;

Spacing..

[..]

> +
>   int pthread_attr_init(pthread_attr_t *connect_att)
>   {
>     connect_att->dwStackSize	= 0;
>
> === modified file 'mysys/my_winthread.c'
> --- a/mysys/my_winthread.c	2009-12-19 13:11:48 +0000
> +++ b/mysys/my_winthread.c	2010-09-27 19:38:44 +0000
> @@ -155,6 +155,13 @@ int pthread_cancel(pthread_t thread)
>   int my_pthread_once(my_pthread_once_t *once_control,
>       void (*init_routine)(void))
>   {
> +  /*
> +    Do "dirty" read to find out if initialization is already done, to
> +    save an interlocked operation in common case.
> +  */
> +  if (*once_control == MY_PTHREAD_ONCE_DONE)
> +    return 0;

No need..

>     LONG state= InterlockedCompareExchange(once_control, MY_PTHREAD_ONCE_INPROGRESS,
>                                             MY_PTHREAD_ONCE_INIT);
>     switch(state)
>
> === modified file 'mysys/thr_rwlock.c'
> --- a/mysys/thr_rwlock.c	2010-08-12 13:50:23 +0000
> +++ b/mysys/thr_rwlock.c	2010-09-27 19:38:44 +0000
> @@ -20,6 +20,131 @@
>   #if defined(NEED_MY_RW_LOCK)
>   #include<errno.h>
>

[..]

> +
> +static int srw_tryrdlock(my_rw_lock_t *rwp)
> +{
> +  /* TryAcquire is new in Win7, use if available */
> +  if(my_TryAcquireSRWLockShared)
> +  {
> +    if(!my_TryAcquireSRWLockShared(&rwp->srwlock))
> +      return EBUSY;
> +  }
> +  else
> +  {
> +    /* Try harder, actually acquire the lock */
> +    my_AcquireSRWLockShared(&rwp->srwlock);

No...

There is no user for my_rw_tryrdlock, let's just remove it.

> +  }
> +  DBUG_ASSERT(!rwp->have_exclusive_srwlock);
> +  return 0;
> +}
> +

[..]

> +
> +static int srw_trywrlock(my_rw_lock_t *rwp)
> +{
> +  /* TryAcquire is new in Win7, use it if available */
> +  if(my_TryAcquireSRWLockExclusive)
> +  {
> +    if(!my_TryAcquireSRWLockExclusive(&rwp->srwlock))
> +      return EBUSY;
> +  }
> +  else
> +  {
> +     /* Try harder, actually acquire the lock */
> +     my_AcquireSRWLockExclusive(&rwp->srwlock);

Likewise, no user for my_rw_trywrlock. Let's remove.

Otherwise looks good.
Thread
bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204) Vladislav Vaintroub27 Sep
Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Davi Arnaut1 Oct
Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Dmitry Lenev1 Oct
  • RE: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Vladislav Vaintroub1 Oct
    • Re: bzr commit into mysql-5.5-bugfixing branch (vvaintroub:3204)Dmitry Lenev1 Oct