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/llcommon/llrand.cpp') 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 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/llcommon/llrand.cpp') 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 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/llcommon/llrand.cpp') 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 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) (limited to 'indra/llcommon/llrand.cpp') 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; } -- cgit v1.2.3 From a2552a555669490dc2ca173a48989d1b30e62c56 Mon Sep 17 00:00:00 2001 From: Alexander Gavriliuk Date: Thu, 8 Feb 2024 21:03:59 +0100 Subject: Build fix for Visual Studio patch --- indra/llcommon/llrand.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon/llrand.cpp') diff --git a/indra/llcommon/llrand.cpp b/indra/llcommon/llrand.cpp index e4065e23bf..0192111574 100644 --- a/indra/llcommon/llrand.cpp +++ b/indra/llcommon/llrand.cpp @@ -85,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(gRandomGenerator()) }; + F32 rv{ narrow(gRandomGenerator()) }; if(!((rv >= 0.0f) && (rv < 1.0f))) return fmodf(rv, 1.0f); return rv; } -- cgit v1.2.3