From 2b407a99952f766fd041a5f8b9efaa5cc2b579e8 Mon Sep 17 00:00:00 2001 From: scrawl Date: Sat, 23 Aug 2014 17:48:11 +0200 Subject: [PATCH] Refactor NIF cache - Remove broken cache locking mechanism This was supposed to unload NIFFiles after a cell transition completes, but it was never working due to a mistake on the line if (--sLockLevel), should have been if (--sLockLevel == 0). Repairing this would increase load times (NIF files would have to be reloaded more frequently), so just removed it for now. - Decouple cache from NIFFile (now a new nifcache component) - Add API for future background loading - Provide a reliable way (SharedPtr) to hold on to loaded NIFFiles. This will be useful to avoid deep copies of keyframe and text key data, which is currently a performance bottleneck. --- apps/opencs/editor.hpp | 3 + apps/openmw/engine.cpp | 3 - apps/openmw/engine.hpp | 4 + apps/openmw/mwworld/scene.cpp | 3 - components/CMakeLists.txt | 4 + components/nif/niffile.cpp | 136 +---------------------- components/nif/niffile.hpp | 19 +--- components/nifbullet/bulletnifloader.cpp | 6 +- components/nifcache/nifcache.cpp | 40 +++++++ components/nifcache/nifcache.hpp | 48 ++++++++ components/nifogre/mesh.cpp | 3 +- components/nifogre/ogrenifloader.cpp | 5 +- components/nifogre/skeleton.cpp | 3 +- 13 files changed, 112 insertions(+), 165 deletions(-) create mode 100644 components/nifcache/nifcache.cpp create mode 100644 components/nifcache/nifcache.hpp diff --git a/apps/opencs/editor.hpp b/apps/opencs/editor.hpp index d88da9865..c87e24e77 100644 --- a/apps/opencs/editor.hpp +++ b/apps/opencs/editor.hpp @@ -16,6 +16,8 @@ #include +#include + #include "model/settings/usersettings.hpp" #include "model/doc/documentmanager.hpp" @@ -37,6 +39,7 @@ namespace CS { Q_OBJECT + Nif::Cache mNifCache; Files::ConfigurationManager mCfgMgr; CSMSettings::UserSettings mUserSettings; CSMDoc::DocumentManager mDocumentManager; diff --git a/apps/openmw/engine.cpp b/apps/openmw/engine.cpp index 2e5689fa8..21d0a2511 100644 --- a/apps/openmw/engine.cpp +++ b/apps/openmw/engine.cpp @@ -15,7 +15,6 @@ #include #include #include -#include #include #include @@ -315,8 +314,6 @@ void OMW::Engine::prepareEngine (Settings::Manager & settings) mEnvironment.setStateManager ( new MWState::StateManager (mCfgMgr.getUserDataPath() / "saves", mContentFiles.at (0))); - Nif::NIFFile::CacheLock cachelock; - std::string renderSystem = settings.getString("render system", "Video"); if (renderSystem == "") { diff --git a/apps/openmw/engine.hpp b/apps/openmw/engine.hpp index 10c1f6ff2..e5f3e5f99 100644 --- a/apps/openmw/engine.hpp +++ b/apps/openmw/engine.hpp @@ -7,6 +7,8 @@ #include #include #include +#include + #include "mwbase/environment.hpp" @@ -94,6 +96,8 @@ namespace OMW std::vector mScriptBlacklist; bool mScriptBlacklistUse; + Nif::Cache mNifCache; + // not implemented Engine (const Engine&); Engine& operator= (const Engine&); diff --git a/apps/openmw/mwworld/scene.cpp b/apps/openmw/mwworld/scene.cpp index ee107e1f2..ee5f97d72 100644 --- a/apps/openmw/mwworld/scene.cpp +++ b/apps/openmw/mwworld/scene.cpp @@ -259,8 +259,6 @@ namespace MWWorld void Scene::changeCell (int X, int Y, const ESM::Position& position, bool adjustPlayerPos) { - Nif::NIFFile::CacheLock cachelock; - Loading::Listener* loadingListener = MWBase::Environment::get().getWindowManager()->getLoadingScreen(); Loading::ScopedLoad load(loadingListener); @@ -408,7 +406,6 @@ namespace MWWorld if(!loadcell) loadcell = *mCurrentCell != *cell; - Nif::NIFFile::CacheLock lock; MWBase::Environment::get().getWindowManager()->fadeScreenOut(0.5); Loading::Listener* loadingListener = MWBase::Environment::get().getWindowManager()->getLoadingScreen(); diff --git a/components/CMakeLists.txt b/components/CMakeLists.txt index 96ccc281a..897e87735 100644 --- a/components/CMakeLists.txt +++ b/components/CMakeLists.txt @@ -22,6 +22,10 @@ add_component_dir (nif controlled effect niftypes record controller extra node record_ptr data niffile property ) +add_component_dir (nifcache + nifcache + ) + add_component_dir (nifogre ogrenifloader skeleton material mesh particles controller ) diff --git a/components/nif/niffile.cpp b/components/nif/niffile.cpp index 84f4aacee..6c38f73dc 100644 --- a/components/nif/niffile.cpp +++ b/components/nif/niffile.cpp @@ -35,143 +35,11 @@ #include -//TODO: when threading is needed, enable these -//#include -#include - namespace Nif { -class NIFFile::LoadedCache -{ - //TODO: enable this to make cache thread safe... - //typedef boost::mutex mutex; - - struct mutex - { - void lock () {}; - void unlock () {} - }; - - typedef boost::lock_guard lock_guard; - typedef std::map < std::string, boost::weak_ptr > loaded_map; - typedef std::vector < boost::shared_ptr > locked_files; - - static int sLockLevel; - static mutex sProtector; - static loaded_map sLoadedMap; - static locked_files sLockedFiles; - -public: - - static ptr create (const std::string &name) - { - lock_guard _ (sProtector); - - ptr result; - - // lookup the resource - loaded_map::iterator i = sLoadedMap.find (name); - - if (i == sLoadedMap.end ()) // it doesn't existing currently, - { // or hasn't in the very near past - - // create it now, for smoother threading if needed, the - // loading should be performed outside of the sLoaderMap - // lock and an alternate mechanism should be used to - // synchronize threads competing to load the same resource - result = boost::make_shared (name, psudo_private_modifier()); - - // if we are locking the cache add an extra reference - // to keep the file in memory - if (sLockLevel > 0) - sLockedFiles.push_back (result); - - // stash a reference to the resource so that future - // calls can benefit - sLoadedMap [name] = boost::weak_ptr (result); - } - else // it may (probably) still exists - { - // attempt to get the reference - result = i->second.lock (); - - if (!result) // resource is in the process of being destroyed - { - // create a new instance, to replace the one that has - // begun the irreversible process of being destroyed - result = boost::make_shared (name, psudo_private_modifier()); - - // respect the cache lock... - if (sLockLevel > 0) - sLockedFiles.push_back (result); - - // we potentially overwrite an expired pointer here - // but the other thread performing the delete on - // the previous copy of this resource will detect it - // and make sure not to erase the new reference - sLoadedMap [name] = boost::weak_ptr (result); - } - } - - // we made it! - return result; - } - - static void release (NIFFile * file) - { - lock_guard _ (sProtector); - - loaded_map::iterator i = sLoadedMap.find (file->filename); - - // its got to be in here, it just might not be us... - assert (i != sLoadedMap.end ()); - - // if weak_ptr is still expired, this resource hasn't been recreated - // between the initiation of the final release due to destruction - // of the last shared pointer and this thread acquiring the lock on - // the loader map - if (i->second.expired ()) - sLoadedMap.erase (i); - } - - static void lockCache () - { - lock_guard _ (sProtector); - - sLockLevel++; - } - - static void unlockCache () - { - locked_files resetList; - - { - lock_guard _ (sProtector); - - if (--sLockLevel) - sLockedFiles.swap(resetList); - } - - // this not necessary, but makes it clear that the - // deletion of the locked cache entries is being done - // outside the protection of sProtector - resetList.clear (); - } -}; - -int NIFFile::LoadedCache::sLockLevel = 0; -NIFFile::LoadedCache::mutex NIFFile::LoadedCache::sProtector; -NIFFile::LoadedCache::loaded_map NIFFile::LoadedCache::sLoadedMap; -NIFFile::LoadedCache::locked_files NIFFile::LoadedCache::sLockedFiles; - -// these three calls are forwarded to the cache implementation... -void NIFFile::lockCache () { LoadedCache::lockCache (); } -void NIFFile::unlockCache () { LoadedCache::unlockCache (); } -NIFFile::ptr NIFFile::create (const std::string &name) { return LoadedCache::create (name); } - /// Open a NIF stream. The name is used for error messages. -NIFFile::NIFFile(const std::string &name, psudo_private_modifier) +NIFFile::NIFFile(const std::string &name) : ver(0) , filename(name) { @@ -180,8 +48,6 @@ NIFFile::NIFFile(const std::string &name, psudo_private_modifier) NIFFile::~NIFFile() { - LoadedCache::release (this); - for(std::size_t i=0; i ptr; - /// Open a NIF stream. The name is used for error messages. - NIFFile(const std::string &name, psudo_private_modifier); + NIFFile(const std::string &name); ~NIFFile(); - static ptr create (const std::string &name); - static void lockCache (); - static void unlockCache (); - - struct CacheLock - { - CacheLock () { lockCache (); } - ~CacheLock () { unlockCache (); } - }; - /// Get a given record Record *getRecord(size_t index) const { diff --git a/components/nifbullet/bulletnifloader.cpp b/components/nifbullet/bulletnifloader.cpp index 31d4e10d6..799ae32df 100644 --- a/components/nifbullet/bulletnifloader.cpp +++ b/components/nifbullet/bulletnifloader.cpp @@ -28,6 +28,8 @@ http://www.gnu.org/licenses/ . #include +#include + #include "../nif/niffile.hpp" #include "../nif/node.hpp" #include "../nif/data.hpp" @@ -74,7 +76,7 @@ void ManualBulletShapeLoader::loadResource(Ogre::Resource *resource) // of the early stages of development. Right now we WANT to catch // every error as early and intrusively as possible, as it's most // likely a sign of incomplete code rather than faulty input. - Nif::NIFFile::ptr pnif (Nif::NIFFile::create (mResourceName.substr(0, mResourceName.length()-7))); + Nif::NIFFilePtr pnif (Nif::Cache::getInstance().load(mResourceName.substr(0, mResourceName.length()-7))); Nif::NIFFile & nif = *pnif.get (); if (nif.numRoots() < 1) { @@ -388,7 +390,7 @@ bool findBoundingBox (const Nif::Node* node, Ogre::Vector3& halfExtents, Ogre::V bool getBoundingBox(const std::string& nifFile, Ogre::Vector3& halfExtents, Ogre::Vector3& translation, Ogre::Quaternion& orientation) { - Nif::NIFFile::ptr pnif (Nif::NIFFile::create (nifFile)); + Nif::NIFFilePtr pnif (Nif::Cache::getInstance().load(nifFile)); Nif::NIFFile & nif = *pnif.get (); if (nif.numRoots() < 1) diff --git a/components/nifcache/nifcache.cpp b/components/nifcache/nifcache.cpp new file mode 100644 index 000000000..342251dbc --- /dev/null +++ b/components/nifcache/nifcache.cpp @@ -0,0 +1,40 @@ +#include "nifcache.hpp" + +namespace Nif +{ + +Cache* Cache::sThis = 0; + +Cache& Cache::getInstance() +{ + assert (sThis); + return *sThis; +} + +Cache* Cache::getInstancePtr() +{ + return sThis; +} + +Cache::Cache() +{ + assert (!sThis); + sThis = this; +} + +NIFFilePtr Cache::load(const std::string &filename) +{ + // TODO: normalize file path to make sure we're not loading the same file twice + + LoadedMap::iterator it = mLoadedMap.find(filename); + if (it != mLoadedMap.end()) + return it->second; + else + { + NIFFilePtr file(new Nif::NIFFile(filename)); + mLoadedMap[filename] = file; + return file; + } +} + +} diff --git a/components/nifcache/nifcache.hpp b/components/nifcache/nifcache.hpp new file mode 100644 index 000000000..dc4155cad --- /dev/null +++ b/components/nifcache/nifcache.hpp @@ -0,0 +1,48 @@ +#ifndef OPENMW_COMPONENTS_NIFCACHE_H +#define OPENMW_COMPONENTS_NIFCACHE_H + +#include + +#include + +namespace Nif +{ + + typedef boost::shared_ptr NIFFilePtr; + + /// @brief A basic resource manager for NIF files + class Cache + { + public: + Cache(); + + /// Queue this file for background loading. A worker thread will start loading the file. + /// To get the loaded NIFFilePtr, use the load method, which will wait until the worker thread is finished + /// and then return the loaded file. + //void loadInBackground (const std::string& file); + + /// Read and parse the given file. May retrieve from cache if this file has been used previously. + /// @note If the file is currently loading in the background, this function will block until + /// the background loading finishes, then return the background loaded file. + /// @note Returns a SharedPtr to the file and the file will stay loaded as long as the user holds on to this pointer. + /// When all external SharedPtrs to a file are released, the cache may decide to unload the file. + NIFFilePtr load (const std::string& filename); + + /// Return instance of this class. + static Cache& getInstance(); + static Cache* getInstancePtr(); + + private: + static Cache* sThis; + + Cache(const Cache&); + Cache& operator =(const Cache&); + + typedef std::map LoadedMap; + + LoadedMap mLoadedMap; + }; + +} + +#endif diff --git a/components/nifogre/mesh.cpp b/components/nifogre/mesh.cpp index 8bebe0589..af73df637 100644 --- a/components/nifogre/mesh.cpp +++ b/components/nifogre/mesh.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include "material.hpp" @@ -383,7 +384,7 @@ void NIFMeshLoader::loadResource(Ogre::Resource *resource) Ogre::Mesh *mesh = dynamic_cast(resource); OgreAssert(mesh, "Attempting to load a mesh into a non-mesh resource!"); - Nif::NIFFile::ptr nif = Nif::NIFFile::create(mName); + Nif::NIFFilePtr nif = Nif::Cache::getInstance().load(mName); if(mShapeIndex >= nif->numRecords()) { Ogre::SkeletonManager *skelMgr = Ogre::SkeletonManager::getSingletonPtr(); diff --git a/components/nifogre/ogrenifloader.cpp b/components/nifogre/ogrenifloader.cpp index 5d55ea307..aca27d82a 100644 --- a/components/nifogre/ogrenifloader.cpp +++ b/components/nifogre/ogrenifloader.cpp @@ -45,6 +45,7 @@ #include #include +#include #include #include @@ -1123,7 +1124,7 @@ class NIFObjectLoader public: static void load(Ogre::SceneNode *sceneNode, ObjectScenePtr scene, const std::string &name, const std::string &group, int flags=0) { - Nif::NIFFile::ptr nif = Nif::NIFFile::create(name); + Nif::NIFFilePtr nif = Nif::Cache::getInstance().load(name); if(nif->numRoots() < 1) { nif->warn("Found no root nodes in "+name+"."); @@ -1153,7 +1154,7 @@ public: static void loadKf(Ogre::Skeleton *skel, const std::string &name, TextKeyMap &textKeys, std::vector > &ctrls) { - Nif::NIFFile::ptr nif = Nif::NIFFile::create(name); + Nif::NIFFilePtr nif = Nif::Cache::getInstance().load(name); if(nif->numRoots() < 1) { nif->warn("Found no root nodes in "+name+"."); diff --git a/components/nifogre/skeleton.cpp b/components/nifogre/skeleton.cpp index c96f03950..9e12eec90 100644 --- a/components/nifogre/skeleton.cpp +++ b/components/nifogre/skeleton.cpp @@ -6,6 +6,7 @@ #include #include +#include #include namespace NifOgre @@ -83,7 +84,7 @@ void NIFSkeletonLoader::loadResource(Ogre::Resource *resource) Ogre::Skeleton *skel = dynamic_cast(resource); OgreAssert(skel, "Attempting to load a skeleton into a non-skeleton resource!"); - Nif::NIFFile::ptr nif(Nif::NIFFile::create(skel->getName())); + Nif::NIFFilePtr nif(Nif::Cache::getInstance().load(skel->getName())); const Nif::Node *node = static_cast(nif->getRoot(0)); try {