[RFT PATCH 1/2] win32: optimize condition variable implementation

[Paolo Bonzini <bonzini@gnu.org>]

The fact that the condition variable mutex must be held
(in this implementation) at the time of pthread_cond_signal
and pthread_cond_broadcast means that most of the time the
waiters_lock is useless.  There is exactly one place where
the mutex is not held, and an atomic decrement suffices
there.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 compat/win32/pthread.c |   54 +++++++++++++++++++++++++----------------------
 compat/win32/pthread.h |    1 -
 2 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 010e875..1a38981 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -61,7 +61,6 @@ int pthread_cond_init(pthread_cond_t *cond, const void *unused)
 {
 	cond->waiters = 0;
 	cond->was_broadcast = 0;
-	InitializeCriticalSection(&cond->waiters_lock);
 
 	cond->sema = CreateSemaphore(NULL, 0, LONG_MAX, NULL);
 	if (!cond->sema)
@@ -81,17 +80,17 @@ int pthread_cond_destroy(pthread_cond_t *cond)
 {
 	CloseHandle(cond->sema);
 	CloseHandle(cond->continue_broadcast);
-	DeleteCriticalSection(&cond->waiters_lock);
 	return 0;
 }
 
 int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
 {
-	int last_waiter;
+	int num_waiters;
 
-	EnterCriticalSection(&cond->waiters_lock);
+	/*
+	 * This access is protected under the mutex.
+	 */
 	cond->waiters++;
-	LeaveCriticalSection(&cond->waiters_lock);
 
 	/*
 	 * Unlock external mutex and wait for signal.
@@ -105,17 +104,17 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
 	WaitForSingleObject(cond->sema, INFINITE);
 
 	/*
-	 * Decrease waiters count. If we are the last waiter, then we must
+	 * Decrease waiters count.  The mutex prevents concurrent increments,
+	 * so doing this decrement atomically is enough.
+	 */
+	num_waiters = InterlockedDecrement(&cond->waiters);
+
+	/* If we are the last waiter, then we must
 	 * notify the broadcasting thread that it can continue.
 	 * But if we continued due to cond_signal, we do not have to do that
 	 * because the signaling thread knows that only one waiter continued.
 	 */
-	EnterCriticalSection(&cond->waiters_lock);
-	cond->waiters--;
-	last_waiter = cond->was_broadcast && cond->waiters == 0;
-	LeaveCriticalSection(&cond->waiters_lock);
-
-	if (last_waiter) {
+	if (num_waiters == 0 && cond->was_broadcast) {
 		/*
 		 * cond_broadcast was issued while mutex was held. This means
 		 * that all other waiters have continued, but are contending
@@ -145,16 +144,17 @@ int pthread_cond_wait(pthread_cond_t *cond, CRITICAL_SECTION *mutex)
  */
 int pthread_cond_signal(pthread_cond_t *cond)
 {
-	int have_waiters;
-
-	EnterCriticalSection(&cond->waiters_lock);
-	have_waiters = cond->waiters > 0;
-	LeaveCriticalSection(&cond->waiters_lock);
-
 	/*
-	 * Signal only when there are waiters
+	 * Signal only when there are waiters.  cond->waiters is
+	 * incremented by pthread_cond_wait under the external lock,
+	 * so we are safe about that.
+	 *
+	 * Waiting threads decrement it outside the external lock, but
+	 * only if another thread is executing pthread_cond_signal or
+	 * pthread_cond_broadcast---which means it also cannot be
+	 * decremented concurrently with this particular access.
 	 */
-	if (have_waiters)
+	if (cond->waiters > 0)
 		return ReleaseSemaphore(cond->sema, 1, NULL) ?
 			0 : err_win_to_posix(GetLastError());
 	else
@@ -168,12 +168,18 @@ int pthread_cond_signal(pthread_cond_t *cond)
  */
 int pthread_cond_broadcast(pthread_cond_t *cond)
 {
-	EnterCriticalSection(&cond->waiters_lock);
+	/*
+	 * As in pthread_cond_signal, access to cond->waiters and
+	 * cond->was_broadcast is locked via the external mutex.
+	 */
 
 	if ((cond->was_broadcast = cond->waiters > 0)) {
+		BOOLEAN result;
 		/* wake up all waiters */
-		ReleaseSemaphore(cond->sema, cond->waiters, NULL);
-		LeaveCriticalSection(&cond->waiters_lock);
+		result = ReleaseSemaphore(cond->sema, cond->waiters, NULL);
+		if (!result)
+			return err_win_to_posix(GetLastError());
+
 		/*
 		 * At this point all waiters continue. Each one takes its
 		 * slice of the semaphor. Now it's our turn to wait: Since
@@ -189,8 +195,6 @@ int pthread_cond_broadcast(pthread_cond_t *cond)
 		 * without cond->waiters_lock held.
 		 */
 		cond->was_broadcast = 0;
-	} else {
-		LeaveCriticalSection(&cond->waiters_lock);
 	}
 	return 0;
 }
diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 2e20548..f38c556 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -40,7 +40,6 @@ typedef int pthread_mutexattr_t;
 typedef struct {
 	LONG waiters;
 	int was_broadcast;
-	CRITICAL_SECTION waiters_lock;
 	HANDLE sema;
 	HANDLE continue_broadcast;
 } pthread_cond_t;
-- 
1.7.0.1