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.
