Cleanup thread_local_storage on Windows

base/threading/thread_local_storage_win.cc

Index: base/threading/thread_local_storage_win.cc
===================================================================
--- base/threading/thread_local_storage_win.cc	(revision 139374)
+++ base/threading/thread_local_storage_win.cc	(working copy)
@@ -12,6 +12,30 @@
 namespace base {
 
 namespace {

[rvargas (doing something else), 2012/06/01 18:54:36]

We Should move this namespace out of base::. That would follow the usual
pattern.

[jar (doing other things), 2012/06/05 17:45:45]

Done.

+// We use an anonymous namespace here rather than poluting the global header

[rvargas (doing something else), 2012/06/01 18:54:36]

Drop this comment :)

[jar (doing other things), 2012/06/05 17:45:45]

Done.

+// file header for the ThreadLocalStorage class with windows only static methods
+// and static slots.
+
+// Called when we terminate a thread, this function calls any TLS destructors
+// that are pending for this thread.
+void WinThreadExit();
+
+// In order to make TLS destructors work, we need to keep function
+// pointers to the destructor for each TLS that we allocate.
+// We make this work by allocating a single OS-level TLS, which
+// contains an array of slots for the application to use.  In
+// parallel, we also allocate an array of destructors, which we
+// keep track of and call when threads terminate.
+
+// g_windows_native_tls_key is the one native TLS that we use.  It stores our
+// table.
+long g_windows_native_tls_key = TLS_OUT_OF_INDEXES;

[rvargas (doing something else), 2012/06/01 18:54:36]

nit: this sounds a little redundant to me (the windows_native part), given that
it is on the anon namespace of a win specific file :). But of course, keep it if
you feel strong about it.

[jar (doing other things), 2012/06/05 17:45:45]

Done.

+
+// g_last_used_tls_key is the high-water-mark of allocated thread local storage.
+// We intentionally skip 0 (claiming it was used) so that it is not confused
+// with an unallocated TLS slot.

[rvargas (doing something else), 2012/06/01 18:54:36]

Isn't this comment stale? I mean, 0 is not the last one but the next one (and
maybe the name should reflect that), but now the thing about skipping one, and
especially avoiding confusion with an unallocated TLS  is just... confusing.

[jar (doing other things), 2012/06/05 17:45:45]

The comment is (now) perchance more meaningful than it used to be with the other
varible.  We "skip 0" which is why me "claim it was used" and set
g_last_used_tls_key = 0.

0 is *not* the next one.  The next one to use will be 1.  We will never use 0
(but here we act like we did).

On 2012/06/01 18:54:36, rvargas wrote:

[rvargas (doing something else), 2012/06/05 18:25:01]

Well... the comment is still confusing (and I was confused last time I reviewed
the same change, a while ago).

Even if before it was not totally precise, seeing "the code is skipping 0
because it sets the variable to 1" was in agreement with the comment, and then
the reader just moved on (and faced the funky -1 later on).

Now the comment says "we skip 0", but it's hard to see the code doing that. How
will not skip 0, setting the variable to -1? that would be even more weird. What
does "confused with an unallocated TLS slot" means in this context?

So I think the comment needs to say either more, or less than what is saying now
:)

On 2012/06/05 17:45:45, jar wrote:

[jar (doing other things), 2012/06/07 18:23:45]

Done.

+long g_last_used_tls_key = 0;

[jar (doing other things), 2012/05/31 18:32:22]

Note that this is consistently one-less than the old static |tls_max_|, which
held the first unused key (index).

+
 // The maximum number of 'slots' in our thread local storage stack.
 const int kThreadLocalStorageSize = 64;
 
@@ -31,40 +55,23 @@
 volatile ThreadLocalStorage::TLSDestructorFunc
     g_tls_destructors[kThreadLocalStorageSize];
 
-}  // namespace anonymous
-
-// In order to make TLS destructors work, we need to keep function
-// pointers to the destructor for each TLS that we allocate.
-// We make this work by allocating a single OS-level TLS, which
-// contains an array of slots for the application to use.  In
-// parallel, we also allocate an array of destructors, which we
-// keep track of and call when threads terminate.
-
-// tls_key_ is the one native TLS that we use.  It stores our
-// table.
-long ThreadLocalStorage::tls_key_ = TLS_OUT_OF_INDEXES;
-
-// tls_max_ is the high-water-mark of allocated thread local storage.
-// We intentionally skip 0 so that it is not confused with an
-// unallocated TLS slot.
-long ThreadLocalStorage::tls_max_ = 1;
-
-void** ThreadLocalStorage::Initialize() {
-  if (tls_key_ == TLS_OUT_OF_INDEXES) {
+void** ConstructTlsVector() {
+  if (g_windows_native_tls_key == TLS_OUT_OF_INDEXES) {
     long value = TlsAlloc();
     DCHECK(value != TLS_OUT_OF_INDEXES);
 
     // Atomically test-and-set the tls_key.  If the key is TLS_OUT_OF_INDEXES,
     // go ahead and set it.  Otherwise, do nothing, as another
     // thread already did our dirty work.
-    if (InterlockedCompareExchange(&tls_key_, value, TLS_OUT_OF_INDEXES) !=
-            TLS_OUT_OF_INDEXES) {
-      // We've been shortcut. Another thread replaced tls_key_ first so we need
-      // to destroy our index and use the one the other thread got first.
+    if (TLS_OUT_OF_INDEXES != InterlockedCompareExchange(
+            &g_windows_native_tls_key, value, TLS_OUT_OF_INDEXES)) {
+      // We've been shortcut. Another thread replaced g_windows_native_tls_key
+      // first so we need to destroy our index and use the one the other thread
+      // got first.
       TlsFree(value);
     }
   }
-  DCHECK(!TlsGetValue(tls_key_));
+  DCHECK(!TlsGetValue(g_windows_native_tls_key));
 
   // Some allocators, such as TCMalloc, make use of thread local storage.
   // As a result, any attempt to call new (or malloc) will lazily cause such a
@@ -77,15 +84,17 @@
   void* stack_allocated_tls_data[kThreadLocalStorageSize];
   memset(stack_allocated_tls_data, 0, sizeof(stack_allocated_tls_data));
   // Ensure that any rentrant calls change the temp version.
-  TlsSetValue(tls_key_, stack_allocated_tls_data);
+  TlsSetValue(g_windows_native_tls_key, stack_allocated_tls_data);
 
   // Allocate an array to store our data.
   void** tls_data = new void*[kThreadLocalStorageSize];
   memcpy(tls_data, stack_allocated_tls_data, sizeof(stack_allocated_tls_data));
-  TlsSetValue(tls_key_, tls_data);
+  TlsSetValue(g_windows_native_tls_key, tls_data);
   return tls_data;
 }
 
+}  // namespace
+
 ThreadLocalStorage::Slot::Slot(TLSDestructorFunc destructor) {
   initialized_ = false;
   slot_ = 0;
@@ -93,11 +102,12 @@
 }
 
 bool ThreadLocalStorage::StaticSlot::Initialize(TLSDestructorFunc destructor) {
-  if (tls_key_ == TLS_OUT_OF_INDEXES || !TlsGetValue(tls_key_))
-    ThreadLocalStorage::Initialize();
+  if (g_windows_native_tls_key == TLS_OUT_OF_INDEXES ||
+      !TlsGetValue(g_windows_native_tls_key))

[rvargas (doing something else), 2012/06/01 18:54:36]

nit: now we need {}

[jar (doing other things), 2012/06/05 17:45:45]

Name change collapsed line... so we don't need curlies now ;-).

On 2012/06/01 18:54:36, rvargas wrote:

+    ConstructTlsVector();
 
   // Grab a new slot.
-  slot_ = InterlockedIncrement(&tls_max_) - 1;
+  slot_ = InterlockedIncrement(&g_last_used_tls_key);
   DCHECK_GT(slot_, 0);
   if (slot_ >= kThreadLocalStorageSize) {
     NOTREACHED();
@@ -121,28 +131,28 @@
 }
 
 void* ThreadLocalStorage::StaticSlot::Get() const {
-  void** tls_data = static_cast<void**>(TlsGetValue(tls_key_));
+  void** tls_data = static_cast<void**>(TlsGetValue(g_windows_native_tls_key));
   if (!tls_data)
-    tls_data = ThreadLocalStorage::Initialize();
+    tls_data = ConstructTlsVector();
   DCHECK_GT(slot_, 0);
   DCHECK_LT(slot_, kThreadLocalStorageSize);
   return tls_data[slot_];
 }
 
 void ThreadLocalStorage::StaticSlot::Set(void* value) {
-  void** tls_data = static_cast<void**>(TlsGetValue(tls_key_));
+  void** tls_data = static_cast<void**>(TlsGetValue(g_windows_native_tls_key));
   if (!tls_data)
-    tls_data = ThreadLocalStorage::Initialize();
+    tls_data = ConstructTlsVector();
   DCHECK_GT(slot_, 0);
   DCHECK_LT(slot_, kThreadLocalStorageSize);
   tls_data[slot_] = value;
 }
 
-void ThreadLocalStorage::ThreadExit() {
-  if (tls_key_ == TLS_OUT_OF_INDEXES)
+void WinThreadExit() {

[rvargas (doing something else), 2012/06/01 18:54:36]

And this code should move up to the anon namespace (either before check in this
CL or on a follow up CL).

[jar (doing other things), 2012/06/05 17:45:45]

Done.

+  if (g_windows_native_tls_key == TLS_OUT_OF_INDEXES)
     return;
 
-  void** tls_data = static_cast<void**>(TlsGetValue(tls_key_));
+  void** tls_data = static_cast<void**>(TlsGetValue(g_windows_native_tls_key));
   // Maybe we have never initialized TLS for this thread.
   if (!tls_data)
     return;
@@ -159,7 +169,7 @@
   void* stack_allocated_tls_data[kThreadLocalStorageSize];
   memcpy(stack_allocated_tls_data, tls_data, sizeof(stack_allocated_tls_data));
   // Ensure that any re-entrant calls change the temp version.
-  TlsSetValue(tls_key_, stack_allocated_tls_data);
+  TlsSetValue(g_windows_native_tls_key, stack_allocated_tls_data);
   delete[] tls_data;  // Our last dependence on an allocator.
 
   int remaining_attempts = kMaxDestructorIterations;
@@ -172,11 +182,12 @@
     // allocator) and should also be destroyed last.  If we get the order wrong,
     // then we'll itterate several more times, so it is really not that
     // critical (but it might help).
-    for (int slot = tls_max_ - 1; slot > 0; --slot) {
+    for (int slot = g_last_used_tls_key; slot > 0; --slot) {
       void* value = stack_allocated_tls_data[slot];
       if (value == NULL)
         continue;
-      TLSDestructorFunc destructor = g_tls_destructors[slot];
+      ThreadLocalStorage::TLSDestructorFunc destructor =
+          g_tls_destructors[slot];
       if (destructor == NULL)
         continue;
       stack_allocated_tls_data[slot] = NULL;  // pre-clear the slot.
@@ -193,7 +204,7 @@
   }
 
   // Remove our stack allocated vector.
-  TlsSetValue(tls_key_, NULL);
+  TlsSetValue(g_windows_native_tls_key, NULL);
 }
 
 }  // namespace base
@@ -226,7 +237,7 @@
   // On XP SP0 & SP1, the DLL_PROCESS_ATTACH is never seen. It is sent on SP2+
   // and on W2K and W2K3. So don't assume it is sent.
   if (DLL_THREAD_DETACH == reason || DLL_PROCESS_DETACH == reason)
-    base::ThreadLocalStorage::ThreadExit();
+    base::WinThreadExit();

[rvargas (doing something else), 2012/06/01 18:54:36]

... and this would become just WinThreadExit();

[jar (doing other things), 2012/06/05 17:45:45]

Done.

 }
 
 // .CRT$XLA to .CRT$XLZ is an array of PIMAGE_TLS_CALLBACK pointers that are