From 5e7df752a66b2082d063d2c4a10bc7013d479f55 Mon Sep 17 00:00:00 2001
From: Nat Goodspeed <nat@lindenlab.com>
Date: Fri, 6 Dec 2019 16:31:49 -0500
Subject: [PATCH] DRTVWR-494: Use std::thread::id for LLThread::currentID().
LLThread::currentID() used to return a U32, a distinct unsigned value
incremented by explicitly constructing LLThread or by calling LLThread::
registerThreadID() early in a thread launched by other means. The latter
imposed an unobvious requirement on new code based on std::thread. Using
std::thread::id instead delegates to the compiler/library the problem of
distinguishing threads launched by any means.
Change lots of explicit U32 declarations. Introduce LLThread::id_t typedef to
avoid having to run around fixing uses again if we later revisit this decision.
LLMutex, which stores an LLThread::id_t, wants a distinguished value meaning
NO_THREAD, and had an enum with that name. But as std::thread::id promises
that the default-constructed value is distinct from every valid value,
NO_THREAD becomes unnecessary and goes away.
Because LLMutex now stores LLThread::id_t instead of U32, make llmutex.h
#include "llthread.h" instead of the other way around. This makes LLMutex an
incomplete type within llthread.h, so move LLThread::lockData() and
unlockData() to the .cpp file. Similarly, remove llrefcount.h's #include
"llmutex.h" to break circularity; instead forward-declare LLMutex.
It turns out that a number of source files assumed that #include "llthread.h"
would get the definition for LLMutex. Sprinkle #include "llmutex.h" as needed.
In the SAFE_SSL code in llcorehttp/httpcommon.cpp, there's an ssl_thread_id()
callback that returns an unsigned long to the SSL library. When LLThread::
currentID() was U32, we could simply return that. But std::thread::id is very
deliberately opaque, and can't be reinterpret_cast to unsigned long.
Fortunately it can be hashed because std::hash is specialized with that type.
---
indra/llcommon/llmutex.cpp | 13 +++++-----
indra/llcommon/llmutex.h | 14 ++++-------
indra/llcommon/llrefcount.h | 3 ++-
indra/llcommon/llthread.cpp | 36 +++++++++++++++-------------
indra/llcommon/llthread.h | 24 +++++--------------
indra/llcommon/lluuid.cpp | 3 ++-
indra/llcommon/llworkerthread.h | 1 +
indra/llcorehttp/httpcommon.cpp | 4 +++-
indra/llmessage/llbuffer.cpp | 1 +
indra/llmessage/llbufferstream.cpp | 1 +
indra/llmessage/llproxy.h | 1 +
indra/llplugin/llpluginmessagepipe.h | 1 +
indra/llvfs/llvfs.h | 1 +
13 files changed, 49 insertions(+), 54 deletions(-)
diff --git a/indra/llcommon/llmutex.cpp b/indra/llcommon/llmutex.cpp
index 75f43a47042..4d73c04d076 100644
--- a/indra/llcommon/llmutex.cpp
+++ b/indra/llcommon/llmutex.cpp
@@ -32,8 +32,7 @@
//============================================================================
LLMutex::LLMutex() :
- mCount(0),
- mLockingThread(NO_THREAD)
+ mCount(0)
{
}
@@ -55,7 +54,7 @@ void LLMutex::lock()
#if MUTEX_DEBUG
// Have to have the lock before we can access the debug info
- U32 id = LLThread::currentID();
+ auto id = LLThread::currentID();
if (mIsLocked[id] != FALSE)
LL_ERRS() << "Already locked in Thread: " << id << LL_ENDL;
mIsLocked[id] = TRUE;
@@ -74,13 +73,13 @@ void LLMutex::unlock()
#if MUTEX_DEBUG
// Access the debug info while we have the lock
- U32 id = LLThread::currentID();
+ auto id = LLThread::currentID();
if (mIsLocked[id] != TRUE)
LL_ERRS() << "Not locked in Thread: " << id << LL_ENDL;
mIsLocked[id] = FALSE;
#endif
- mLockingThread = NO_THREAD;
+ mLockingThread = LLThread::id_t();
mMutex.unlock();
}
@@ -102,7 +101,7 @@ bool LLMutex::isSelfLocked()
return mLockingThread == LLThread::currentID();
}
-U32 LLMutex::lockingThread() const
+LLThread::id_t LLMutex::lockingThread() const
{
return mLockingThread;
}
@@ -122,7 +121,7 @@ bool LLMutex::trylock()
#if MUTEX_DEBUG
// Have to have the lock before we can access the debug info
- U32 id = LLThread::currentID();
+ auto id = LLThread::currentID();
if (mIsLocked[id] != FALSE)
LL_ERRS() << "Already locked in Thread: " << id << LL_ENDL;
mIsLocked[id] = TRUE;
diff --git a/indra/llcommon/llmutex.h b/indra/llcommon/llmutex.h
index 1a93c048b64..838d7d34c06 100644
--- a/indra/llcommon/llmutex.h
+++ b/indra/llcommon/llmutex.h
@@ -28,6 +28,7 @@
#define LL_LLMUTEX_H
#include "stdtypes.h"
+#include "llthread.h"
#include <boost/noncopyable.hpp>
#include "mutex.h"
@@ -44,11 +45,6 @@
class LL_COMMON_API LLMutex
{
public:
- typedef enum
- {
- NO_THREAD = 0xFFFFFFFF
- } e_locking_thread;
-
LLMutex();
virtual ~LLMutex();
@@ -57,15 +53,15 @@ class LL_COMMON_API LLMutex
void unlock(); // undefined behavior when called on mutex not being held
bool isLocked(); // non-blocking, but does do a lock/unlock so not free
bool isSelfLocked(); //return true if locked in a same thread
- U32 lockingThread() const; //get ID of locking thread
-
+ LLThread::id_t lockingThread() const; //get ID of locking thread
+
protected:
std::mutex mMutex;
mutable U32 mCount;
- mutable U32 mLockingThread;
+ mutable LLThread::id_t mLockingThread;
#if MUTEX_DEBUG
- std::map<U32, BOOL> mIsLocked;
+ std::map<LLThread::id_t, BOOL> mIsLocked;
#endif
};
diff --git a/indra/llcommon/llrefcount.h b/indra/llcommon/llrefcount.h
index fb0411d27bf..7e4af6ea66a 100644
--- a/indra/llcommon/llrefcount.h
+++ b/indra/llcommon/llrefcount.h
@@ -28,9 +28,10 @@
#include <boost/noncopyable.hpp>
#include <boost/intrusive_ptr.hpp>
-#include "llmutex.h"
#include "llatomic.h"
+class LLMutex;
+
//----------------------------------------------------------------------------
// RefCount objects should generally only be accessed by way of LLPointer<>'s
// see llthread.h for LLThreadSafeRefCount
diff --git a/indra/llcommon/llthread.cpp b/indra/llcommon/llthread.cpp
index f875e4e0dc3..0b9dec969ce 100644
--- a/indra/llcommon/llthread.cpp
+++ b/indra/llcommon/llthread.cpp
@@ -92,21 +92,16 @@ void set_thread_name( DWORD dwThreadID, const char* threadName)
// }
//
//----------------------------------------------------------------------------
-
-U32 LL_THREAD_LOCAL sThreadID = 0;
-
-U32 LLThread::sIDIter = 0;
-
namespace
{
- U32 main_thread()
+ LLThread::id_t main_thread()
{
// Using a function-static variable to identify the main thread
// requires that control reach here from the main thread before it
// reaches here from any other thread. We simply trust that whichever
// thread gets here first is the main thread.
- static U32 s_thread_id = LLThread::currentID();
+ static LLThread::id_t s_thread_id = LLThread::currentID();
return s_thread_id;
}
@@ -128,10 +123,8 @@ LL_COMMON_API void assert_main_thread()
}
}
-void LLThread::registerThreadID()
-{
- sThreadID = ++sIDIter;
-}
+// this function has become moot
+void LLThread::registerThreadID() {}
//
// Handed to the APR thread creation function
@@ -142,11 +135,12 @@ void LLThread::threadRun()
set_thread_name(-1, mName.c_str());
#endif
+ // this is the first point at which we're actually running in the new thread
+ mID = currentID();
+
// for now, hard code all LLThreads to report to single master thread recorder, which is known to be running on main thread
mRecorder = new LLTrace::ThreadRecorder(*LLTrace::get_master_thread_recorder());
- sThreadID = mID;
-
// Run the user supplied function
do
{
@@ -188,8 +182,6 @@ LLThread::LLThread(const std::string& name, apr_pool_t *poolp) :
mStatus(STOPPED),
mRecorder(NULL)
{
-
- mID = ++sIDIter;
mRunCondition = new LLCondition();
mDataLock = new LLMutex();
mLocalAPRFilePoolp = NULL ;
@@ -367,9 +359,9 @@ void LLThread::setQuitting()
}
// static
-U32 LLThread::currentID()
+LLThread::id_t LLThread::currentID()
{
- return sThreadID;
+ return std::this_thread::get_id();
}
// static
@@ -396,6 +388,16 @@ void LLThread::wakeLocked()
}
}
+void LLThread::lockData()
+{
+ mDataLock->lock();
+}
+
+void LLThread::unlockData()
+{
+ mDataLock->unlock();
+}
+
//============================================================================
//----------------------------------------------------------------------------
diff --git a/indra/llcommon/llthread.h b/indra/llcommon/llthread.h
index 37f6e66bbb8..5cd0731f6c9 100644
--- a/indra/llcommon/llthread.h
+++ b/indra/llcommon/llthread.h
@@ -30,7 +30,6 @@
#include "llapp.h"
#include "llapr.h"
#include "boost/intrusive_ptr.hpp"
-#include "llmutex.h"
#include "llrefcount.h"
#include <thread>
@@ -43,7 +42,6 @@ class LL_COMMON_API LLThread
{
private:
friend class LLMutex;
- static U32 sIDIter;
public:
typedef enum e_thread_status
@@ -53,6 +51,7 @@ class LL_COMMON_API LLThread
QUITTING= 2, // Someone wants this thread to quit
CRASHED = -1 // An uncaught exception was thrown by the thread
} EThreadStatus;
+ typedef std::thread::id id_t;
LLThread(const std::string& name, apr_pool_t *poolp = NULL);
virtual ~LLThread(); // Warning! You almost NEVER want to destroy a thread unless it's in the STOPPED state.
@@ -62,7 +61,7 @@ class LL_COMMON_API LLThread
bool isStopped() const { return (STOPPED == mStatus) || (CRASHED == mStatus); }
bool isCrashed() const { return (CRASHED == mStatus); }
- static U32 currentID(); // Return ID of current thread
+ static id_t currentID(); // Return ID of current thread
static void yield(); // Static because it can be called by the main thread, which doesn't have an LLThread data structure.
public:
@@ -86,7 +85,7 @@ class LL_COMMON_API LLThread
LLVolatileAPRPool* getLocalAPRFilePool() { return mLocalAPRFilePoolp ; }
- U32 getID() const { return mID; }
+ id_t getID() const { return mID; }
// Called by threads *not* created via LLThread to register some
// internal state used by LLMutex. You must call this once early
@@ -107,7 +106,7 @@ class LL_COMMON_API LLThread
std::thread *mThreadp;
EThreadStatus mStatus;
- U32 mID;
+ id_t mID;
LLTrace::ThreadRecorder* mRecorder;
//a local apr_pool for APRFile operations in this thread. If it exists, LLAPRFile::sAPRFilePoolp should not be used.
@@ -124,8 +123,8 @@ class LL_COMMON_API LLThread
virtual bool runCondition(void);
// Lock/Unlock Run Condition -- use around modification of any variable used in runCondition()
- inline void lockData();
- inline void unlockData();
+ void lockData();
+ void unlockData();
// This is the predicate that decides whether the thread should sleep.
// It should only be called with mDataLock locked, since the virtual runCondition() function may need to access
@@ -140,17 +139,6 @@ class LL_COMMON_API LLThread
};
-void LLThread::lockData()
-{
- mDataLock->lock();
-}
-
-void LLThread::unlockData()
-{
- mDataLock->unlock();
-}
-
-
//============================================================================
// Simple responder for self destructing callbacks
diff --git a/indra/llcommon/lluuid.cpp b/indra/llcommon/lluuid.cpp
index 8f33d789ebd..b05630c6b59 100644
--- a/indra/llcommon/lluuid.cpp
+++ b/indra/llcommon/lluuid.cpp
@@ -43,6 +43,7 @@
#include "llstring.h"
#include "lltimer.h"
#include "llthread.h"
+#include "llmutex.h"
const LLUUID LLUUID::null;
const LLTransactionID LLTransactionID::tnull;
@@ -738,7 +739,7 @@ void LLUUID::getCurrentTime(uuid_time_t *timestamp)
getSystemTime(&time_last);
uuids_this_tick = uuids_per_tick;
init = TRUE;
- mMutex = new LLMutex();
+ mMutex = new LLMutex();
}
uuid_time_t time_now = {0,0};
diff --git a/indra/llcommon/llworkerthread.h b/indra/llcommon/llworkerthread.h
index b1a6f613607..0387e75c653 100644
--- a/indra/llcommon/llworkerthread.h
+++ b/indra/llcommon/llworkerthread.h
@@ -34,6 +34,7 @@
#include "llqueuedthread.h"
#include "llatomic.h"
+#include "llmutex.h"
#define USE_FRAME_CALLBACK_MANAGER 0
diff --git a/indra/llcorehttp/httpcommon.cpp b/indra/llcorehttp/httpcommon.cpp
index 7c93c54cdfc..e37a38b05f8 100644
--- a/indra/llcorehttp/httpcommon.cpp
+++ b/indra/llcorehttp/httpcommon.cpp
@@ -40,6 +40,7 @@
#include <sstream>
#if SAFE_SSL
#include <openssl/crypto.h>
+#include <functional> // std::hash
#endif
@@ -369,7 +370,8 @@ void ssl_locking_callback(int mode, int type, const char *file, int line)
//static
unsigned long ssl_thread_id(void)
{
- return LLThread::currentID();
+ // std::thread::id is very deliberately opaque, but we can hash it
+ return std::hash<LLThread::id_t>()(LLThread::currentID());
}
#endif
diff --git a/indra/llmessage/llbuffer.cpp b/indra/llmessage/llbuffer.cpp
index 1a0eceba0f8..cfe38605ad5 100644
--- a/indra/llmessage/llbuffer.cpp
+++ b/indra/llmessage/llbuffer.cpp
@@ -32,6 +32,7 @@
#include "llmath.h"
#include "llstl.h"
#include "llthread.h"
+#include "llmutex.h"
#include <iterator>
#define ASSERT_LLBUFFERARRAY_MUTEX_LOCKED() llassert(!mMutexp || mMutexp->isSelfLocked())
diff --git a/indra/llmessage/llbufferstream.cpp b/indra/llmessage/llbufferstream.cpp
index ff1c9993cc0..39508c1c52e 100644
--- a/indra/llmessage/llbufferstream.cpp
+++ b/indra/llmessage/llbufferstream.cpp
@@ -31,6 +31,7 @@
#include "llbuffer.h"
#include "llthread.h"
+#include "llmutex.h"
static const S32 DEFAULT_OUTPUT_SEGMENT_SIZE = 1024 * 4;
diff --git a/indra/llmessage/llproxy.h b/indra/llmessage/llproxy.h
index 87891901ad6..a1ffa9e5d56 100644
--- a/indra/llmessage/llproxy.h
+++ b/indra/llmessage/llproxy.h
@@ -32,6 +32,7 @@
#include "llmemory.h"
#include "llsingleton.h"
#include "llthread.h"
+#include "llmutex.h"
#include <curl/curl.h>
#include <string>
diff --git a/indra/llplugin/llpluginmessagepipe.h b/indra/llplugin/llpluginmessagepipe.h
index c3498beac04..9d5835eb828 100644
--- a/indra/llplugin/llpluginmessagepipe.h
+++ b/indra/llplugin/llpluginmessagepipe.h
@@ -31,6 +31,7 @@
#include "lliosocket.h"
#include "llthread.h"
+#include "llmutex.h"
class LLPluginMessagePipe;
diff --git a/indra/llvfs/llvfs.h b/indra/llvfs/llvfs.h
index dca5ff4ad5f..42feafe20b8 100644
--- a/indra/llvfs/llvfs.h
+++ b/indra/llvfs/llvfs.h
@@ -31,6 +31,7 @@
#include "lluuid.h"
#include "llassettype.h"
#include "llthread.h"
+#include "llmutex.h"
enum EVFSValid
{
--
GitLab