Added VALGRIND define so we can silence helgrind race-report at places

we know we cheated on in order to reduce contention.

2 files changed, 39 insertions, 7 deletions

diff --git a/ChangeLog b/ChangeLog
index 085f07e..43f9f1d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,10 +1,13 @@
-2010-04-02  Zoran Vasiljevic <vasiljevic@users.sourceforge.net>
+2010-04-03  Zoran Vasiljevic <vasiljevic@users.sourceforge.net>
 
 	* generic/tclStringObj.c: (SetStringFromAny): avoid trampling
 	over the tclEmptyStringRep as it is thread-shared.
 
 	* generic/tclThreadStorage.c (ThreadStorageGetHashTable):
-	avoid accessing shared table index w/o mutex protection.
+	avoid accessing shared table index w/o mutex protection
+	if VALGRIND defined on compilation time. This rules out
+	helgrind complains about potential race-conditions at
+	that place.
 
 	Thanks to Gustaf Neumann for the (hard) work.
 
diff --git a/generic/tclThreadStorage.c b/generic/tclThreadStorage.c
index d24973f..1afc231 100644
--- a/generic/tclThreadStorage.c
+++ b/generic/tclThreadStorage.c
@@ -8,7 +8,7 @@
  * See the file "license.terms" for information on usage and redistribution of
  * this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tclThreadStorage.c,v 1.15.2.1 2010/04/02 15:53:53 vasiljevic Exp $
+ * RCS: @(#) $Id: tclThreadStorage.c,v 1.15.2.2 2010/04/03 09:38:47 vasiljevic Exp $
  */
 
 #include "tclInt.h"
@@ -172,7 +172,7 @@ FreeThreadStorageEntry(
  * ThreadStorageGetHashTable --
  *
  *	This procedure returns a hash table pointer to be used for thread
- *	storage for the specified thread.
+ *	storage for the specified thread. 
  *
  * Results:
  *	A hash table pointer for the specified thread, or NULL if the hash
@@ -182,6 +182,11 @@ FreeThreadStorageEntry(
  *	May change an entry in the master thread storage cache to point to the
  *	specified thread and it's associated hash table.
  *
+ * Thread safety:
+ *	This function assumes that integer operations are safe (atomic)
+ *	on all (currently) supported Tcl platforms.  Hence there are 
+ * 	places where shared integer arithmetic is done w/o protective locks.    
+ *
  *----------------------------------------------------------------------
  */
 
@@ -194,11 +199,31 @@ ThreadStorageGetHashTable(
     int isNew;
     Tcl_HashTable *hashTablePtr;
 
-    Tcl_MutexLock(&threadStorageLock);
+    /*
+     * It's important that we pick up the hash table pointer BEFORE comparing
+     * thread Id in case another thread is in the critical region changing
+     * things out from under you.
+     *
+     * Thread safety: threadStorageCache is accessed w/o locks in order to
+     * avoid serialization of all threads at this hot-spot. It is safe to
+     * do this here because (threadStorageCache[index].id != id) test below
+     * should be atomic on all (currently) supported platforms and there 
+     * are no devastatig side effects of the test.
+     *
+     * Note Valgrind users: this place will show up as a race-condition in
+     * helgrind-tool output. To silence this warnings, define VALGRIND
+     * symbol at compilation time. 
+     */
 
+#if !defined(VALGRIND)
     hashTablePtr = threadStorageCache[index].hashTablePtr;
-
     if (threadStorageCache[index].id != id) {
+	Tcl_MutexLock(&threadStorageLock);
+#else
+    Tcl_MutexLock(&threadStorageLock);
+    hashTablePtr = threadStorageCache[index].hashTablePtr;
+    if (threadStorageCache[index].id != id) {
+#endif
 
 	/*
 	 * It's not in the cache, so we look it up...
@@ -252,9 +277,13 @@ ThreadStorageGetHashTable(
 
 	threadStorageCache[index].id = id;
 	threadStorageCache[index].hashTablePtr = hashTablePtr;
+#if !defined(VALGRIND)
+	Tcl_MutexUnlock(&threadStorageLock);
+    }
+#else
     }
-
     Tcl_MutexUnlock(&threadStorageLock);
+#endif
 
     return hashTablePtr;
 }