Improvements to the general readability of the TSD implementation.

2 files changed, 220 insertions, 143 deletions

diff --git a/ChangeLog b/ChangeLog
index be37c31..9e5f936 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,15 +1,22 @@
+2008-11-29  Donal K. Fellows  <dkf@users.sf.net>
+
+	* generic/tclThreadStorage.c: General revisions to make code clearer
+	and more like the style used in the rest of the core. Includes adding
+	more comments and explanation of what is going on.
+
 2008-11-27  Alexandre Ferrieux 	<ferrieux@users.sourceforge.net>
 
-	* generic/tcl.h:        Alternate fix for[Bug 2251175]: missing
-	* generic/tclCompile.c	backslash substitution on expanded literals.
-	* generic/tclParse.c
-	* generic/tclTest.c
-	* tests/parse.test
+	* generic/tcl.h:        Alternate fix for [Bug 2251175]: missing
+	* generic/tclCompile.c:	backslash substitution on expanded literals.
+	* generic/tclParse.c:
+	* generic/tclTest.c:
+	* tests/parse.test:
 
 2008-11-26  Jan Nijtmans  <nijtmans@users.sf.net>
 
 	* generic/tclIndexObj.c:   Eliminate warning: unused variable
-	* generic/tclTest.c:       A few more (harmless) Tcl_SetResult eliminations
+	* generic/tclTest.c:       A few more (harmless) Tcl_SetResult
+				   eliminations.
 
 2008-11-26  Kevin B. Kenny  <kennykb@acm.org>
 
@@ -22,19 +29,20 @@
 
 	* generic/tclIndexObj.c:   Eliminate 3 calls to Tcl_SetResult, as
 	* generic/tclIO.c          examples how it should have been done.
-	* generic/tclTestObj.c     purpose: contribute in the TIP #340 discussion.
+	* generic/tclTestObj.c     purpose: contribute in the TIP #340
+				   discussion.
 
 2008-11-25  Andreas Kupries  <andreask@activestate.com>
 
 	* generic/tclIO.c (TclFinalizeIOSubsystem): Applied Alexandre
-	  Ferrieux's patch for [Bug 2270477] to prevent infinite looping
-	  during finalization of channels not bound to interpreters.
+	Ferrieux's patch for [Bug 2270477] to prevent infinite looping during
+	finalization of channels not bound to interpreters.
 
 2008-11-25  Jan Nijtmans  <nijtmans@users.sf.net>
 
-	* generic/tclTest.c: Don't assume that Tcl_SetResult
-	sets interp->result, especially not in a dstring test,
-	in preparation for TIP #340
+	* generic/tclTest.c: Don't assume that Tcl_SetResult sets
+	interp->result, especially not in a dstring test, in preparation for
+	TIP #340
 
 2008-11-24  Donal K. Fellows  <dkf@users.sf.net>
 
@@ -1048,17 +1056,17 @@
 
 2008-08-14  Daniel Steffen  <das@users.sourceforge.net>
 
-	* generic/tclBasic.c (TclNREvalObjv, Tcl_NRCallObjProc):  DTrace probes
-	* generic/tclProc.c (TclNRInterpProcCore, InterpProcNR2): for NRE.
-								  [Bug 2017160]
+	* generic/tclBasic.c (TclNREvalObjv, Tcl_NRCallObjProc):
+	* generic/tclProc.c (TclNRInterpProcCore, InterpProcNR2):
+	DTrace probes for NRE. [Bug 2017160]
 
 	* generic/tclBasic.c (TclDTraceInfo):	Add two extra arguments to
 	* generic/tclCompile.h:			DTrace 'info' probes for tclOO
 	* generic/tclDTrace.d:			method & class/object info.
 
-	* generic/tclCompile.h:		Add support for debug logging of DTrace
-	* generic/tclBasic.c:		'proc', 'cmd' and 'inst' probes (does
-					_not_ require a platform with DTrace).
+	* generic/tclCompile.h:	Add support for debug logging of DTrace
+	* generic/tclBasic.c:	'proc', 'cmd' and 'inst' probes (does _not_
+				require a platform with DTrace).
 
 	* generic/tclCmdIL.c (TclInfoFrame):	Check fPtr->line before
 						dereferencing as line info may
@@ -1285,11 +1293,11 @@
 
 2008-08-01  Don Porter	<dgp@users.sourceforge.net>
 
-	* generic/tclBasic.c:	Revised timing of the CmdFrame stack management
-	* tests/info.test:	in TclEvalEx so that the CmdFrame will still
-	be on the stack at the time Tcl_LogCommandInfo is called to append
-	another level of -errorinfo information. Sets the stage to add file
-	and line data to the stack trace. Added test to check that [info
+	* generic/tclBasic.c:	Revised timing of the CmdFrame stack
+	* tests/info.test:	management in TclEvalEx so that the CmdFrame
+	will still be on the stack at the time Tcl_LogCommandInfo is called to
+	append another level of -errorinfo information. Sets the stage to add
+	file and line data to the stack trace. Added test to check that [info
 	frame] functioning remains unchanged by the revision.
 
 2008-07-31  Miguel Sofer  <msofer@users.sf.net>
diff --git a/generic/tclThreadStorage.c b/generic/tclThreadStorage.c
index d0fdeec..71fa1e9 100644
--- a/generic/tclThreadStorage.c
+++ b/generic/tclThreadStorage.c
@@ -1,78 +1,117 @@
 /*
  * tclThreadStorage.c --
  *
- *	This file implements platform independent thread storage operations.
+ *	This file implements platform independent thread storage operations to
+ *	work around system limits on the number of thread-specific variables.
  *
  * Copyright (c) 2003-2004 by Joe Mistachkin
  * Copyright (c) 2008 by George Peter Staplin
  *
- * The primary idea is that we create one platform-specific TSD slot, and
- * use it for storing a table pointer.
- *
- * Each Tcl_ThreadDataKey has an offset into the table of TSD values.
- *
- * We don't use more than 1 platform-specific TSD slot, because there is
- * a hard limit on the number of TSD slots.
- *
- * Valid key offsets are > 0.  0 is for the initialized Tcl_ThreadDataKey.
- *
  * 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.16 2008/05/09 04:58:54 georgeps Exp $
+ * RCS: @(#) $Id: tclThreadStorage.c,v 1.17 2008/11/29 12:15:30 dkf Exp $
  */
 
 #include "tclInt.h"
+
+#ifdef TCL_THREADS
 #include <signal.h>
 
-#if defined(TCL_THREADS)
+/*
+ * IMPLEMENTATION NOTES:
+ *
+ * The primary idea is that we create one platform-specific TSD slot, and use
+ * it for storing a table pointer. Each Tcl_ThreadDataKey has an offset into
+ * the table of TSD values. We don't use more than 1 platform-specific TSD
+ * slot, because there is a hard limit on the number of TSD slots. Valid key
+ * offsets are greater than 0; 0 is for the initialized Tcl_ThreadDataKey.
+ */
+
+/*
+ * The master collection of information about TSDs. This is shared across the
+ * whole process, and includes the mutex used to protect it.
+ */
+
+static struct TSDMaster {
+    void *key;			/* Key into the system TSD structure. The
+				 * collection of Tcl TSD values for a
+				 * particular thread will hang off the
+				 * back-end of this. */
+    sig_atomic_t counter;	/* The number of different Tcl TSDs used
+				 * across *all* threads. This is a strictly
+				 * increasing value. */
+    Tcl_Mutex mutex;		/* Protection for the rest of this structure,
+				 * which holds per-process data. */
+} tsdMaster = { NULL, 0 };
 
-static void *tclTsdKey = NULL;
-static Tcl_Mutex tclTsdMutex;
-static sig_atomic_t tclTsdCounter = 0;
+/*
+ * The type of the data held per thread in a system TSD.
+ */
 
 typedef struct TSDTable {
-    sig_atomic_t allocated;
-    void **table;
+    ClientData *tablePtr;	/* The table of Tcl TSDs. */
+    sig_atomic_t allocated;	/* The size of the table in the current
+				 * thread. */
 } TSDTable;
 
-typedef union {
-    void *ptr;
+/*
+ * The actual type of Tcl_ThreadDataKey.
+ */
+
+typedef union TSDUnion {
     volatile sig_atomic_t offset;
+				/* The type is really an offset into the
+				 * thread-local table of TSDs, which is this
+				 * field. */
+    void *ptr;			/* For alignment purposes only. Not actually
+				 * accessed through this. */
 } TSDUnion;
 
-static TSDTable *TSDTableCreate(void);
-static void TSDTableDelete(TSDTable *t);
-static void TSDTableGrow(TSDTable *t, sig_atomic_t atLeast);
+/*
+ * Forward declarations of functions in this file.
+ */
 
+static TSDTable *	TSDTableCreate(void);
+static void		TSDTableDelete(TSDTable *tsdTablePtr);
+static void		TSDTableGrow(TSDTable *tsdTablePtr,
+			    sig_atomic_t atLeast);
 
+/*
+ * Allocator and deallocator for a TSDTable structure.
+ */
+
 static TSDTable *
-TSDTableCreate(void) {
-    TSDTable *t;
+TSDTableCreate(void)
+{
+    TSDTable *tsdTablePtr;
     sig_atomic_t i;
 
-    t = TclpSysAlloc(sizeof *t, 0);
-    if (NULL == t) {
+    tsdTablePtr = TclpSysAlloc(sizeof(TSDTable), 0);
+    if (tsdTablePtr == NULL) {
 	Tcl_Panic("unable to allocate TSDTable");
     }
 
-    t->allocated = 8;
-    t->table = TclpSysAlloc(sizeof (*(t->table)) * t->allocated, 0);
-    if (NULL == t->table) {
+    tsdTablePtr->allocated = 8;
+    tsdTablePtr->tablePtr =
+	    TclpSysAlloc(sizeof(void *) * tsdTablePtr->allocated, 0);
+    if (tsdTablePtr->tablePtr == NULL) {
 	Tcl_Panic("unable to allocate TSDTable");
     }
 
-    for (i = 0; i < t->allocated; ++i) {
-	t->table[i] = NULL;
+    for (i = 0; i < tsdTablePtr->allocated; ++i) {
+	tsdTablePtr->tablePtr[i] = NULL;
     }
 
-    return t;
+    return tsdTablePtr;
 }
-
+
 static void
-TSDTableDelete(TSDTable *t) {
-    TclpSysFree(t->table);
-    TclpSysFree(t);
+TSDTableDelete(
+    TSDTable *tsdTablePtr)
+{
+    TclpSysFree(tsdTablePtr->tablePtr);
+    TclpSysFree(tsdTablePtr);
 }
 
 /*
@@ -80,34 +119,43 @@ TSDTableDelete(TSDTable *t) {
  *
  * TSDTableGrow --
  *
- *    This procedure makes the passed TSDTable grow to fit the atLeast value.
+ *	This procedure makes the passed TSDTable grow to fit the atLeast
+ *	value.
  *
- * Side effects: The table is enlarged.
+ * Results:
+ *	None.
+ *
+ * Side effects:
+ *	The table is enlarged.
  *
  *----------------------------------------------------------------------
  */
+
 static void
-TSDTableGrow(TSDTable *t, sig_atomic_t atLeast) {
-    sig_atomic_t newAllocated = t->allocated * 2;
-    void **newTablePtr;
+TSDTableGrow(
+    TSDTable *tsdTablePtr,
+    sig_atomic_t atLeast)
+{
+    sig_atomic_t newAllocated = tsdTablePtr->allocated * 2;
+    ClientData *newTablePtr;
     sig_atomic_t i;
 
     if (newAllocated <= atLeast) {
 	newAllocated = atLeast + 10;
     }
 
-    newTablePtr = TclpSysRealloc(t->table, sizeof (*newTablePtr) * newAllocated);
-
-    if (NULL == newTablePtr) {
+    newTablePtr = TclpSysRealloc(tsdTablePtr->tablePtr,
+	    sizeof(ClientData) * newAllocated);
+    if (newTablePtr == NULL) {
 	Tcl_Panic("unable to reallocate TSDTable");
     }
-    
-    for (i = t->allocated; i < newAllocated; ++i) {
+
+    for (i = tsdTablePtr->allocated; i < newAllocated; ++i) {
 	newTablePtr[i] = NULL;
     }
-    
-    t->allocated = newAllocated;
-    t->table = newTablePtr;
+
+    tsdTablePtr->allocated = newAllocated;
+    tsdTablePtr->tablePtr = newTablePtr;
 }
 
 /*
@@ -117,25 +165,28 @@ TSDTableGrow(TSDTable *t, sig_atomic_t atLeast) {
  *
  *    This procedure gets the value associated with the passed key.
  *
- *  Results: A pointer value associated with the Tcl_ThreadDataKey or NULL.
+ * Results:
+ *	A pointer value associated with the Tcl_ThreadDataKey or NULL.
+ *
+ * Side effects:
+ *	None.
  *
  *----------------------------------------------------------------------
  */
+
 void *
-TclThreadStorageKeyGet(Tcl_ThreadDataKey *dataKeyPtr) {
-    TSDTable *t = TclpThreadGetMasterTSD(tclTsdKey);
-    void *resultPtr = NULL;
-    TSDUnion *keyPtr = (TSDUnion *)dataKeyPtr;
+TclThreadStorageKeyGet(
+    Tcl_ThreadDataKey *dataKeyPtr)
+{
+    TSDTable *tsdTablePtr = TclpThreadGetMasterTSD(tsdMaster.key);
+    ClientData resultPtr = NULL;
+    TSDUnion *keyPtr = (TSDUnion *) dataKeyPtr;
     sig_atomic_t offset = keyPtr->offset;
-     
-    if (t == NULL) {
-	return NULL;
-    }
 
-    if (offset && offset > 0 && offset < t->allocated) {
-	resultPtr = t->table[offset];
+    if ((tsdTablePtr != NULL) && (offset > 0)
+	    && (offset < tsdTablePtr->allocated)) {
+	resultPtr = tsdTablePtr->tablePtr[offset];
     }
-
     return resultPtr;
 }
 
@@ -144,53 +195,62 @@ TclThreadStorageKeyGet(Tcl_ThreadDataKey *dataKeyPtr) {
  *
  * TclThreadStorageKeySet --
  *
- *     This procedure set an association of value with the key passed.
- *     The associated value may be retrieved with TclThreadDataKeyGet().
+ *	This procedure set an association of value with the key passed. The
+ *	associated value may be retrieved with TclThreadDataKeyGet().
  * 
- *  Side effects: The thread-specific table may be created or reallocated. 
+ * Results:
+ *	None.
+ *
+ * Side effects:
+ *	The thread-specific table may be created or reallocated. 
  *
  *----------------------------------------------------------------------
  */
 
 void
-TclThreadStorageKeySet(Tcl_ThreadDataKey *dataKeyPtr, void *value) {
-    TSDTable *t = TclpThreadGetMasterTSD(tclTsdKey);
-    TSDUnion *keyPtr = (TSDUnion *)dataKeyPtr;
+TclThreadStorageKeySet(
+    Tcl_ThreadDataKey *dataKeyPtr,
+    void *value)
+{
+    TSDTable *tsdTablePtr = TclpThreadGetMasterTSD(tsdMaster.key);
+    TSDUnion *keyPtr = (TSDUnion *) dataKeyPtr;
 
-    if (NULL == t) {
-	t = TSDTableCreate();
-	TclpThreadSetMasterTSD(tclTsdKey, t);
+    if (tsdTablePtr == NULL) {
+	tsdTablePtr = TSDTableCreate();
+	TclpThreadSetMasterTSD(tsdMaster.key, tsdTablePtr);
     }
 
-    Tcl_MutexLock(&tclTsdMutex);
+    /*
+     * Get the lock while we check if this TSD is new or not. Note that this
+     * is the only place where Tcl_ThreadDataKey values are set.
+     */
 
-    if (0 == keyPtr->offset) {
+    Tcl_MutexLock(&tsdMaster.mutex);
+    if (keyPtr->offset == 0) {
 	/* 
-	 * The Tcl_ThreadDataKey hasn't been used yet.
+	 * The Tcl_ThreadDataKey hasn't been used yet. Make a new one.
 	 */
-	++tclTsdCounter;
-
-	if (tclTsdCounter >= t->allocated) {
-	    TSDTableGrow(t, tclTsdCounter);
-	}
-
-	keyPtr->offset = tclTsdCounter;
 
-	t->table[tclTsdCounter] = value;
-    } else {
+	keyPtr->offset = ++tsdMaster.counter;
+    }
+    Tcl_MutexUnlock(&tsdMaster.mutex);
 
-	if (keyPtr->offset >= t->allocated) {
-	    /*
-	     * This is the first time this Tcl_ThreadDataKey has been
-	     * used with the current thread.
-	     */
-	    TSDTableGrow(t, keyPtr->offset);
-	}
+    /*
+     * Check if this is the first time this Tcl_ThreadDataKey has been used
+     * with the current thread. Note that we don't need to hold a lock when
+     * doing this, as we are *definitely* the only point accessing this
+     * tsdTablePtr right now; it's thread-local.
+     */
 
-	t->table[keyPtr->offset] = value;
+    if (keyPtr->offset >= tsdTablePtr->allocated) {
+	TSDTableGrow(tsdTablePtr, keyPtr->offset);
     }
 
-    Tcl_MutexUnlock(&tclTsdMutex);
+    /*
+     * Set the value in the Tcl thread-local variable.
+     */
+
+    tsdTablePtr->tablePtr[keyPtr->offset] = value;
 }
 
 /*
@@ -198,23 +258,26 @@ TclThreadStorageKeySet(Tcl_ThreadDataKey *dataKeyPtr, void *value) {
  *
  * TclFinalizeThreadDataThread --
  *
- *     This procedure finalizes the data for a single thread.
- *  
+ *	This procedure finalizes the data for a single thread.
+ *
+ * Results:
+ *	None.
+ *
+ * Side effects:
+ *	The TSDTable is deleted/freed.
  *
- *  Side effects: The TSDTable is deleted/freed.
  *----------------------------------------------------------------------
  */
+
 void 
-TclFinalizeThreadDataThread(void) {
-    TSDTable *t = TclpThreadGetMasterTSD(tclTsdKey);
+TclFinalizeThreadDataThread(void)
+{
+    TSDTable *tsdTablePtr = TclpThreadGetMasterTSD(tsdMaster.key);
 
-    if (NULL == t) {
-	return;
+    if (tsdTablePtr != NULL) {
+	TSDTableDelete(tsdTablePtr);
+	TclpThreadSetMasterTSD(tsdMaster.key, NULL);
     }
-
-    TSDTableDelete(t);
-    
-    TclpThreadSetMasterTSD(tclTsdKey, NULL);
 }
 
 /*
@@ -222,14 +285,22 @@ TclFinalizeThreadDataThread(void) {
  *
  * TclInitializeThreadStorage --
  *
- *      This procedure initializes the TSD subsystem with per-platform
- *      code.  This should be called before any Tcl threads are created.
+ *	This procedure initializes the TSD subsystem with per-platform code.
+ *	This should be called before any Tcl threads are created.
+ *
+ * Results:
+ *	None.
+ *
+ * Side effects:
+ *	Allocates a system TSD.
  *
  *----------------------------------------------------------------------
  */
+
 void
-TclInitThreadStorage(void) {
-    tclTsdKey = TclpThreadCreateKey();
+TclInitThreadStorage(void)
+{
+    tsdMaster.key = TclpThreadCreateKey();
 }
 
 /*
@@ -237,27 +308,26 @@ TclInitThreadStorage(void) {
  *
  * TclFinalizeThreadStorage --
  *
- *      This procedure cleans up the thread storage data key for all
- *      threads.
- *
- * IMPORTANT: All Tcl threads must be finalized before calling this!
+ *	This procedure cleans up the thread storage data key for all threads.
+ *	IMPORTANT: All Tcl threads must be finalized before calling this!
  *
  * Results:
- *      None.
+ *	None.
  *
  * Side effects:
- *      Releases the thread data key.
+ *	Releases the thread data key.
  *
  *----------------------------------------------------------------------
  */
+
 void
-TclFinalizeThreadStorage(void) {
-    TclpThreadDeleteKey(tclTsdKey);
-    tclTsdKey = NULL;
+TclFinalizeThreadStorage(void)
+{
+    TclpThreadDeleteKey(tsdMaster.key);
+    tsdMaster.key = NULL;
 }
 
-#else /* !defined(TCL_THREADS) */
-
+#else /* !TCL_THREADS */
 /*
  * Stub functions for non-threaded builds
  */
@@ -276,8 +346,7 @@ void
 TclFinalizeThreadStorage(void)
 {
 }
-
-#endif /* defined(TCL_THREADS) && defined(USE_THREAD_STORAGE) */
+#endif /* TCL_THREADS */
 
 /*
  * Local Variables: