From 7b7bf9311842564375b4872c8b91da3a0d361c1c Mon Sep 17 00:00:00 2001 From: rdb Date: Tue, 12 Jul 2016 11:24:09 +0200 Subject: [PATCH 1/2] Fix frame chug when loading big model asynchronously (LP #1019599) --- doc/ReleaseNotes | 1 + panda/src/putil/bamCache.cxx | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/doc/ReleaseNotes b/doc/ReleaseNotes index 3796b840c0..a4ca29a6cf 100644 --- a/doc/ReleaseNotes +++ b/doc/ReleaseNotes @@ -25,6 +25,7 @@ This issue fixes several bugs that were still found in 1.9.2. * Work around Cg bug generating invalid ASM for saturated tex loads * Fix issues with certain Cg shader inputs in DX9 * Support uint8 index buffers in DX9 +* Fix occasional frame lag when loading a big model asynchronously ------------------------ RELEASE 1.9.2 ------------------------ diff --git a/panda/src/putil/bamCache.cxx b/panda/src/putil/bamCache.cxx index 7616350e07..f0d04d08c8 100644 --- a/panda/src/putil/bamCache.cxx +++ b/panda/src/putil/bamCache.cxx @@ -321,13 +321,24 @@ emergency_read_only() { //////////////////////////////////////////////////////////////////// void BamCache:: consider_flush_index() { - ReMutexHolder holder(_lock); +#if defined(HAVE_THREADS) || defined(DEBUG_THREADS) + if (!_lock.try_acquire()) { + // If we can't grab the lock, no big deal. We don't want to hold up + // the frame waiting for a cache operation. We can try again later. + return; + } +#endif + if (_index_stale_since != 0) { int elapsed = (int)time(NULL) - (int)_index_stale_since; if (elapsed > _flush_time) { flush_index(); } } + +#if defined(HAVE_THREADS) || defined(DEBUG_THREADS) + _lock.release(); +#endif } //////////////////////////////////////////////////////////////////// From 69f15258e4394f8df680f4a6a71de2cbca7bf770 Mon Sep 17 00:00:00 2001 From: rdb Date: Tue, 12 Jul 2016 14:47:20 +0200 Subject: [PATCH 2/2] Fix race condition reading filename/string from config var --- doc/ReleaseNotes | 1 + dtool/src/prc/configVariableFilename.cxx | 30 +++++++++++++++++------- dtool/src/prc/configVariableString.I | 3 +-- dtool/src/prc/configVariableString.cxx | 23 ++++++++++++++++++ dtool/src/prc/configVariableString.h | 3 +++ 5 files changed, 49 insertions(+), 11 deletions(-) diff --git a/doc/ReleaseNotes b/doc/ReleaseNotes index a4ca29a6cf..94ebbe4e9e 100644 --- a/doc/ReleaseNotes +++ b/doc/ReleaseNotes @@ -26,6 +26,7 @@ This issue fixes several bugs that were still found in 1.9.2. * Fix issues with certain Cg shader inputs in DX9 * Support uint8 index buffers in DX9 * Fix occasional frame lag when loading a big model asynchronously +* Fix race condition reading string config var ------------------------ RELEASE 1.9.2 ------------------------ diff --git a/dtool/src/prc/configVariableFilename.cxx b/dtool/src/prc/configVariableFilename.cxx index dd9a4ab3ef..2b898f5b11 100644 --- a/dtool/src/prc/configVariableFilename.cxx +++ b/dtool/src/prc/configVariableFilename.cxx @@ -23,16 +23,28 @@ //////////////////////////////////////////////////////////////////// void ConfigVariableFilename:: reload_cache() { - nassertv(_core != (ConfigVariableCore *)NULL); - mark_cache_valid(_local_modified); + // NB. MSVC doesn't guarantee that this mutex is initialized in a + // thread-safe manner. But chances are that the first time this is called + // is at static init time, when there is no risk of data races. + static MutexImpl lock; + lock.acquire(); - const ConfigDeclaration *decl = _core->get_declaration(0); - const ConfigPage *page = decl->get_page(); + // We check again for cache validity since another thread may have beaten + // us to the punch while we were waiting for the lock. + if (!is_cache_valid(_local_modified)) { + nassertv(_core != (ConfigVariableCore *)NULL); - Filename page_filename(page->get_name()); - Filename page_dirname = page_filename.get_dirname(); - ExecutionEnvironment::shadow_environment_variable("THIS_PRC_DIR", page_dirname.to_os_specific()); + const ConfigDeclaration *decl = _core->get_declaration(0); + const ConfigPage *page = decl->get_page(); - _cache = Filename::expand_from(decl->get_string_value()); - ExecutionEnvironment::clear_shadow("THIS_PRC_DIR"); + Filename page_filename(page->get_name()); + Filename page_dirname = page_filename.get_dirname(); + ExecutionEnvironment::shadow_environment_variable("THIS_PRC_DIR", page_dirname.to_os_specific()); + + _cache = Filename::expand_from(decl->get_string_value()); + ExecutionEnvironment::clear_shadow("THIS_PRC_DIR"); + + mark_cache_valid(_local_modified); + } + lock.release(); } diff --git a/dtool/src/prc/configVariableString.I b/dtool/src/prc/configVariableString.I index 580ad91fd3..2982b2ca24 100644 --- a/dtool/src/prc/configVariableString.I +++ b/dtool/src/prc/configVariableString.I @@ -155,8 +155,7 @@ INLINE const string &ConfigVariableString:: get_value() const { TAU_PROFILE("const string &ConfigVariableString::get_value() const", " ", TAU_USER); if (!is_cache_valid(_local_modified)) { - mark_cache_valid(((ConfigVariableString *)this)->_local_modified); - ((ConfigVariableString *)this)->_cache = get_string_value(); + ((ConfigVariableString *)this)->reload_cache(); } return _cache; } diff --git a/dtool/src/prc/configVariableString.cxx b/dtool/src/prc/configVariableString.cxx index e1b7796f3f..7f4c3048bf 100644 --- a/dtool/src/prc/configVariableString.cxx +++ b/dtool/src/prc/configVariableString.cxx @@ -13,3 +13,26 @@ //////////////////////////////////////////////////////////////////// #include "configVariableString.h" + +//////////////////////////////////////////////////////////////////// +// Function: ConfigVariableString::reload_cache +// Access: Published +// Description: Refreshes the variable's cached value. +//////////////////////////////////////////////////////////////////// +void ConfigVariableString:: +reload_cache() { + // NB. MSVC doesn't guarantee that this mutex is initialized in a + // thread-safe manner. But chances are that the first time this is called + // is at static init time, when there is no risk of data races. + static MutexImpl lock; + lock.acquire(); + + // We check again for cache validity since another thread may have beaten + // us to the punch while we were waiting for the lock. + if (!is_cache_valid(_local_modified)) { + _cache = get_string_value(); + mark_cache_valid(_local_modified); + } + + lock.release(); +} diff --git a/dtool/src/prc/configVariableString.h b/dtool/src/prc/configVariableString.h index 0685af8a67..f8b4e51c9d 100644 --- a/dtool/src/prc/configVariableString.h +++ b/dtool/src/prc/configVariableString.h @@ -51,6 +51,9 @@ PUBLISHED: INLINE string get_word(int n) const; INLINE void set_word(int n, const string &value); +private: + void reload_cache(); + private: AtomicAdjust::Integer _local_modified; string _cache;