From 576c558759aa84df7b30ee29ca55143719d73028 Mon Sep 17 00:00:00 2001 From: Andrey Lihatskiy Date: Tue, 17 Sep 2024 20:03:03 +0300 Subject: Xcode16 build fix --- indra/llcommon/llqueuedthread.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llqueuedthread.cpp b/indra/llcommon/llqueuedthread.cpp index 1c4ac5a7bf..0196a24b18 100644 --- a/indra/llcommon/llqueuedthread.cpp +++ b/indra/llcommon/llqueuedthread.cpp @@ -146,7 +146,7 @@ size_t LLQueuedThread::updateQueue(F32 max_time_ms) // schedule a call to threadedUpdate for every call to updateQueue if (!isQuitting()) { - mRequestQueue.post([=]() + mRequestQueue.post([=, this]() { LL_PROFILE_ZONE_NAMED_CATEGORY_THREAD("qt - update"); mIdleThread = false; @@ -474,7 +474,7 @@ void LLQueuedThread::processRequest(LLQueuedThread::QueuedRequest* req) #else using namespace std::chrono_literals; auto retry_time = LL::WorkQueue::TimePoint::clock::now() + 16ms; - mRequestQueue.post([=] + mRequestQueue.post([=, this] { LL_PROFILE_ZONE_NAMED("processRequest - retry"); if (LL::WorkQueue::TimePoint::clock::now() < retry_time) -- cgit v1.2.3 From 705fd68ed7cdc6db39ddd237b7f2c70045fc01fb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 24 Sep 2024 12:53:33 -0400 Subject: Make `LLPointer` equality comparisons accept not-identical types. That is, both `LLPointer::operator==()` and `operator!=()` and the free function `operator==()` and `operator!=()` now accept pointer types other than the type of the subject `LLPointer`, letting the compiler apply implicit pointer conversions or diagnose an error. --- indra/llcommon/llpointer.h | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index 048547e4cc..a886d6538e 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -105,11 +105,16 @@ public: bool notNull() const { return (mPointer != nullptr); } operator Type*() const { return mPointer; } - bool operator !=(Type* ptr) const { return (mPointer != ptr); } - bool operator ==(Type* ptr) const { return (mPointer == ptr); } - bool operator ==(const LLPointer& ptr) const { return (mPointer == ptr.mPointer); } - bool operator < (const LLPointer& ptr) const { return (mPointer < ptr.mPointer); } - bool operator > (const LLPointer& ptr) const { return (mPointer > ptr.mPointer); } + template + bool operator !=(Type1* ptr) const { return (mPointer != ptr); } + template + bool operator ==(Type1* ptr) const { return (mPointer == ptr); } + template + bool operator !=(const LLPointer& ptr) const { return (mPointer != ptr.mPointer); } + template + bool operator ==(const LLPointer& ptr) const { return (mPointer == ptr.mPointer); } + bool operator < (const LLPointer& ptr) const { return (mPointer < ptr.mPointer); } + bool operator > (const LLPointer& ptr) const { return (mPointer > ptr.mPointer); } LLPointer& operator =(Type* ptr) { @@ -418,14 +423,14 @@ private: bool mStayUnique; }; -template -bool operator!=(Type* lhs, const LLPointer& rhs) +template +bool operator!=(Type0* lhs, const LLPointer& rhs) { return (lhs != rhs.get()); } -template -bool operator==(Type* lhs, const LLPointer& rhs) +template +bool operator==(Type0* lhs, const LLPointer& rhs) { return (lhs == rhs.get()); } -- cgit v1.2.3 From 1a682de4e42dcea932d25ffe980a2dfec345211e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 24 Sep 2024 13:41:02 -0400 Subject: LLConstPointer is the same as LLPointer. Instead of restating the whole class, requiring changes to be made in parallel (which has already failed), just make a template alias. --- indra/llcommon/llpointer.h | 166 +-------------------------------------------- 1 file changed, 2 insertions(+), 164 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index a886d6538e..bbe3595435 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -210,170 +210,8 @@ protected: Type* mPointer; }; -template class LLConstPointer -{ - template - friend class LLConstPointer; -public: - LLConstPointer() : - mPointer(nullptr) - { - } - - LLConstPointer(const Type* ptr) : - mPointer(ptr) - { - ref(); - } - - LLConstPointer(const LLConstPointer& ptr) : - mPointer(ptr.mPointer) - { - ref(); - } - - LLConstPointer(LLConstPointer&& ptr) noexcept - { - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } - - // support conversion up the type hierarchy. See Item 45 in Effective C++, 3rd Ed. - template - LLConstPointer(const LLConstPointer& ptr) : - mPointer(ptr.get()) - { - ref(); - } - - template - LLConstPointer(LLConstPointer&& ptr) noexcept : - mPointer(ptr.get()) - { - ptr.mPointer = nullptr; - } - - ~LLConstPointer() - { - unref(); - } - - const Type* get() const { return mPointer; } - const Type* operator->() const { return mPointer; } - const Type& operator*() const { return *mPointer; } - - operator BOOL() const { return (mPointer != nullptr); } - operator bool() const { return (mPointer != nullptr); } - bool operator!() const { return (mPointer == nullptr); } - bool isNull() const { return (mPointer == nullptr); } - bool notNull() const { return (mPointer != nullptr); } - - operator const Type*() const { return mPointer; } - bool operator !=(const Type* ptr) const { return (mPointer != ptr); } - bool operator ==(const Type* ptr) const { return (mPointer == ptr); } - bool operator ==(const LLConstPointer& ptr) const { return (mPointer == ptr.mPointer); } - bool operator < (const LLConstPointer& ptr) const { return (mPointer < ptr.mPointer); } - bool operator > (const LLConstPointer& ptr) const { return (mPointer > ptr.mPointer); } - - LLConstPointer& operator =(const Type* ptr) - { - if( mPointer != ptr ) - { - unref(); - mPointer = ptr; - ref(); - } - - return *this; - } - - LLConstPointer& operator =(const LLConstPointer& ptr) - { - if( mPointer != ptr.mPointer ) - { - unref(); - mPointer = ptr.mPointer; - ref(); - } - return *this; - } - - LLConstPointer& operator =(LLConstPointer&& ptr) - { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } - return *this; - } - - // support assignment up the type hierarchy. See Item 45 in Effective C++, 3rd Ed. - template - LLConstPointer& operator =(const LLConstPointer& ptr) - { - if( mPointer != ptr.get() ) - { - unref(); - mPointer = ptr.get(); - ref(); - } - return *this; - } - - template - LLConstPointer& operator =(LLConstPointer&& ptr) - { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } - return *this; - } - - // Just exchange the pointers, which will not change the reference counts. - static void swap(LLConstPointer& a, LLConstPointer& b) - { - const Type* temp = a.mPointer; - a.mPointer = b.mPointer; - b.mPointer = temp; - } - -protected: -#ifdef LL_LIBRARY_INCLUDE - void ref(); - void unref(); -#else // LL_LIBRARY_INCLUDE - void ref() - { - if (mPointer) - { - mPointer->ref(); - } - } - - void unref() - { - if (mPointer) - { - const Type *temp = mPointer; - mPointer = nullptr; - temp->unref(); - if (mPointer != nullptr) - { - LL_WARNS() << "Unreference did assignment to non-NULL because of destructor" << LL_ENDL; - unref(); - } - } - } -#endif // LL_LIBRARY_INCLUDE - -protected: - const Type* mPointer; -}; +template +using LLConstPointer = LLPointer; template class LLCopyOnWritePointer : public LLPointer -- cgit v1.2.3 From 98c829a47cd2b38d51602cce3de873179a394ba7 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 25 Sep 2024 09:43:43 -0400 Subject: Use copy-and-swap idiom for LLPointer's assignment operators. This affords strong exception safety. Also, eliminating the conditional may improve speed. --- indra/llcommon/llpointer.h | 53 ++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 28 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index bbe3595435..c5dd5532cc 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -28,6 +28,7 @@ #include "llerror.h" // *TODO: consider eliminating this #include "llmutex.h" +#include // std::swap() //---------------------------------------------------------------------------- // RefCount objects should generally only be accessed by way of LLPointer<>'s @@ -118,24 +119,26 @@ public: LLPointer& operator =(Type* ptr) { - assign(ptr); + // copy-and-swap idiom, see http://gotw.ca/gotw/059.htm + LLPointer temp(ptr); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } LLPointer& operator =(const LLPointer& ptr) { - assign(ptr); + LLPointer temp(ptr); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } LLPointer& operator =(LLPointer&& ptr) { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } + LLPointer temp(std::move(ptr)); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } @@ -143,28 +146,32 @@ public: template LLPointer& operator =(const LLPointer& ptr) { - assign(ptr.get()); + LLPointer temp(ptr); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } template LLPointer& operator =(LLPointer&& ptr) { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } + LLPointer temp(std::move(ptr)); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } // Just exchange the pointers, which will not change the reference counts. static void swap(LLPointer& a, LLPointer& b) { - Type* temp = a.mPointer; - a.mPointer = b.mPointer; - b.mPointer = temp; + using std::swap; // per Swappable convention + swap(a.mPointer, b.mPointer); + } + + // Put swap() overload in the global namespace, per Swappable convention + friend void swap(LLPointer& a, LLPointer& b) + { + LLPointer::swap(a, b); } protected: @@ -196,16 +203,6 @@ protected: } #endif // LL_LIBRARY_INCLUDE - void assign(const LLPointer& ptr) - { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ref(); - } - } - protected: Type* mPointer; }; -- cgit v1.2.3 From 5d7b3abe7759545b5ff12dc8ae5a6de9ed258953 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 25 Sep 2024 10:15:20 -0400 Subject: Explain why apparently redundant LLPointer methods are necessary. Given templated constructors and assignment operators, it's tempting to remove specific constructors and assignment operators with the same LLPointer parameters. That turns out to be a mistake. Add comments to warn future maintainers. --- indra/llcommon/llpointer.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index c5dd5532cc..71c955c4c5 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -61,6 +61,13 @@ public: ref(); } + // Even though the template constructors below accepting + // (const LLPointer&) and (LLPointer&&) appear to + // subsume these specific (const LLPointer&) and (LLPointer&&) + // constructors, the compiler recognizes these as The Copy Constructor and + // The Move Constructor, respectively. In other words, even in the + // presence of the LLPointer constructors, we still must specify + // the LLPointer constructors. LLPointer(const LLPointer& ptr) : mPointer(ptr.mPointer) { @@ -126,6 +133,13 @@ public: return *this; } + // Even though the template assignment operators below accepting + // (const LLPointer&) and (LLPointer&&) appear to + // subsume these specific (const LLPointer&) and (LLPointer&&) + // assignment operators, the compiler recognizes these as Copy Assignment + // and Move Assignment, respectively. In other words, even in the presence + // of the LLPointer assignment operators, we still must specify + // the LLPointer operators. LLPointer& operator =(const LLPointer& ptr) { LLPointer temp(ptr); -- cgit v1.2.3 From dc0c6d396eb70392f294eea65975646dad662f6a Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 25 Sep 2024 21:38:46 -0400 Subject: Adapt `fsyspath` for C++20 conventions. In C++20, `std::filesystem::u8path()` (that accepts a UTF-8 encoded `std::string` and returns a `std::filesystem::path`) is deprecated. Instead, to engage UTF-8 coding conversions, we're supposed to pass the `path` constructor a `std::u8string`, i.e. a `std::basic_string`. Since `char8_t` is a type distinct from both `char` and `unsigned char`, we must Do Something to pass a UTF-8 encoded `std::string` into `std::filesystem::path`. To avoid copying characters from a `std::string` into a temporary `std::u8string` and from there into the `std::filesystem::path`, make a `boost::transform_iterator` that accepts a `std::string_view::iterator` and adapts it to dereference `char8_t` characters. Make `fsyspath(std::string_view)` engage the base-class constructor accepting (iterator, iterator), adapting `string_view::begin()` and `end()` to deliver `char8_t` characters. Use the same tactic for `fsyspath::operator=(std::string_view)`, explicitly calling `std::filesystem::path::assign()` with the adapted iterators. To resolve ambiguities, provide both constructors and assignment operators accepting `(const std::string&)` and `(const char*)`, explicitly converting each to `std::string_view`. At the same time, `std::filesystem::path::u8string()` now returns `std::u8string` rather than `std::string`. Since `std::filesystem::path` delivers only that `std::u8string` rather than iterators into its internal representation, we can't avoid capturing it and copying to the returned `std::string`. Remove explicit `.u8string()` calls from a few existing `fsyspath` instances, now that `fsyspath` supports implicit conversion to `std::string`. --- indra/llcommon/fsyspath.h | 56 ++++++++++++++++++++++++++--------------- indra/llcommon/lua_function.cpp | 8 +++--- 2 files changed, 40 insertions(+), 24 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/fsyspath.h b/indra/llcommon/fsyspath.h index 1b4aec09b4..f66970ed8f 100644 --- a/indra/llcommon/fsyspath.h +++ b/indra/llcommon/fsyspath.h @@ -12,7 +12,10 @@ #if ! defined(LL_FSYSPATH_H) #define LL_FSYSPATH_H +#include #include +#include +#include // While std::filesystem::path can be directly constructed from std::string on // both Posix and Windows, that's not what we want on Windows. Per @@ -33,42 +36,55 @@ // char"), the "native narrow encoding" isn't UTF-8, so file paths containing // non-ASCII characters get mangled. // -// Once we're building with C++20, we could pass a UTF-8 std::string through a -// vector to engage std::filesystem::path's own UTF-8 conversion. But -// sigh, as of 2024-04-03 we're not yet there. -// -// Anyway, encapsulating the important UTF-8 conversions in our own subclass -// allows us to migrate forward to C++20 conventions without changing -// referencing code. +// Encapsulating the important UTF-8 conversions in our own subclass allows us +// to migrate forward to C++20 conventions without changing referencing code. class fsyspath: public std::filesystem::path { using super = std::filesystem::path; + // In C++20 (__cpp_lib_char8_t), std::filesystem::u8path() is deprecated. + // std::filesystem::path(iter, iter) performs UTF-8 conversions when the + // value_type of the iterators is char8_t. While we could copy into a + // temporary std::u8string and from there into std::filesystem::path, to + // minimize string copying we'll define a transform_iterator that accepts + // a std::string_view::iterator and dereferences to char8_t. + struct u8ify + { + char8_t operator()(char c) const { return char8_t(c); } + }; + using u8iter = boost::transform_iterator; + public: // default fsyspath() {} - // construct from UTF-8 encoded std::string - fsyspath(const std::string& path): super(std::filesystem::u8path(path)) {} - // construct from UTF-8 encoded const char* - fsyspath(const char* path): super(std::filesystem::u8path(path)) {} + // construct from UTF-8 encoded string + fsyspath(const std::string& path): fsyspath(std::string_view(path)) {} + fsyspath(const char* path): fsyspath(std::string_view(path)) {} + fsyspath(std::string_view path): + super(u8iter(path.begin(), u8ify()), u8iter(path.end(), u8ify())) + {} // construct from existing path fsyspath(const super& path): super(path) {} - fsyspath& operator=(const super& p) { super::operator=(p); return *this; } - fsyspath& operator=(const std::string& p) - { - super::operator=(std::filesystem::u8path(p)); - return *this; - } - fsyspath& operator=(const char* p) + fsyspath& operator=(const super& p) { super::operator=(p); return *this; } + fsyspath& operator=(const std::string& p) { return (*this) = std::string_view(p); } + fsyspath& operator=(const char* p) { return (*this) = std::string_view(p); } + fsyspath& operator=(std::string_view p) { - super::operator=(std::filesystem::u8path(p)); + assign(u8iter(p.begin(), u8ify()), u8iter(p.end(), u8ify())); return *this; } // shadow base-class string() method with UTF-8 aware method - std::string string() const { return super::u8string(); } + std::string string() const + { + // Short of forbidden type punning, I see no way to avoid copying this + // std::u8string to a std::string. + auto u8str{ super::u8string() }; + // from https://github.com/tahonermann/char8_t-remediation/blob/master/char8_t-remediation.h#L180-L182 + return { u8str.begin(), u8str.end() }; + } // On Posix systems, where value_type is already char, this operator // std::string() method shadows the base class operator string_type() // method. But on Windows, where value_type is wchar_t, the base class diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp index a9f88f3170..e28c0caa27 100644 --- a/indra/llcommon/lua_function.cpp +++ b/indra/llcommon/lua_function.cpp @@ -170,7 +170,7 @@ fsyspath source_path(lua_State* L) { lua_getinfo(L, i, "s", &ar); } - return ar.source; + return { ar.source }; } } // namespace lluau @@ -1108,7 +1108,7 @@ lua_function(source_path, "source_path(): return the source path of the running { lua_checkdelta(L, 1); lluau_checkstack(L, 1); - lua_pushstdstring(L, lluau::source_path(L).u8string()); + lua_pushstdstring(L, lluau::source_path(L)); return 1; } @@ -1119,7 +1119,7 @@ lua_function(source_dir, "source_dir(): return the source directory of the runni { lua_checkdelta(L, 1); lluau_checkstack(L, 1); - lua_pushstdstring(L, lluau::source_path(L).parent_path().u8string()); + lua_pushstdstring(L, lluau::source_path(L).parent_path()); return 1; } @@ -1132,7 +1132,7 @@ lua_function(abspath, "abspath(path): " lua_checkdelta(L); auto path{ lua_tostdstring(L, 1) }; lua_pop(L, 1); - lua_pushstdstring(L, (lluau::source_path(L).parent_path() / path).u8string()); + lua_pushstdstring(L, (lluau::source_path(L).parent_path() / path)); return 1; } -- cgit v1.2.3 From 988a0fdd1eedab24677ed517942a4ef20c830f08 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 26 Sep 2024 09:00:48 -0400 Subject: Fix a few more fsyspath conversions, removing explicit u8string(). --- indra/llcommon/lua_function.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/lua_function.cpp b/indra/llcommon/lua_function.cpp index e28c0caa27..de06dab264 100644 --- a/indra/llcommon/lua_function.cpp +++ b/indra/llcommon/lua_function.cpp @@ -1119,7 +1119,7 @@ lua_function(source_dir, "source_dir(): return the source directory of the runni { lua_checkdelta(L, 1); lluau_checkstack(L, 1); - lua_pushstdstring(L, lluau::source_path(L).parent_path()); + lua_pushstdstring(L, fsyspath(lluau::source_path(L).parent_path())); return 1; } @@ -1132,7 +1132,7 @@ lua_function(abspath, "abspath(path): " lua_checkdelta(L); auto path{ lua_tostdstring(L, 1) }; lua_pop(L, 1); - lua_pushstdstring(L, (lluau::source_path(L).parent_path() / path)); + lua_pushstdstring(L, fsyspath(lluau::source_path(L).parent_path() / path)); return 1; } -- cgit v1.2.3 From 6ac59d1d5e200cccd3ddaa6d1c3b0d8d116a06be Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 26 Sep 2024 10:30:40 -0400 Subject: Fix GCC ambiguous-reversed-operator errors for `LLKeyData` compares. `LLKeyData::operator==(const LLKeyData&)` and `operator!=(const LLKeyData&)` were not themselves `const` methods. In C++20, that can produce a fatal warning that if the compare operands were reversed, you'd get different results. Making them both `const` should fix it. While touching the method definitions, make `operator==()` more intuitive, and make `operator!=()` simply negate `operator==()` instead of restating it in reverse. --- indra/llcommon/llkeybind.cpp | 20 ++++++++------------ indra/llcommon/llkeybind.h | 4 ++-- 2 files changed, 10 insertions(+), 14 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llkeybind.cpp b/indra/llcommon/llkeybind.cpp index e36c1d0a4c..34254f3ef5 100644 --- a/indra/llcommon/llkeybind.cpp +++ b/indra/llcommon/llkeybind.cpp @@ -123,22 +123,18 @@ LLKeyData& LLKeyData::operator=(const LLKeyData& rhs) return *this; } -bool LLKeyData::operator==(const LLKeyData& rhs) +bool LLKeyData::operator==(const LLKeyData& rhs) const { - if (mMouse != rhs.mMouse) return false; - if (mKey != rhs.mKey) return false; - if (mMask != rhs.mMask) return false; - if (mIgnoreMasks != rhs.mIgnoreMasks) return false; - return true; + return + (mMouse == rhs.mMouse) && + (mKey == rhs.mKey) && + (mMask == rhs.mMask) && + (mIgnoreMasks == rhs.mIgnoreMasks); } -bool LLKeyData::operator!=(const LLKeyData& rhs) +bool LLKeyData::operator!=(const LLKeyData& rhs) const { - if (mMouse != rhs.mMouse) return true; - if (mKey != rhs.mKey) return true; - if (mMask != rhs.mMask) return true; - if (mIgnoreMasks != rhs.mIgnoreMasks) return true; - return false; + return ! (*this == rhs); } bool LLKeyData::canHandle(const LLKeyData& data) const diff --git a/indra/llcommon/llkeybind.h b/indra/llcommon/llkeybind.h index 1bbb2fadb5..68efe38689 100644 --- a/indra/llcommon/llkeybind.h +++ b/indra/llcommon/llkeybind.h @@ -44,8 +44,8 @@ public: bool empty() const { return isEmpty(); }; void reset(); LLKeyData& operator=(const LLKeyData& rhs); - bool operator==(const LLKeyData& rhs); - bool operator!=(const LLKeyData& rhs); + bool operator==(const LLKeyData& rhs) const; + bool operator!=(const LLKeyData& rhs) const; bool canHandle(const LLKeyData& data) const; bool canHandle(EMouseClickType mouse, KEY key, MASK mask) const; -- cgit v1.2.3 From fcfcca977c23e318769171f06fc8554869495870 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Thu, 26 Sep 2024 16:44:56 -0400 Subject: Ditch last instances of LL_LIBRARY_INCLUDE. --- indra/llcommon/llerror.h | 5 ----- indra/llcommon/llpointer.h | 5 ----- 2 files changed, 10 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llerror.h b/indra/llcommon/llerror.h index b17b9ff21e..b6d560a121 100644 --- a/indra/llcommon/llerror.h +++ b/indra/llcommon/llerror.h @@ -239,17 +239,12 @@ namespace LLError ~CallSite(); -#ifdef LL_LIBRARY_INCLUDE - bool shouldLog(); -#else // LL_LIBRARY_INCLUDE bool shouldLog() { return mCached ? mShouldLog : Log::shouldLog(*this); } - // this member function needs to be in-line for efficiency -#endif // LL_LIBRARY_INCLUDE void invalidate(); diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index 71c955c4c5..1b2a961c40 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -189,10 +189,6 @@ public: } protected: -#ifdef LL_LIBRARY_INCLUDE - void ref(); - void unref(); -#else void ref() { if (mPointer) @@ -215,7 +211,6 @@ protected: } } } -#endif // LL_LIBRARY_INCLUDE protected: Type* mPointer; -- cgit v1.2.3 From a2b76b69ca1a29e40616d9279a25f9dd22a81127 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 27 Sep 2024 05:38:39 -0400 Subject: Migrate ~LLPointer()'s peculiar warning case to llpointer.cpp. This allows removing #include "llerror.h" from llpointer.h. Also remove #include "llmutex.h" as a heavy way to get . That requires adding #include "llmutex.h" to llimage.h, llnotifications.h, llwatchdog.cpp and llvolumemgr.cpp, which were inheriting it from llpointer.h. --- indra/llcommon/CMakeLists.txt | 1 + indra/llcommon/llpointer.cpp | 26 ++++++++++++++++++++++++++ indra/llcommon/llpointer.h | 19 ++++++++++++++----- 3 files changed, 41 insertions(+), 5 deletions(-) create mode 100755 indra/llcommon/llpointer.cpp (limited to 'indra/llcommon') diff --git a/indra/llcommon/CMakeLists.txt b/indra/llcommon/CMakeLists.txt index 78bfaade55..aa8810f00b 100644 --- a/indra/llcommon/CMakeLists.txt +++ b/indra/llcommon/CMakeLists.txt @@ -70,6 +70,7 @@ set(llcommon_SOURCE_FILES llmetrics.cpp llmortician.cpp llmutex.cpp + llpointer.cpp llpredicate.cpp llprocess.cpp llprocessor.cpp diff --git a/indra/llcommon/llpointer.cpp b/indra/llcommon/llpointer.cpp new file mode 100755 index 0000000000..adea447caa --- /dev/null +++ b/indra/llcommon/llpointer.cpp @@ -0,0 +1,26 @@ +/** + * @file llpointer.cpp + * @author Nat Goodspeed + * @date 2024-09-26 + * @brief Implementation for llpointer. + * + * $LicenseInfo:firstyear=2024&license=viewerlgpl$ + * Copyright (c) 2024, Linden Research, Inc. + * $/LicenseInfo$ + */ + +// Precompiled header +#include "linden_common.h" +// associated header +#include "llpointer.h" +// STL headers +// std headers +// external library headers +// other Linden headers +#include "llerror.h" + +void LLPointerBase::wild_dtor(std::string_view msg) +{ +// LL_WARNS() << msg << LL_ENDL; + llassert_msg(false, msg); +} diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index 1b2a961c40..b53cfcdd1a 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -26,8 +26,8 @@ #ifndef LLPOINTER_H #define LLPOINTER_H -#include "llerror.h" // *TODO: consider eliminating this -#include "llmutex.h" +#include +#include #include // std::swap() //---------------------------------------------------------------------------- @@ -43,8 +43,18 @@ //---------------------------------------------------------------------------- +class LLPointerBase +{ +protected: + // alert the coder that a referenced type's destructor did something very + // strange -- this is in a non-template base class so we can hide the + // implementation in llpointer.cpp + static void wild_dtor(std::string_view msg); +}; + // Note: relies on Type having ref() and unref() methods -template class LLPointer +template +class LLPointer: public LLPointerBase { public: template @@ -106,7 +116,6 @@ public: const Type& operator*() const { return *mPointer; } Type& operator*() { return *mPointer; } - operator BOOL() const { return (mPointer != nullptr); } operator bool() const { return (mPointer != nullptr); } bool operator!() const { return (mPointer == nullptr); } bool isNull() const { return (mPointer == nullptr); } @@ -206,7 +215,7 @@ protected: temp->unref(); if (mPointer != nullptr) { - LL_WARNS() << "Unreference did assignment to non-NULL because of destructor" << LL_ENDL; + wild_dtor("Unreference did assignment to non-NULL because of destructor"); unref(); } } -- cgit v1.2.3 From 1c78829ab3177940adbdb7f0081b5f46c1e37763 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 27 Sep 2024 11:07:08 -0400 Subject: Introduce owning_ptr; use it for JPEG2KEncode and JPEG2KDecode. owning_ptr adapts std::unique_ptr to be a better drop-in replacement for a legacy class that formerly stored plain T* data members, and explicitly destroyed them using domain-specific destructor functions. Directly substituting std::unique_ptr into JPEG2KEncode and JPEG2KDecode was cumbersome because every such pointer declaration required a redundant template parameter describing the deleter function passed into its constructor. Moreover, it required lots of little syntax tweaks: changing every assignment to a reset() call, changing every reference to a get() call. Using owning_ptr allows us to leave the code more or less as it was before, save that assignment and destruction automatically handle the previous referenced T instance. --- indra/llcommon/owning_ptr.h | 71 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100755 indra/llcommon/owning_ptr.h (limited to 'indra/llcommon') diff --git a/indra/llcommon/owning_ptr.h b/indra/llcommon/owning_ptr.h new file mode 100755 index 0000000000..c0da245265 --- /dev/null +++ b/indra/llcommon/owning_ptr.h @@ -0,0 +1,71 @@ +/** + * @file owning_ptr.h + * @author Nat Goodspeed + * @date 2024-09-27 + * @brief owning_ptr is like std::unique_ptr, but easier to integrate + * + * $LicenseInfo:firstyear=2024&license=viewerlgpl$ + * Copyright (c) 2024, Linden Research, Inc. + * $/LicenseInfo$ + */ + +#if ! defined(LL_OWNING_PTR_H) +#define LL_OWNING_PTR_H + +#include +#include + +/** + * owning_ptr adapts std::unique_ptr to make it easier to adopt into + * older code using dumb pointers. + * + * Consider a class Outer with a member Thing* mThing. After the constructor, + * each time a method wants to assign to mThing, it must test for nullptr and + * destroy the previous Thing instance. During Outer's lifetime, mThing is + * passed to legacy domain-specific functions accepting plain Thing*. Finally + * the destructor must again test for nullptr and destroy the remaining Thing + * instance. + * + * Multiply that by several different Outer members of different types, + * possibly with different domain-specific destructor functions. + * + * Dropping std::unique_ptr into Outer is cumbersome for a several + * reasons. First, if Thing requires a domain-specific destructor function, + * the unique_ptr declaration of mThing must explicitly state the type of that + * function (as a function pointer, for a typical legacy function). Second, + * every Thing* assignment to mThing must be changed to mThing.reset(). Third, + * every time we call a legacy domain-specific function, we must pass + * mThing.get(). + * + * owning_ptr is designed to drop into a situation like this. The domain- + * specific destructor function, if any, is passed to its constructor; it need + * not be encoded into the pointer type. owning_ptr supports plain pointer + * assignment, internally calling std::unique_ptr::reset(). It also + * supports implicit conversion to plain T*, to pass the owned pointer to + * legacy domain-specific functions. + * + * Obviously owning_ptr must not be used in situations where ownership of + * the referenced object is passed on to another pointer: use std::unique_ptr + * for that. Similarly, it is not for shared ownership. It simplifies lifetime + * management for classes that currently store (and explicitly destroy) plain + * T* pointers. + */ +template +class owning_ptr +{ + using deleter = std::function; +public: + owning_ptr(T* p=nullptr, const deleter& d=std::default_delete()): + mPtr(p, d) + {} + void reset(T* p=nullptr) { mPtr.reset(p); } + owning_ptr& operator=(T* p) { mPtr.reset(p); return *this; } + operator T*() const { return mPtr.get(); } + T& operator*() const { return *mPtr; } + T* operator->() const { return mPtr.operator->(); } + +private: + std::unique_ptr> mPtr; +}; + +#endif /* ! defined(LL_OWNING_PTR_H) */ -- cgit v1.2.3 From cf2f482ceca796e587dfe2fb8df552c09156fb50 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Fri, 27 Sep 2024 11:11:29 -0400 Subject: Slightly streamline owning_ptr class definition. --- indra/llcommon/owning_ptr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/owning_ptr.h b/indra/llcommon/owning_ptr.h index c0da245265..7cf8d3f0ba 100755 --- a/indra/llcommon/owning_ptr.h +++ b/indra/llcommon/owning_ptr.h @@ -65,7 +65,7 @@ public: T* operator->() const { return mPtr.operator->(); } private: - std::unique_ptr> mPtr; + std::unique_ptr mPtr; }; #endif /* ! defined(LL_OWNING_PTR_H) */ -- cgit v1.2.3 From a76209d099c3a18967d26e5b091f83e9ae9fd6a1 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2024 09:44:43 -0400 Subject: Eliminate double names for coros + "empty name is main" convention. Instead, introduce bool CoroData::isMain and test that. Use "main" for the name of the main coroutine. That eliminates the logname() method, also the llcoro::logname() free function. It also obviates the alternate CoroData constructor. Use boost::fibers::fiber::id as the LLInstanceTracker key for CoroData, instead of the coroutine name. Introduce get_CoroData(id), also getName(id). Extract static CoroData for the main coroutine to main_CoroData() so both get_CoroData() overloads can use it. Ditch unused get_CoroData(string) parameter: now get_CoroData(). Introduce LLCoros::mNameMap for lookup by name (e.g. killreq()). CoroData's constructor puts an entry into mNameMap; the destructor removes it. Since mNameMap is thread_local (unlike an LLInstanceTracker key), that theoretically permits duplicate coroutine names on different threads. Introduce mPrefixMap to help generate distinct coroutine names, instead of a single scalar. Introduce CoroData::getName(), and use it in both LLCoros::getName() overloads. CoroData::getName() appends mStatus if it's not empty. This is useful for disambiguating generic pool coroutines based on the current task. --- indra/llcommon/llcoros.cpp | 119 +++++++++++++++++++++++++++++-------------- indra/llcommon/llcoros.h | 97 +++++++++++++++++------------------ indra/llcommon/workqueue.cpp | 4 +- 3 files changed, 128 insertions(+), 92 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 1ae5c87a00..0a3ca99b05 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -62,11 +62,13 @@ #include #endif +thread_local std::unordered_map LLCoros::mPrefixMap; +thread_local std::unordered_map LLCoros::mNameMap; + // static bool LLCoros::on_main_coro() { - return (!LLCoros::instanceExists() || - LLCoros::getName().empty()); + return (!instanceExists() || get_CoroData().isMain); } // static @@ -76,11 +78,11 @@ bool LLCoros::on_main_thread_main_coro() } // static -LLCoros::CoroData& LLCoros::get_CoroData(const std::string&) +LLCoros::CoroData& LLCoros::get_CoroData() { CoroData* current{ nullptr }; // be careful about attempted accesses in the final throes of app shutdown - if (! wasDeleted()) + if (instanceExists()) { current = instance().mCurrent.get(); } @@ -89,16 +91,26 @@ LLCoros::CoroData& LLCoros::get_CoroData(const std::string&) // canonical values. if (! current) { - static std::atomic which_thread(0); - // Use alternate CoroData constructor. - static thread_local CoroData sMain(which_thread++); // We need not reset() the local_ptr to this instance; we'll simply // find it again every time we discover that current is null. - current = &sMain; + current = &main_CoroData(); } return *current; } +LLCoros::CoroData& LLCoros::get_CoroData(id id) +{ + auto found = CoroData::getInstance(id); + return found? *found : main_CoroData(); +} + +LLCoros::CoroData& LLCoros::main_CoroData() +{ + // tell CoroData we're "main" + static thread_local CoroData sMain(""); + return sMain; +} + //static LLCoros::coro::id LLCoros::get_self() { @@ -108,28 +120,28 @@ LLCoros::coro::id LLCoros::get_self() //static void LLCoros::set_consuming(bool consuming) { - auto& data(get_CoroData("set_consuming()")); + auto& data(get_CoroData()); // DO NOT call this on the main() coroutine. - llassert_always(! data.mName.empty()); + llassert_always(! data.isMain); data.mConsuming = consuming; } //static bool LLCoros::get_consuming() { - return get_CoroData("get_consuming()").mConsuming; + return get_CoroData().mConsuming; } // static void LLCoros::setStatus(const std::string& status) { - get_CoroData("setStatus()").mStatus = status; + get_CoroData().mStatus = status; } // static std::string LLCoros::getStatus() { - return get_CoroData("getStatus()").mStatus; + return get_CoroData().mStatus; } LLCoros::LLCoros(): @@ -186,9 +198,8 @@ void LLCoros::cleanupSingleton() std::string LLCoros::generateDistinctName(const std::string& prefix) const { - static int unique = 0; - - // Allowing empty name would make getName()'s not-found return ambiguous. + // Empty name would trigger CoroData's constructor's special case for the + // main coroutine. if (prefix.empty()) { LL_ERRS("LLCoros") << "LLCoros::launch(): pass non-empty name string" << LL_ENDL; @@ -196,9 +207,11 @@ std::string LLCoros::generateDistinctName(const std::string& prefix) const // If the specified name isn't already in the map, just use that. std::string name(prefix); + // maintain a distinct int suffix for each prefix + int& unique = mPrefixMap[prefix]; - // Until we find an unused name, append a numeric suffix for uniqueness. - while (CoroData::getInstance(name)) + // Until we find an unused name, append int suffix for uniqueness. + while (mNameMap.find(name) != mNameMap.end()) { name = stringize(prefix, unique++); } @@ -207,9 +220,16 @@ std::string LLCoros::generateDistinctName(const std::string& prefix) const bool LLCoros::killreq(const std::string& name) { - auto found = CoroData::getInstance(name); + auto foundName = mNameMap.find(name); + if (foundName == mNameMap.end()) + { + // couldn't find that name in map + return false; + } + auto found = CoroData::getInstance(foundName->second); if (! found) { + // found name, but CoroData with that ID key no longer exists return false; } // Next time the subject coroutine calls checkStop(), make it terminate. @@ -224,14 +244,13 @@ bool LLCoros::killreq(const std::string& name) //static std::string LLCoros::getName() { - return get_CoroData("getName()").mName; + return get_CoroData().getName(); } -//static -std::string LLCoros::logname() +// static +std::string LLCoros::getName(id id) { - auto& data(get_CoroData("logname()")); - return data.mName.empty()? data.getKey() : data.mName; + return get_CoroData(id).getName(); } void LLCoros::saveException(const std::string& name, std::exception_ptr exc) @@ -264,7 +283,7 @@ void LLCoros::printActiveCoroutines(const std::string& when) { LL_INFOS("LLCoros") << "-------------- List of active coroutines ------------"; F64 time = LLTimer::getTotalSeconds(); - for (auto& cd : CoroData::instance_snapshot()) + for (const auto& cd : CoroData::instance_snapshot()) { F64 life_time = time - cd.mCreationTime; LL_CONT << LL_NEWLINE @@ -364,8 +383,8 @@ void LLCoros::checkStop(callable_t cleanup) // do this AFTER the check above, because get_CoroData() depends on the // local_ptr in our instance(). - auto& data(get_CoroData("checkStop()")); - if (data.mName.empty()) + auto& data(get_CoroData()); + if (data.isMain) { // Our Stop exception and its subclasses are intended to stop loitering // coroutines. Don't throw it from the main coroutine. @@ -385,7 +404,7 @@ void LLCoros::checkStop(callable_t cleanup) { // Someone wants to kill this coroutine cleanup(); - LLTHROW(Killed(stringize("coroutine ", data.mName, " killed by ", data.mKilledBy))); + LLTHROW(Killed(stringize("coroutine ", data.getName(), " killed by ", data.mKilledBy))); } } @@ -445,20 +464,44 @@ LLBoundListener LLCoros::getStopListener(const std::string& caller, } LLCoros::CoroData::CoroData(const std::string& name): - LLInstanceTracker(name), + super(boost::this_fiber::get_id()), mName(name), mCreationTime(LLTimer::getTotalSeconds()) { + // we expect the empty string for the main coroutine + if (name.empty()) + { + isMain = true; + if (on_main_thread()) + { + // main coroutine on main thread + mName = "main"; + } + else + { + // main coroutine on some other thread + static std::atomic main_no{ 0 }; + mName = stringize("main", ++main_no); + } + } + // maintain LLCoros::mNameMap + LLCoros::mNameMap.emplace(mName, getKey()); } -LLCoros::CoroData::CoroData(int n): - // This constructor is used for the thread_local instance belonging to the - // default coroutine on each thread. We must give each one a different - // LLInstanceTracker key because LLInstanceTracker's map spans all - // threads, but we want the default coroutine on each thread to have the - // empty string as its visible name because some consumers test for that. - LLInstanceTracker("main" + stringize(n)), - mName(), - mCreationTime(LLTimer::getTotalSeconds()) +LLCoros::CoroData::~CoroData() +{ + // Don't try to erase the static main CoroData from our static + // thread_local mNameMap; that could run into destruction order problems. + if (! isMain) + { + LLCoros::mNameMap.erase(mName); + } +} + +std::string LLCoros::CoroData::getName() const { + if (mStatus.empty()) + return mName; + else + return stringize(mName, " (", mStatus, ")"); } diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index 0291d7f1d9..662be9dcc9 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -39,39 +39,35 @@ #include #include #include +#include + +namespace llcoro +{ +class scheduler; +} /** - * Registry of named Boost.Coroutine instances - * - * The Boost.Coroutine library supports the general case of a coroutine - * accepting arbitrary parameters and yielding multiple (sets of) results. For - * such use cases, it's natural for the invoking code to retain the coroutine - * instance: the consumer repeatedly calls into the coroutine, perhaps passing - * new parameter values, prompting it to yield its next result. - * - * Our typical coroutine usage is different, though. For us, coroutines - * provide an alternative to the @c Responder pattern. Our typical coroutine - * has @c void return, invoked in fire-and-forget mode: the handler for some - * user gesture launches the coroutine and promptly returns to the main loop. - * The coroutine initiates some action that will take multiple frames (e.g. a - * capability request), waits for its result, processes it and silently steals - * away. + * Registry of named Boost.Fiber instances * - * This usage poses two (related) problems: + * When the viewer first introduced the semi-independent execution agents now + * called fibers, the term "fiber" had not yet become current, and the only + * available libraries used the term "coroutine" instead. Within the viewer we + * continue to use the term "coroutines," though at present they are actually + * boost::fibers::fiber instances. * - * # Who should own the coroutine instance? If it's simply local to the - * handler code that launches it, return from the handler will destroy the - * coroutine object, terminating the coroutine. - * # Once the coroutine terminates, in whatever way, who's responsible for - * cleaning up the coroutine object? + * Coroutines provide an alternative to the @c Responder pattern. Our typical + * coroutine has @c void return, invoked in fire-and-forget mode: the handler + * for some user gesture launches the coroutine and promptly returns to the + * main loop. The coroutine initiates some action that will take multiple + * frames (e.g. a capability request), waits for its result, processes it and + * silently steals away. * * LLCoros is a Singleton collection of currently-active coroutine instances. * Each has a name. You ask LLCoros to launch a new coroutine with a suggested * name prefix; from your prefix it generates a distinct name, registers the * new coroutine and returns the actual name. * - * The name - * can provide diagnostic info: we can look up the name of the + * The name can provide diagnostic info: we can look up the name of the * currently-running coroutine. */ class LL_COMMON_API LLCoros: public LLSingleton @@ -91,12 +87,8 @@ public: // llassert(LLCoros::on_main_thread_main_coro()) static bool on_main_thread_main_coro(); - /// The viewer's use of the term "coroutine" became deeply embedded before - /// the industry term "fiber" emerged to distinguish userland threads from - /// simpler, more transient kinds of coroutines. Semantically they've - /// always been fibers. But at this point in history, we're pretty much - /// stuck with the term "coroutine." typedef boost::fibers::fiber coro; + typedef coro::id id; /// Canonical callable type typedef std::function callable_t; @@ -150,12 +142,15 @@ public: /** * From within a coroutine, look up the (tweaked) name string by which - * this coroutine is registered. Returns the empty string if not found - * (e.g. if the coroutine was launched by hand rather than using - * LLCoros::launch()). + * this coroutine is registered. */ static std::string getName(); + /** + * Given an id, return the name of that coroutine. + */ + static std::string getName(id); + /** * rethrow() is called by the thread's main fiber to propagate an * exception from any coroutine into the main fiber, where it can engage @@ -169,13 +164,6 @@ public: */ void rethrow(); - /** - * This variation returns a name suitable for log messages: the explicit - * name for an explicitly-launched coroutine, or "mainN" for the default - * coroutine on a thread. - */ - static std::string logname(); - /** * For delayed initialization. To be clear, this will only affect * coroutines launched @em after this point. The underlying facility @@ -187,7 +175,7 @@ public: void printActiveCoroutines(const std::string& when=std::string()); /// get the current coro::id for those who really really care - static coro::id get_self(); + static id get_self(); /** * Most coroutines, most of the time, don't "consume" the events for which @@ -236,6 +224,7 @@ public: setStatus(status); } TempStatus(const TempStatus&) = delete; + TempStatus& operator=(const TempStatus&) = delete; ~TempStatus() { setStatus(mOldStatus); @@ -331,10 +320,14 @@ public: using local_ptr = boost::fibers::fiber_specific_ptr; private: + friend class llcoro::scheduler; + std::string generateDistinctName(const std::string& prefix) const; void toplevel(std::string name, callable_t callable); struct CoroData; - static CoroData& get_CoroData(const std::string& caller); + static CoroData& get_CoroData(); + static CoroData& get_CoroData(id); + static CoroData& main_CoroData(); void saveException(const std::string& name, std::exception_ptr exc); LLTempBoundListener mConn; @@ -355,13 +348,18 @@ private: S32 mStackSize; // coroutine-local storage, as it were: one per coro we track - struct CoroData: public LLInstanceTracker + struct CoroData: public LLInstanceTracker { + using super = LLInstanceTracker; + CoroData(const std::string& name); - CoroData(int n); + ~CoroData(); + std::string getName() const; + + bool isMain{ false }; // tweaked name of the current coroutine - const std::string mName; + std::string mName; // set_consuming() state -- don't consume events unless specifically directed bool mConsuming{ false }; // killed by which coroutine @@ -375,14 +373,11 @@ private: // because it's a member of an LLSingleton, and we rely on it being // cleaned up in proper dependency order. local_ptr mCurrent; -}; -namespace llcoro -{ - -inline -std::string logname() { return LLCoros::logname(); } - -} // llcoro + // ensure name uniqueness + static thread_local std::unordered_map mPrefixMap; + // lookup by name + static thread_local std::unordered_map mNameMap; +}; #endif /* ! defined(LL_LLCOROS_H) */ diff --git a/indra/llcommon/workqueue.cpp b/indra/llcommon/workqueue.cpp index 9138c862f9..8b7b97a1f9 100644 --- a/indra/llcommon/workqueue.cpp +++ b/indra/llcommon/workqueue.cpp @@ -127,9 +127,7 @@ void LL::WorkQueueBase::error(const std::string& msg) void LL::WorkQueueBase::checkCoroutine(const std::string& method) { - // By convention, the default coroutine on each thread has an empty name - // string. See also LLCoros::logname(). - if (LLCoros::getName().empty()) + if (LLCoros::on_main_coro()) { LLTHROW(Error("Do not call " + method + " from a thread's default coroutine")); } -- cgit v1.2.3 From b0645835595f3517223329ba62f46277d3e3a9dd Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2024 11:34:46 -0400 Subject: Make llcoro::scheduler log coros that run too long between yields. Introduce LLCoros::CoroData::mHistogram, a map of cutoff times (bucket breakpoints) with counts of occurrences. The idea is that mHistogram counts how many times the real time taken by a particular coroutine resumption falls into one of those buckets. Initialize the map with guessed buckets; these are set in llcoros.cpp so they can be changed without requiring extensive rebuilds. scheduler::pick_next() now records the timestamp and fiber context just before the fiber manager resumes the next coroutine. If the next pick_next() call reveals that the previous resumption took longer than the minimum bucket breakpoint, it increments the appropriate bucket counter and logs the instance. LLCoros::toplevel() reports nonzero mHistogram entries on coroutine termination. --- indra/llcommon/coro_scheduler.cpp | 84 ++++++++++++++++++++++++++++++--------- indra/llcommon/coro_scheduler.h | 15 ++++--- indra/llcommon/llcoros.cpp | 37 ++++++++++++++++- indra/llcommon/llcoros.h | 8 ++++ 4 files changed, 119 insertions(+), 25 deletions(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/coro_scheduler.cpp b/indra/llcommon/coro_scheduler.cpp index 2d8b6e1a97..93efac7c7e 100644 --- a/indra/llcommon/coro_scheduler.cpp +++ b/indra/llcommon/coro_scheduler.cpp @@ -20,6 +20,7 @@ #include // other Linden headers #include "llcallbacklist.h" +#include "llcoros.h" #include "lldate.h" #include "llerror.h" @@ -56,17 +57,56 @@ void scheduler::awakened( boost::fibers::context* ctx) noexcept boost::fibers::context* scheduler::pick_next() noexcept { + auto now = LLDate::now().secondsSinceEpoch(); // count calls to pick_next() ++mSwitches; // pick_next() is called when the previous fiber has suspended, and we // need to pick another. Did the previous pick_next() call pick the main - // fiber? If so, it's the main fiber that just suspended. - auto now = LLDate::now().secondsSinceEpoch(); - if (mMainRunning) + // fiber? (Or is this the first pick_next() call?) If so, it's the main + // fiber that just suspended. + if ((! mPrevCtx) || mPrevCtx->get_id() == mMainID) { - mMainRunning = false; mMainLast = now; } + else + { + // How long did we spend in the fiber that just suspended? + // Don't bother with long runs of the main fiber, since (a) it happens + // pretty often and (b) it's moderately likely that we've reached here + // from the canonical yield at the top of mainloop, and what we'd want + // to know about is whatever the main fiber was doing in the + // *previous* iteration of mainloop. + F64 elapsed{ now - mResumeTime }; + LLCoros::CoroData& data{ LLCoros::get_CoroData(mPrevCtx->get_id()) }; + // Find iterator to the first mHistogram key greater than elapsed. + auto past = data.mHistogram.upper_bound(elapsed); + // If the smallest key (mHistogram.begin()->first) is greater than + // elapsed, then we need not bother with this timeslice. + if (past != data.mHistogram.begin()) + { + // Here elapsed was greater than at least one key. Back off to the + // previous entry and increment that count. If it's end(), backing + // off gets us the last entry -- assuming mHistogram isn't empty. + llassert(! data.mHistogram.empty()); + ++(--past)->second; + LL::WorkQueue::ptr_t queue{ getWorkQueue() }; + // make sure the queue exists + if (queue) + { + // If it proves difficult to track down *why* the fiber spent so + // much time, consider also binding and reporting + // boost::stacktrace::stacktrace(). + queue->post( + [name=std::move(data.getName()), elapsed] + { + LL_WARNS_ONCE("LLCoros.scheduler") + << "Coroutine " << name << " ran for " + << elapsed << " seconds" << LL_ENDL; + + }); + } + } + } boost::fibers::context* next; @@ -96,17 +136,9 @@ boost::fibers::context* scheduler::pick_next() noexcept // passage could be skipped. // Record this event for logging, but push it off to a thread pool to - // perform that work. Presumably std::weak_ptr::lock() is cheaper than - // WorkQueue::getInstance(). - LL::WorkQueue::ptr_t queue{ mQueue.lock() }; - // We probably started before the relevant WorkQueue was created. - if (! queue) - { - // Try again to locate the specified WorkQueue. - queue = LL::WorkQueue::getInstance(qname); - mQueue = queue; - } - // Both the lock() call and the getInstance() call might have failed. + // perform that work. + LL::WorkQueue::ptr_t queue{ getWorkQueue() }; + // The work queue we're looking for might not exist right now. if (queue) { // Bind values. Do NOT bind 'this' to avoid cross-thread access! @@ -116,7 +148,6 @@ boost::fibers::context* scheduler::pick_next() noexcept // so we have no access. queue->post( [switches=mSwitches, start=mStart, elapsed, now] - () { U32 runtime(U32(now) - U32(start)); U32 minutes(runtime / 60u); @@ -150,12 +181,29 @@ boost::fibers::context* scheduler::pick_next() noexcept { // we're about to resume the main fiber: it's no longer "ready" mMainCtx = nullptr; - // instead, it's "running" - mMainRunning = true; } + mPrevCtx = next; + // remember when we resumed this fiber so our next call can measure how + // long the previous resumption was + mResumeTime = LLDate::now().secondsSinceEpoch(); return next; } +LL::WorkQueue::ptr_t scheduler::getWorkQueue() +{ + // Cache a weak_ptr to our target work queue, presuming that + // std::weak_ptr::lock() is cheaper than WorkQueue::getInstance(). + LL::WorkQueue::ptr_t queue{ mQueue.lock() }; + // We probably started before the relevant WorkQueue was created. + if (! queue) + { + // Try again to locate the specified WorkQueue. + queue = LL::WorkQueue::getInstance(qname); + mQueue = queue; + } + return queue; +} + void scheduler::use() { boost::fibers::use_scheduling_algorithm(); diff --git a/indra/llcommon/coro_scheduler.h b/indra/llcommon/coro_scheduler.h index eee2d746b5..7af90685dc 100644 --- a/indra/llcommon/coro_scheduler.h +++ b/indra/llcommon/coro_scheduler.h @@ -47,17 +47,20 @@ public: static void use(); private: - // This is the fiber::id of the main fiber. We use this to discover - // whether the fiber passed to awakened() is in fact the main fiber. + LL::WorkQueue::ptr_t getWorkQueue(); + + // This is the fiber::id of the main fiber. boost::fibers::fiber::id mMainID; - // This context* is nullptr until awakened() notices that the main fiber - // has become ready, at which point it contains the main fiber's context*. + // This context* is nullptr while the main fiber is running or suspended, + // but is set to the main fiber's context each time the main fiber is ready. boost::fibers::context* mMainCtx{}; - // Set when pick_next() returns the main fiber. - bool mMainRunning{ false }; + // Remember the context returned by the previous pick_next() call. + boost::fibers::context* mPrevCtx{}; // If it's been at least this long since the last time the main fiber got // control, jump it to the head of the queue. F64 mTimeslice{ DEFAULT_TIMESLICE }; + // Time when we resumed the most recently running fiber + F64 mResumeTime{ 0 }; // Timestamp as of the last time we suspended the main fiber. F64 mMainLast{ 0 }; // Timestamp of start time diff --git a/indra/llcommon/llcoros.cpp b/indra/llcommon/llcoros.cpp index 0a3ca99b05..5a3cbd2ef1 100644 --- a/indra/llcommon/llcoros.cpp +++ b/indra/llcommon/llcoros.cpp @@ -57,6 +57,7 @@ #include "llsdutil.h" #include "lltimer.h" #include "stringize.h" +#include "scope_exit.h" #if LL_WINDOWS #include @@ -342,6 +343,33 @@ void LLCoros::toplevel(std::string name, callable_t callable) // run the code the caller actually wants in the coroutine try { + LL::scope_exit report{ + [&corodata] + { + bool allzero = true; + for (const auto& [threshold, occurs] : corodata.mHistogram) + { + if (occurs) + { + allzero = false; + break; + } + } + if (! allzero) + { + LL_WARNS("LLCoros") << "coroutine " << corodata.mName; + const char* sep = " exceeded "; + for (const auto& [threshold, occurs] : corodata.mHistogram) + { + if (occurs) + { + LL_CONT << sep << threshold << " " << occurs << " times"; + sep = ", "; + } + } + LL_ENDL; + } + }}; LL::seh::catcher(callable); } catch (const Stop& exc) @@ -466,7 +494,14 @@ LLBoundListener LLCoros::getStopListener(const std::string& caller, LLCoros::CoroData::CoroData(const std::string& name): super(boost::this_fiber::get_id()), mName(name), - mCreationTime(LLTimer::getTotalSeconds()) + mCreationTime(LLTimer::getTotalSeconds()), + // Preset threshold times in mHistogram + mHistogram{ + {0.004, 0}, + {0.040, 0}, + {0.400, 0}, + {1.000, 0} + } { // we expect the empty string for the main coroutine if (name.empty()) diff --git a/indra/llcommon/llcoros.h b/indra/llcommon/llcoros.h index 662be9dcc9..1edcb7e387 100644 --- a/indra/llcommon/llcoros.h +++ b/indra/llcommon/llcoros.h @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -367,6 +368,13 @@ private: // setStatus() state std::string mStatus; F64 mCreationTime; // since epoch + // Histogram of how many times this coroutine's timeslice exceeds + // certain thresholds. mHistogram is pre-populated with those + // thresholds as keys. If k0 is one threshold key and k1 is the next, + // mHistogram[k0] is the number of times a coroutine timeslice tn ran + // (k0 <= tn < k1). A timeslice less than mHistogram.begin()->first is + // fine; we don't need to record those. + std::map mHistogram; }; // Identify the current coroutine's CoroData. This local_ptr isn't static -- cgit v1.2.3 From 7f094aa20418b3c46a0c2c1d0730b0a14c058a6f Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2024 12:00:26 -0400 Subject: Eliminate meaningless blank line --- indra/llcommon/coro_scheduler.cpp | 1 - 1 file changed, 1 deletion(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/coro_scheduler.cpp b/indra/llcommon/coro_scheduler.cpp index 93efac7c7e..7b900a13f6 100644 --- a/indra/llcommon/coro_scheduler.cpp +++ b/indra/llcommon/coro_scheduler.cpp @@ -102,7 +102,6 @@ boost::fibers::context* scheduler::pick_next() noexcept LL_WARNS_ONCE("LLCoros.scheduler") << "Coroutine " << name << " ran for " << elapsed << " seconds" << LL_ENDL; - }); } } -- cgit v1.2.3 From 77f9ab96779c55a20a5ccd8b9e906227b3e8e5c8 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 22 Oct 2024 12:04:59 -0400 Subject: Remove move --- indra/llcommon/coro_scheduler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'indra/llcommon') diff --git a/indra/llcommon/coro_scheduler.cpp b/indra/llcommon/coro_scheduler.cpp index 7b900a13f6..b6117fa6a1 100644 --- a/indra/llcommon/coro_scheduler.cpp +++ b/indra/llcommon/coro_scheduler.cpp @@ -97,7 +97,7 @@ boost::fibers::context* scheduler::pick_next() noexcept // much time, consider also binding and reporting // boost::stacktrace::stacktrace(). queue->post( - [name=std::move(data.getName()), elapsed] + [name=data.getName(), elapsed] { LL_WARNS_ONCE("LLCoros.scheduler") << "Coroutine " << name << " ran for " -- cgit v1.2.3