From 0e253cb9098cb58b2f3528860a5fd9f00ec5af96 Mon Sep 17 00:00:00 2001 From: Ansariel Date: Wed, 12 May 2021 10:45:23 +0200 Subject: [PATCH 1/6] BUG-230673: Trim asset disk cache regularly --- indra/llfilesystem/lldiskcache.cpp | 22 ++++++++++++++++++++++ indra/llfilesystem/lldiskcache.h | 11 +++++++++++ indra/newview/app_settings/settings.xml | 2 +- indra/newview/llappviewer.cpp | 6 ++++++ indra/newview/llappviewer.h | 3 +++ 5 files changed, 43 insertions(+), 1 deletion(-) diff --git a/indra/llfilesystem/lldiskcache.cpp b/indra/llfilesystem/lldiskcache.cpp index 02678864b9..68423cc4f8 100644 --- a/indra/llfilesystem/lldiskcache.cpp +++ b/indra/llfilesystem/lldiskcache.cpp @@ -335,3 +335,25 @@ uintmax_t LLDiskCache::dirFileSize(const std::string dir) return total_file_size; } + +LLPurgeDiskCacheThread::LLPurgeDiskCacheThread() : + LLThread("PurgeDiskCacheThread", nullptr) +{ +} + +void LLPurgeDiskCacheThread::run() +{ + constexpr F64 CHECK_INTERVAL = 60; + mTimer.setTimerExpirySec(CHECK_INTERVAL); + mTimer.start(); + + do + { + if (mTimer.checkExpirationAndReset(CHECK_INTERVAL)) + { + LLDiskCache::instance().purge(); + } + + ms_sleep(100); + } while (!isQuitting()); +} \ No newline at end of file diff --git a/indra/llfilesystem/lldiskcache.h b/indra/llfilesystem/lldiskcache.h index c19714434a..867a80f332 100644 --- a/indra/llfilesystem/lldiskcache.h +++ b/indra/llfilesystem/lldiskcache.h @@ -180,4 +180,15 @@ class LLDiskCache : bool mEnableCacheDebugInfo; }; +class LLPurgeDiskCacheThread : public LLThread +{ +public: + LLPurgeDiskCacheThread(); + +protected: + void run() override; + +private: + LLTimer mTimer; +}; #endif // _LLDISKCACHE diff --git a/indra/newview/app_settings/settings.xml b/indra/newview/app_settings/settings.xml index cbd33e9244..0cae9cd9cc 100644 --- a/indra/newview/app_settings/settings.xml +++ b/indra/newview/app_settings/settings.xml @@ -1362,7 +1362,7 @@ Value 23 - EnableCacheDebugInfo + EnableDiskCacheDebugInfo Comment When set, display additional cache debugging information diff --git a/indra/newview/llappviewer.cpp b/indra/newview/llappviewer.cpp index c519e55fc3..fd094b12d7 100644 --- a/indra/newview/llappviewer.cpp +++ b/indra/newview/llappviewer.cpp @@ -655,6 +655,7 @@ LLAppViewer* LLAppViewer::sInstance = NULL; LLTextureCache* LLAppViewer::sTextureCache = NULL; LLImageDecodeThread* LLAppViewer::sImageDecodeThread = NULL; LLTextureFetch* LLAppViewer::sTextureFetch = NULL; +LLPurgeDiskCacheThread* LLAppViewer::sPurgeDiskCacheThread = NULL; std::string getRuntime() { @@ -2032,6 +2033,7 @@ bool LLAppViewer::cleanup() sTextureFetch->shutdown(); sTextureCache->shutdown(); sImageDecodeThread->shutdown(); + sPurgeDiskCacheThread->shutdown(); sTextureFetch->shutDownTextureCacheThread() ; sTextureFetch->shutDownImageDecodeThread() ; @@ -2054,6 +2056,8 @@ bool LLAppViewer::cleanup() sImageDecodeThread = NULL; delete mFastTimerLogThread; mFastTimerLogThread = NULL; + delete sPurgeDiskCacheThread; + sPurgeDiskCacheThread = NULL; if (LLFastTimerView::sAnalyzePerformance) { @@ -2174,6 +2178,7 @@ bool LLAppViewer::initThreads() sImageDecodeThread, enable_threads && true, app_metrics_qa_mode); + LLAppViewer::sPurgeDiskCacheThread = new LLPurgeDiskCacheThread(); if (LLTrace::BlockTimer::sLog || LLTrace::BlockTimer::sMetricLog) { @@ -4210,6 +4215,7 @@ bool LLAppViewer::initCache() LLDiskCache::getInstance()->purge(); } } + LLAppViewer::getPurgeDiskCacheThread()->start(); LLSplashScreen::update(LLTrans::getString("StartupInitializingTextureCache")); diff --git a/indra/newview/llappviewer.h b/indra/newview/llappviewer.h index 5e24398caa..00d6943047 100644 --- a/indra/newview/llappviewer.h +++ b/indra/newview/llappviewer.h @@ -58,6 +58,7 @@ class LLImageDecodeThread; class LLTextureFetch; class LLWatchdogTimeout; class LLViewerJoystick; +class LLPurgeDiskCacheThread; extern LLTrace::BlockTimerStatHandle FTM_FRAME; @@ -117,6 +118,7 @@ public: static LLTextureCache* getTextureCache() { return sTextureCache; } static LLImageDecodeThread* getImageDecodeThread() { return sImageDecodeThread; } static LLTextureFetch* getTextureFetch() { return sTextureFetch; } + static LLPurgeDiskCacheThread* getPurgeDiskCacheThread() { return sPurgeDiskCacheThread; } static U32 getTextureCacheVersion() ; static U32 getObjectCacheVersion() ; @@ -284,6 +286,7 @@ private: static LLTextureCache* sTextureCache; static LLImageDecodeThread* sImageDecodeThread; static LLTextureFetch* sTextureFetch; + static LLPurgeDiskCacheThread* sPurgeDiskCacheThread; S32 mNumSessions; From 89cf988aaf0de402641cd945e7e9ad5292bc78d6 Mon Sep 17 00:00:00 2001 From: Ansariel Date: Mon, 17 May 2021 09:49:32 +0200 Subject: [PATCH 2/6] BUG-230673: Add warning that LLDiskCache::purge() is also called from outside the main thread --- indra/llfilesystem/lldiskcache.cpp | 2 ++ indra/llfilesystem/lldiskcache.h | 3 +++ 2 files changed, 5 insertions(+) diff --git a/indra/llfilesystem/lldiskcache.cpp b/indra/llfilesystem/lldiskcache.cpp index 68423cc4f8..17906ce369 100644 --- a/indra/llfilesystem/lldiskcache.cpp +++ b/indra/llfilesystem/lldiskcache.cpp @@ -51,6 +51,8 @@ LLDiskCache::LLDiskCache(const std::string cache_dir, LLFile::mkdir(cache_dir); } +// WARNING: purge() is called by LLPurgeDiskCacheThread. As such it must +// NOT touch any LLDiskCache data without introducing and locking a mutex! void LLDiskCache::purge() { if (mEnableCacheDebugInfo) diff --git a/indra/llfilesystem/lldiskcache.h b/indra/llfilesystem/lldiskcache.h index 867a80f332..7c5b798f7e 100644 --- a/indra/llfilesystem/lldiskcache.h +++ b/indra/llfilesystem/lldiskcache.h @@ -118,6 +118,9 @@ class LLDiskCache : /** * Purge the oldest items in the cache so that the combined size of all files * is no bigger than mMaxSizeBytes. + * + * WARNING: purge() is called by LLPurgeDiskCacheThread. As such it must + * NOT touch any LLDiskCache data without introducing and locking a mutex! */ void purge(); From 87faf258911f5d23416500ff632050ce05b30e3e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 17 May 2021 10:24:27 -0400 Subject: [PATCH 3/6] SL-15200: Explain why purge() is called on another thread. Also add Ansariel's explanation for why interaction through the filesystem itself should be safe. --- indra/llfilesystem/lldiskcache.cpp | 28 +++++++++++++++++++++++++++- indra/llfilesystem/lldiskcache.h | 4 ++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/indra/llfilesystem/lldiskcache.cpp b/indra/llfilesystem/lldiskcache.cpp index 17906ce369..c0f10ac620 100644 --- a/indra/llfilesystem/lldiskcache.cpp +++ b/indra/llfilesystem/lldiskcache.cpp @@ -53,6 +53,32 @@ LLDiskCache::LLDiskCache(const std::string cache_dir, // WARNING: purge() is called by LLPurgeDiskCacheThread. As such it must // NOT touch any LLDiskCache data without introducing and locking a mutex! + +// Interaction through the filesystem itself should be safe. Let’s say thread +// A is accessing the cache file for reading/writing and thread B is trimming +// the cache. Let’s also assume using llifstream to open a file and +// boost::filesystem::remove are not atomic (which will be pretty much the +// case). + +// Now, A is trying to open the file using llifstream ctor. It does some +// checks if the file exists and whatever else it might be doing, but has not +// issued the call to the OS to actually open the file yet. Now B tries to +// delete the file: If the file has been already marked as in use by the OS, +// deleting the file will fail and B will continue with the next file. A can +// safely continue opening the file. If the file has not yet been marked as in +// use, B will delete the file. Now A actually wants to open it, operation +// will fail, subsequent check via llifstream.is_open will fail, asset will +// have to be re-requested. (Assuming here the viewer will actually handle +// this situation properly, that can also happen if there is a file containing +// garbage.) + +// Other situation: B is trimming the cache and A wants to read a file that is +// about to get deleted. boost::filesystem::remove does whatever it is doing +// before actually deleting the file. If A opens the file before the file is +// actually gone, the OS call from B to delete the file will fail since the OS +// will prevent this. B continues with the next file. If the file is already +// gone before A finally gets to open it, this operation will fail and the +// asset will have to be re-requested. void LLDiskCache::purge() { if (mEnableCacheDebugInfo) @@ -358,4 +384,4 @@ void LLPurgeDiskCacheThread::run() ms_sleep(100); } while (!isQuitting()); -} \ No newline at end of file +} diff --git a/indra/llfilesystem/lldiskcache.h b/indra/llfilesystem/lldiskcache.h index 7c5b798f7e..268fe92bcc 100644 --- a/indra/llfilesystem/lldiskcache.h +++ b/indra/llfilesystem/lldiskcache.h @@ -121,6 +121,10 @@ class LLDiskCache : * * WARNING: purge() is called by LLPurgeDiskCacheThread. As such it must * NOT touch any LLDiskCache data without introducing and locking a mutex! + * + * Purging the disk cache involves nontrivial work on the viewer's + * filesystem. If called on the main thread, this causes a noticeable + * freeze. */ void purge(); From d313d7021ff551f2a5ef7ec54ff2b3234d96017b Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 17 May 2021 13:37:13 -0400 Subject: [PATCH 4/6] SL-15200: Add LLApp::sleep(duration) methods. Two sleep() methods: one accepting F32Milliseconds, or in general any LLUnits time class; the other accepting any std::chrono::duration. The significant thing about each of these sleep() methods, as opposed to any freestanding sleep() function, is that it only sleeps until the app starts shutdown. Moreover, it returns true if it slept for the whole specified duration, false if it woke for app shutdown. This is accomplished by making LLApp::sStatus be an LLScalarCond instead of a plain EAppStatus enum, and by making setStatus() call set_all() each time the value changes. Then each new sleep() method can call wait_for_unequal(duration, APP_STATUS_RUNNING). Introducing llcond.h into llapp.h triggered an #include circularity because llthread.h #included llapp.h even though it didn't reference anything from it. Removed. This, in turn, necessitated adding #include "llapp.h" to several .cpp files that reference LLApp but had been depending on other header files to drag in llapp.h. --- indra/llcommon/llapp.cpp | 20 +++++++++---- indra/llcommon/llapp.h | 38 ++++++++++++++++++++++-- indra/llcommon/llthread.h | 1 - indra/llcommon/lluuid.cpp | 1 + indra/llcrashlogger/llcrashlogger.cpp | 2 +- indra/llmessage/message.cpp | 1 + indra/llplugin/llpluginprocessparent.cpp | 1 + 7 files changed, 53 insertions(+), 11 deletions(-) diff --git a/indra/llcommon/llapp.cpp b/indra/llcommon/llapp.cpp index a90b294550..34f6ba140a 100644 --- a/indra/llcommon/llapp.cpp +++ b/indra/llcommon/llapp.cpp @@ -113,7 +113,8 @@ BOOL LLApp::sDisableCrashlogger = FALSE; BOOL LLApp::sLogInSignal = FALSE; // static -LLApp::EAppStatus LLApp::sStatus = LLApp::APP_STATUS_STOPPED; // Keeps track of application status +// Keeps track of application status +LLScalarCond LLApp::sStatus{LLApp::APP_STATUS_STOPPED}; LLAppErrorHandler LLApp::sErrorHandler = NULL; BOOL LLApp::sErrorThreadRunning = FALSE; @@ -579,7 +580,8 @@ static std::map statusDesc // static void LLApp::setStatus(EAppStatus status) { - sStatus = status; + // notify everyone waiting on sStatus any time its value changes + sStatus.set_all(status); // This can also happen very late in the application lifecycle -- don't // resurrect a deleted LLSingleton @@ -609,6 +611,12 @@ void LLApp::setError() setStatus(APP_STATUS_ERROR); } +// static +bool LLApp::sleep(F32Milliseconds duration) +{ + return ! sStatus.wait_for_unequal(duration, APP_STATUS_RUNNING); +} + void LLApp::setMiniDumpDir(const std::string &path) { if (path.empty()) @@ -668,28 +676,28 @@ void LLApp::setStopped() // static bool LLApp::isStopped() { - return (APP_STATUS_STOPPED == sStatus); + return (APP_STATUS_STOPPED == sStatus.get()); } // static bool LLApp::isRunning() { - return (APP_STATUS_RUNNING == sStatus); + return (APP_STATUS_RUNNING == sStatus.get()); } // static bool LLApp::isError() { - return (APP_STATUS_ERROR == sStatus); + return (APP_STATUS_ERROR == sStatus.get()); } // static bool LLApp::isQuitting() { - return (APP_STATUS_QUITTING == sStatus); + return (APP_STATUS_QUITTING == sStatus.get()); } // static diff --git a/indra/llcommon/llapp.h b/indra/llcommon/llapp.h index 245c73e3a2..e95929b865 100644 --- a/indra/llcommon/llapp.h +++ b/indra/llcommon/llapp.h @@ -28,9 +28,11 @@ #define LL_LLAPP_H #include +#include "llcond.h" #include "llrun.h" #include "llsd.h" #include +#include // Forward declarations class LLErrorThread; class LLLiveFile; @@ -211,6 +213,36 @@ public: static bool isExiting(); // Either quitting or error (app is exiting, cleanly or not) static int getPid(); + // + // Sleep for specified time while still running + // + // For use by a coroutine or thread that performs some maintenance on a + // periodic basis. (See also LLEventTimer.) This method supports the + // pattern of an "infinite" loop that sleeps for some time, performs some + // action, then sleeps again. The trouble with literally sleeping a worker + // thread is that it could potentially sleep right through attempted + // application shutdown. This method avoids that by returning false as + // soon as the application status changes away from APP_STATUS_RUNNING + // (isRunning()). + // + // sleep() returns true if it sleeps undisturbed for the entire specified + // duration. The idea is that you can code 'while sleep(duration) ...', + // which will break the loop once shutdown begins. + // + // Since any time-based LLUnit should be implicitly convertible to + // F32Milliseconds, accept that specific type as a proxy. + static bool sleep(F32Milliseconds duration); + // Allow any duration defined in terms of . + // One can imagine a wonderfully general bidirectional conversion system + // between any type derived from LLUnits::LLUnit and + // any std::chrono::duration -- but that doesn't yet exist. + template + bool sleep(const std::chrono::duration& duration) + { + // wait_for_unequal() has the opposite bool return convention + return ! sStatus.wait_for_unequal(duration, APP_STATUS_RUNNING); + } + /** @name Error handling methods */ //@{ /** @@ -241,8 +273,8 @@ public: // Return the Google Breakpad minidump filename after a crash. char *getMiniDumpFilename() { return mMinidumpPath; } - std::string* getStaticDebugFile() { return &mStaticDebugFileName; } - std::string* getDynamicDebugFile() { return &mDynamicDebugFileName; } + std::string* getStaticDebugFile() { return &mStaticDebugFileName; } + std::string* getDynamicDebugFile() { return &mDynamicDebugFileName; } // Write out a Google Breakpad minidump file. void writeMiniDump(); @@ -266,7 +298,7 @@ public: protected: static void setStatus(EAppStatus status); // Use this to change the application status. - static EAppStatus sStatus; // Reflects current application status + static LLScalarCond sStatus; // Reflects current application status static BOOL sErrorThreadRunning; // Set while the error thread is running static BOOL sDisableCrashlogger; // Let the OS handle crashes for us. std::wstring mCrashReportPipeStr; //Name of pipe to use for crash reporting. diff --git a/indra/llcommon/llthread.h b/indra/llcommon/llthread.h index 5cd0731f6c..50202631e7 100644 --- a/indra/llcommon/llthread.h +++ b/indra/llcommon/llthread.h @@ -27,7 +27,6 @@ #ifndef LL_LLTHREAD_H #define LL_LLTHREAD_H -#include "llapp.h" #include "llapr.h" #include "boost/intrusive_ptr.hpp" #include "llrefcount.h" diff --git a/indra/llcommon/lluuid.cpp b/indra/llcommon/lluuid.cpp index b05630c6b5..b4879c2489 100644 --- a/indra/llcommon/lluuid.cpp +++ b/indra/llcommon/lluuid.cpp @@ -33,6 +33,7 @@ #include #endif +#include "llapp.h" #include "lldefs.h" #include "llerror.h" diff --git a/indra/llcrashlogger/llcrashlogger.cpp b/indra/llcrashlogger/llcrashlogger.cpp index 62fcdaf545..24bf91b595 100644 --- a/indra/llcrashlogger/llcrashlogger.cpp +++ b/indra/llcrashlogger/llcrashlogger.cpp @@ -595,7 +595,7 @@ bool LLCrashLogger::init() #if LL_WINDOWS Sleep(1000); #else - sleep(1); + ::sleep(1); #endif locked = mKeyMaster.checkMaster(); } diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp index da62bb12e8..19146c64f4 100644 --- a/indra/llmessage/message.cpp +++ b/indra/llmessage/message.cpp @@ -46,6 +46,7 @@ #include "apr_poll.h" // linden library headers +#include "llapp.h" #include "indra_constants.h" #include "lldir.h" #include "llerror.h" diff --git a/indra/llplugin/llpluginprocessparent.cpp b/indra/llplugin/llpluginprocessparent.cpp index 7d18bae947..a89561fce0 100644 --- a/indra/llplugin/llpluginprocessparent.cpp +++ b/indra/llplugin/llpluginprocessparent.cpp @@ -28,6 +28,7 @@ #include "linden_common.h" +#include "llapp.h" #include "llpluginprocessparent.h" #include "llpluginmessagepipe.h" #include "llpluginmessageclasses.h" From b3708ac238d51eaf808cb77a4493e518c1593e33 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Mon, 17 May 2021 15:10:06 -0400 Subject: [PATCH 5/6] SL-15200: Use new LLApp::sleep() in LLPurgeDiskCacheThread::run(). --- indra/llfilesystem/lldiskcache.cpp | 16 +++++----------- indra/llfilesystem/lldiskcache.h | 3 --- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/indra/llfilesystem/lldiskcache.cpp b/indra/llfilesystem/lldiskcache.cpp index c0f10ac620..6b0bbaeb48 100644 --- a/indra/llfilesystem/lldiskcache.cpp +++ b/indra/llfilesystem/lldiskcache.cpp @@ -31,6 +31,7 @@ */ #include "linden_common.h" +#include "llapp.h" #include "llassettype.h" #include "lldir.h" #include @@ -371,17 +372,10 @@ LLPurgeDiskCacheThread::LLPurgeDiskCacheThread() : void LLPurgeDiskCacheThread::run() { - constexpr F64 CHECK_INTERVAL = 60; - mTimer.setTimerExpirySec(CHECK_INTERVAL); - mTimer.start(); + constexpr long CHECK_INTERVAL = 60; - do + while (LLApp::instance()->sleep(std::chrono::seconds(CHECK_INTERVAL))) { - if (mTimer.checkExpirationAndReset(CHECK_INTERVAL)) - { - LLDiskCache::instance().purge(); - } - - ms_sleep(100); - } while (!isQuitting()); + LLDiskCache::instance().purge(); + } } diff --git a/indra/llfilesystem/lldiskcache.h b/indra/llfilesystem/lldiskcache.h index 268fe92bcc..1cbd2c58aa 100644 --- a/indra/llfilesystem/lldiskcache.h +++ b/indra/llfilesystem/lldiskcache.h @@ -194,8 +194,5 @@ public: protected: void run() override; - -private: - LLTimer mTimer; }; #endif // _LLDISKCACHE From ac8640d338997020ca0650001ff004e1103ac5cb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 18 May 2021 09:51:45 -0400 Subject: [PATCH 6/6] SL-15200: LLPurgeDiskCacheThread's CHECK_INTERVAL is secs. --- indra/llfilesystem/lldiskcache.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/indra/llfilesystem/lldiskcache.cpp b/indra/llfilesystem/lldiskcache.cpp index 6b0bbaeb48..905351886f 100644 --- a/indra/llfilesystem/lldiskcache.cpp +++ b/indra/llfilesystem/lldiskcache.cpp @@ -372,9 +372,9 @@ LLPurgeDiskCacheThread::LLPurgeDiskCacheThread() : void LLPurgeDiskCacheThread::run() { - constexpr long CHECK_INTERVAL = 60; + constexpr std::chrono::seconds CHECK_INTERVAL{60}; - while (LLApp::instance()->sleep(std::chrono::seconds(CHECK_INTERVAL))) + while (LLApp::instance()->sleep(CHECK_INTERVAL)) { LLDiskCache::instance().purge(); }