From d255c3dda852731b6709ac4e9c9821b3be84ec86 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 14 Nov 2023 20:29:51 -0500 Subject: DRTVWR-588: Try to fix sporadic llrand test failures. With GitHub viewer builds, every few weeks we've seen test failures when ll_frand() returns exactly 1.0. This is a problem for a function that's supposed to return [0.0 .. 1.0). Monty suggests that the problem is likely to be conversion of F32 to F64 to pass to fmod(), and then truncation of fmod()'s F64 result back to F32. Moved the clamping code to each size-specific ll_internal_random specialization. Monty also noted that a stateful static random number engine isn't thread-safe. Added a mutex lock. --- indra/llcommon/llrand.cpp | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llrand.cpp b/indra/llcommon/llrand.cpp index 33afc50cf7..20e25177f0 100644 --- a/indra/llcommon/llrand.cpp +++ b/indra/llcommon/llrand.cpp @@ -28,6 +28,7 @@ #include "llrand.h" #include "lluuid.h" +#include "mutex.h" /** * Through analysis, we have decided that we want to take values which @@ -58,8 +59,17 @@ * to restore uniform distribution. */ +static std::mutex gRandomGeneratorMutex; static LLRandLagFib2281 gRandomGenerator(LLUUID::getRandomSeed()); +inline F64 ll_internal_random_unclamped() +{ + // gRandomGenerator is a stateful static object, which is therefore not + // inherently thread-safe. Lock it before use. + std::unique_lock lk(gRandomGeneratorMutex); + return gRandomGenerator(); +} + // no default implementation, only specific F64 and F32 specializations template inline REAL ll_internal_random(); @@ -71,7 +81,7 @@ inline F64 ll_internal_random() // CPUs (or at least multi-threaded processes) seem to // occasionally give an obviously incorrect random number -- like // 5^15 or something. Sooooo, clamp it as described above. - F64 rv = gRandomGenerator(); + F64 rv{ ll_internal_random_unclamped() }; if(!((rv >= 0.0) && (rv < 1.0))) return fmod(rv, 1.0); return rv; } @@ -79,7 +89,13 @@ inline F64 ll_internal_random() template <> inline F32 ll_internal_random() { - return F32(ll_internal_random()); + // *HACK: clamp the result as described above. + // Per Monty, it's important to clamp using the correct fmodf() rather + // than expanding to F64 for fmod() and then truncating back to F32. Prior + // to this change, we were getting sporadic ll_frand() == 1.0 results. + F32 rv{ ll_internal_random_unclamped() }; + if(!((rv >= 0.0) && (rv < 1.0))) return fmodf(rv, 1.0f); + return rv; } /*------------------------------ F64 aliases -------------------------------*/ -- cgit v1.2.3 From 7670f190827b7d1e1c2a424ec6aa3379cb42ed52 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Nov 2023 10:11:30 -0500 Subject: SL-20546: Rely on CTAD for 'narrow' class. Now that we're building with C++17, we can use Class Template Argument Deduction to infer the type passed to the constructor of the 'narrow' class. We no longer require a narrow_holder class with a narrow() factory function. --- indra/llcommon/stdtypes.h | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/stdtypes.h b/indra/llcommon/stdtypes.h index 0b43d7ad4b..3aba9dda00 100644 --- a/indra/llcommon/stdtypes.h +++ b/indra/llcommon/stdtypes.h @@ -156,18 +156,15 @@ typedef int intptr_t; * type. */ // narrow_holder is a struct that accepts the passed value as its original -// type and provides templated conversion functions to other types. Once we're -// building with compilers that support Class Template Argument Deduction, we -// can rename this class template 'narrow' and eliminate the narrow() factory -// function below. +// type and provides templated conversion functions to other types. template -class narrow_holder +class narrow { private: FROM mValue; public: - narrow_holder(FROM value): mValue(value) {} + narrow(FROM value): mValue(value) {} /*---------------------- Narrowing unsigned to signed ----------------------*/ template (), which can be -/// implicitly converted to the target type. -template -inline -narrow_holder narrow(FROM value) -{ - return { value }; -} - #endif -- cgit v1.2.3 From d427d5dbfa09f0bdec743e75a41e8ea0ee4abeea Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Nov 2023 10:12:12 -0500 Subject: SL-20546: Use narrow() explicit conversion from F64 to F32. --- indra/llcommon/llrand.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra') diff --git a/indra/llcommon/llrand.cpp b/indra/llcommon/llrand.cpp index 20e25177f0..702d6b34c9 100644 --- a/indra/llcommon/llrand.cpp +++ b/indra/llcommon/llrand.cpp @@ -93,7 +93,7 @@ inline F32 ll_internal_random() // Per Monty, it's important to clamp using the correct fmodf() rather // than expanding to F64 for fmod() and then truncating back to F32. Prior // to this change, we were getting sporadic ll_frand() == 1.0 results. - F32 rv{ ll_internal_random_unclamped() }; + F32 rv{ narrow(ll_internal_random_unclamped()) }; if(!((rv >= 0.0) && (rv < 1.0))) return fmodf(rv, 1.0f); return rv; } -- cgit v1.2.3 From 1c71a8e78e37d8605e009d623a5281ab4b509350 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 15 Nov 2023 11:10:42 -0500 Subject: SL-20546: Even with C++17 CTAD, makeClassicCallback() still useful. --- indra/llcommon/classic_callback.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/classic_callback.h b/indra/llcommon/classic_callback.h index 1ad6dbc58f..009c25d67c 100644 --- a/indra/llcommon/classic_callback.h +++ b/indra/llcommon/classic_callback.h @@ -119,11 +119,11 @@ public: * ClassicCallback must not itself be copied or moved! Once you've passed * get_userdata() to some API, this object MUST remain at that address. */ - // However, we can't yet count on C++17 Class Template Argument Deduction, - // which means makeClassicCallback() is still useful, which means we MUST - // be able to return one to construct into caller's instance (move ctor). - // Possible defense: bool 'referenced' data member set by get_userdata(), - // with an llassert_always(! referenced) check in the move constructor. + // However, makeClassicCallback() is useful for deducing the CALLABLE + // type, which means we MUST be able to return one to construct into + // caller's instance (move ctor). Possible defense: bool 'referenced' data + // member set by get_userdata(), with an llassert_always(! referenced) + // check in the move constructor. ClassicCallback(ClassicCallback const&) = delete; ClassicCallback(ClassicCallback&&) = default; // delete; ClassicCallback& operator=(ClassicCallback const&) = delete; -- cgit v1.2.3 From e7ae20c96fccdad06e39a3f8e5fe61a812029242 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 17 Nov 2023 10:24:14 -0500 Subject: SL-20546: Avoid promoting F32 to double just to compare bounds. --- indra/llcommon/llrand.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra') diff --git a/indra/llcommon/llrand.cpp b/indra/llcommon/llrand.cpp index 702d6b34c9..8206bf8e0c 100644 --- a/indra/llcommon/llrand.cpp +++ b/indra/llcommon/llrand.cpp @@ -94,7 +94,7 @@ inline F32 ll_internal_random() // than expanding to F64 for fmod() and then truncating back to F32. Prior // to this change, we were getting sporadic ll_frand() == 1.0 results. F32 rv{ narrow(ll_internal_random_unclamped()) }; - if(!((rv >= 0.0) && (rv < 1.0))) return fmodf(rv, 1.0f); + if(!((rv >= 0.0f) && (rv < 1.0f))) return fmodf(rv, 1.0f); return rv; } -- cgit v1.2.3 From 5fa7f69101a889009194eeddb927599d7536613f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 17 Nov 2023 14:31:21 -0500 Subject: SL-20546: Defend llrand's random generator against concurrent access by making it thread_local. --- indra/llcommon/llrand.cpp | 18 +++++------------- indra/llcommon/llthread.cpp | 13 +++++++------ indra/llcommon/llthread.h | 2 +- 3 files changed, 13 insertions(+), 20 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llrand.cpp b/indra/llcommon/llrand.cpp index 8206bf8e0c..e4065e23bf 100644 --- a/indra/llcommon/llrand.cpp +++ b/indra/llcommon/llrand.cpp @@ -28,7 +28,6 @@ #include "llrand.h" #include "lluuid.h" -#include "mutex.h" /** * Through analysis, we have decided that we want to take values which @@ -59,16 +58,9 @@ * to restore uniform distribution. */ -static std::mutex gRandomGeneratorMutex; -static LLRandLagFib2281 gRandomGenerator(LLUUID::getRandomSeed()); - -inline F64 ll_internal_random_unclamped() -{ - // gRandomGenerator is a stateful static object, which is therefore not - // inherently thread-safe. Lock it before use. - std::unique_lock lk(gRandomGeneratorMutex); - return gRandomGenerator(); -} +// gRandomGenerator is a stateful static object, which is therefore not +// inherently thread-safe. +static thread_local LLRandLagFib2281 gRandomGenerator(LLUUID::getRandomSeed()); // no default implementation, only specific F64 and F32 specializations template @@ -81,7 +73,7 @@ inline F64 ll_internal_random() // CPUs (or at least multi-threaded processes) seem to // occasionally give an obviously incorrect random number -- like // 5^15 or something. Sooooo, clamp it as described above. - F64 rv{ ll_internal_random_unclamped() }; + F64 rv{ gRandomGenerator() }; if(!((rv >= 0.0) && (rv < 1.0))) return fmod(rv, 1.0); return rv; } @@ -93,7 +85,7 @@ inline F32 ll_internal_random() // Per Monty, it's important to clamp using the correct fmodf() rather // than expanding to F64 for fmod() and then truncating back to F32. Prior // to this change, we were getting sporadic ll_frand() == 1.0 results. - F32 rv{ narrow(ll_internal_random_unclamped()) }; + F32 rv{ narrow(gRandomGenerator()) }; if(!((rv >= 0.0f) && (rv < 1.0f))) return fmodf(rv, 1.0f); return rv; } diff --git a/indra/llcommon/llthread.cpp b/indra/llcommon/llthread.cpp index a807acc56e..a051c7f575 100644 --- a/indra/llcommon/llthread.cpp +++ b/indra/llcommon/llthread.cpp @@ -112,15 +112,16 @@ LL_COMMON_API bool on_main_thread() return (LLThread::currentID() == main_thread()); } -LL_COMMON_API void assert_main_thread() +LL_COMMON_API bool assert_main_thread() { auto curr = LLThread::currentID(); auto main = main_thread(); - if (curr != main) - { - LL_WARNS() << "Illegal execution from thread id " << curr - << " outside main thread " << main << LL_ENDL; - } + if (curr == main) + return true; + + LL_WARNS() << "Illegal execution from thread id " << curr + << " outside main thread " << main << LL_ENDL; + return false; } // this function has become moot diff --git a/indra/llcommon/llthread.h b/indra/llcommon/llthread.h index 50202631e7..9f1c589fcd 100644 --- a/indra/llcommon/llthread.h +++ b/indra/llcommon/llthread.h @@ -152,7 +152,7 @@ public: //============================================================================ -extern LL_COMMON_API void assert_main_thread(); +extern LL_COMMON_API bool assert_main_thread(); extern LL_COMMON_API bool on_main_thread(); #endif // LL_LLTHREAD_H -- cgit v1.2.3 From b156dd92957bc5ba123f69781bbe538cdc3a06c7 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 18 Jan 2024 15:38:48 -0500 Subject: SL-20546: Kick the build. --- indra/llmessage/llxfer.cpp | 9 --------- 1 file changed, 9 deletions(-) (limited to 'indra') diff --git a/indra/llmessage/llxfer.cpp b/indra/llmessage/llxfer.cpp index 93d5cfc131..212d0619d1 100644 --- a/indra/llmessage/llxfer.cpp +++ b/indra/llmessage/llxfer.cpp @@ -386,12 +386,3 @@ std::ostream& operator<< (std::ostream& os, LLXfer &hh) os << hh.getFileName() ; return os; } - - - - - - - - - -- cgit v1.2.3