From a854a6e04a8437d74078906d1d1720c0649e4e6f Mon Sep 17 00:00:00 2001 From: Bo Svensson <90132211+bosvensson1@users.noreply.github.com> Date: Wed, 20 Oct 2021 21:02:15 +0000 Subject: [PATCH] removes UnrefQueue (#3181) Currently, we use an `UnrefQueue` which supposedly aims to transfer destruction costs to another thread. The implications of this unusual pattern can not be well understood because some allocators might free resources more efficiently if they are freed by the same thread that allocated them. In addition, `UnrefQueue` complicates the validation of thread safety in our engine. Lastly, our current usage of `UnrefQueue` triggers `ref()`, `unref()` atomic operations as objects are passed into the queue. These operations could be more expensive than the actual destruction. With this PR we thus remove `UnrefQueue`. We can expect these changes to have a minor impact at most because we free most resources elsewhere in `ResourceSystem::updateCache`. --- apps/openmw/mwphysics/physicssystem.cpp | 9 ----- apps/openmw/mwphysics/physicssystem.hpp | 9 ----- apps/openmw/mwrender/objects.cpp | 12 +----- apps/openmw/mwrender/objects.hpp | 9 +---- apps/openmw/mwrender/renderingmanager.cpp | 14 +------ apps/openmw/mwrender/renderingmanager.hpp | 3 -- apps/openmw/mwworld/cellpreloader.cpp | 17 ++------ apps/openmw/mwworld/cellpreloader.hpp | 8 ---- apps/openmw/mwworld/scene.cpp | 6 --- components/sceneutil/unrefqueue.cpp | 39 ------------------ components/sceneutil/unrefqueue.hpp | 44 --------------------- components/terrain/compositemaprenderer.cpp | 17 -------- components/terrain/compositemaprenderer.hpp | 12 ------ components/terrain/world.cpp | 5 --- components/terrain/world.hpp | 8 ---- 15 files changed, 6 insertions(+), 206 deletions(-) delete mode 100644 components/sceneutil/unrefqueue.cpp delete mode 100644 components/sceneutil/unrefqueue.hpp diff --git a/apps/openmw/mwphysics/physicssystem.cpp b/apps/openmw/mwphysics/physicssystem.cpp index 5f7ee0523a..6c46fd6d05 100644 --- a/apps/openmw/mwphysics/physicssystem.cpp +++ b/apps/openmw/mwphysics/physicssystem.cpp @@ -24,7 +24,6 @@ #include #include #include -#include #include #include // FindRecIndexVisitor @@ -160,11 +159,6 @@ namespace MWPhysics mProjectiles.clear(); } - void PhysicsSystem::setUnrefQueue(SceneUtil::UnrefQueue *unrefQueue) - { - mUnrefQueue = unrefQueue; - } - Resource::BulletShapeManager *PhysicsSystem::getShapeManager() { return mShapeManager.get(); @@ -513,9 +507,6 @@ namespace MWPhysics { if (auto foundObject = mObjects.find(ptr.mRef); foundObject != mObjects.end()) { - if (mUnrefQueue.get()) - mUnrefQueue->push(foundObject->second->getShapeInstance()); - mAnimatedObjects.erase(foundObject->second.get()); mObjects.erase(foundObject); diff --git a/apps/openmw/mwphysics/physicssystem.hpp b/apps/openmw/mwphysics/physicssystem.hpp index 30fdbcc122..6ec4ebfda9 100644 --- a/apps/openmw/mwphysics/physicssystem.hpp +++ b/apps/openmw/mwphysics/physicssystem.hpp @@ -36,11 +36,6 @@ namespace Resource class ResourceSystem; } -namespace SceneUtil -{ - class UnrefQueue; -} - class btCollisionWorld; class btBroadphaseInterface; class btDefaultCollisionConfiguration; @@ -118,8 +113,6 @@ namespace MWPhysics PhysicsSystem (Resource::ResourceSystem* resourceSystem, osg::ref_ptr parentNode); virtual ~PhysicsSystem (); - void setUnrefQueue(SceneUtil::UnrefQueue* unrefQueue); - Resource::BulletShapeManager* getShapeManager(); void enableWater(float height); @@ -262,8 +255,6 @@ namespace MWPhysics std::pair>, std::vector> prepareFrameData(bool willSimulate); - osg::ref_ptr mUnrefQueue; - std::unique_ptr mBroadphase; std::unique_ptr mCollisionConfiguration; std::unique_ptr mDispatcher; diff --git a/apps/openmw/mwrender/objects.cpp b/apps/openmw/mwrender/objects.cpp index ec1c4397bf..f51008fffa 100644 --- a/apps/openmw/mwrender/objects.cpp +++ b/apps/openmw/mwrender/objects.cpp @@ -4,7 +4,6 @@ #include #include -#include #include "../mwworld/ptr.hpp" #include "../mwworld/class.hpp" @@ -18,10 +17,9 @@ namespace MWRender { -Objects::Objects(Resource::ResourceSystem* resourceSystem, osg::ref_ptr rootNode, SceneUtil::UnrefQueue* unrefQueue) +Objects::Objects(Resource::ResourceSystem* resourceSystem, osg::ref_ptr rootNode) : mRootNode(rootNode) , mResourceSystem(resourceSystem) - , mUnrefQueue(unrefQueue) { } @@ -117,9 +115,6 @@ bool Objects::removeObject (const MWWorld::Ptr& ptr) PtrAnimationMap::iterator iter = mObjects.find(ptr); if(iter != mObjects.end()) { - if (mUnrefQueue.get()) - mUnrefQueue->push(iter->second); - mObjects.erase(iter); if (ptr.getClass().isActor()) @@ -146,9 +141,6 @@ void Objects::removeCell(const MWWorld::CellStore* store) MWWorld::Ptr ptr = iter->second->getPtr(); if(ptr.getCell() == store) { - if (mUnrefQueue.get()) - mUnrefQueue->push(iter->second); - if (ptr.getClass().isNpc() && ptr.getRefData().getCustomData()) { MWWorld::InventoryStore& invStore = ptr.getClass().getInventoryStore(ptr); @@ -166,8 +158,6 @@ void Objects::removeCell(const MWWorld::CellStore* store) if(cell != mCellSceneNodes.end()) { cell->second->getParent(0)->removeChild(cell->second); - if (mUnrefQueue.get()) - mUnrefQueue->push(cell->second); mCellSceneNodes.erase(cell); } } diff --git a/apps/openmw/mwrender/objects.hpp b/apps/openmw/mwrender/objects.hpp index 98ebb95d98..5f8b7dfc9e 100644 --- a/apps/openmw/mwrender/objects.hpp +++ b/apps/openmw/mwrender/objects.hpp @@ -24,11 +24,6 @@ namespace MWWorld class CellStore; } -namespace SceneUtil -{ - class UnrefQueue; -} - namespace MWRender{ class Animation; @@ -66,12 +61,10 @@ class Objects{ Resource::ResourceSystem* mResourceSystem; - osg::ref_ptr mUnrefQueue; - void insertBegin(const MWWorld::Ptr& ptr); public: - Objects(Resource::ResourceSystem* resourceSystem, osg::ref_ptr rootNode, SceneUtil::UnrefQueue* unrefQueue); + Objects(Resource::ResourceSystem* resourceSystem, osg::ref_ptr rootNode); ~Objects(); /// @param animated Attempt to load separate keyframes from a .kf file matching the model file? diff --git a/apps/openmw/mwrender/renderingmanager.cpp b/apps/openmw/mwrender/renderingmanager.cpp index 14000273db..1f29c45182 100644 --- a/apps/openmw/mwrender/renderingmanager.cpp +++ b/apps/openmw/mwrender/renderingmanager.cpp @@ -37,7 +37,6 @@ #include #include #include -#include #include #include @@ -289,7 +288,6 @@ namespace MWRender , mRootNode(rootNode) , mResourceSystem(resourceSystem) , mWorkQueue(workQueue) - , mUnrefQueue(new SceneUtil::UnrefQueue) , mNavigator(navigator) , mMinimumAmbientLuminance(0.f) , mNightEyeFactor(0.f) @@ -387,7 +385,7 @@ namespace MWRender mRecastMesh.reset(new RecastMesh(mRootNode, Settings::Manager::getBool("enable recast mesh render", "Navigator"))); mPathgrid.reset(new Pathgrid(mRootNode)); - mObjects.reset(new Objects(mResourceSystem, sceneRoot, mUnrefQueue.get())); + mObjects.reset(new Objects(mResourceSystem, sceneRoot)); if (getenv("OPENMW_DONT_PRECOMPILE") == nullptr) { @@ -434,7 +432,6 @@ namespace MWRender mTerrain.reset(new Terrain::TerrainGrid(sceneRoot, mRootNode, mResourceSystem, mTerrainStorage.get(), Mask_Terrain, Mask_PreCompile, Mask_Debug)); mTerrain->setTargetFrameRate(Settings::Manager::getFloat("target framerate", "Cells")); - mTerrain->setWorkQueue(mWorkQueue.get()); if (groundcover) { @@ -569,11 +566,6 @@ namespace MWRender return mWorkQueue.get(); } - SceneUtil::UnrefQueue* RenderingManager::getUnrefQueue() - { - return mUnrefQueue.get(); - } - Terrain::World* RenderingManager::getTerrain() { return mTerrain.get(); @@ -804,8 +796,6 @@ namespace MWRender { reportStats(); - mUnrefQueue->flush(mWorkQueue.get()); - float rainIntensity = mSky->getPrecipitationAlpha(); mWater->setRainIntensity(rainIntensity); @@ -1223,8 +1213,6 @@ namespace MWRender unsigned int frameNumber = mViewer->getFrameStamp()->getFrameNumber(); if (stats->collectStats("resource")) { - stats->setAttribute(frameNumber, "UnrefQueue", mUnrefQueue->getNumItems()); - mTerrain->reportStats(frameNumber, stats); } } diff --git a/apps/openmw/mwrender/renderingmanager.hpp b/apps/openmw/mwrender/renderingmanager.hpp index bd8bbe4694..b8d5d955c8 100644 --- a/apps/openmw/mwrender/renderingmanager.hpp +++ b/apps/openmw/mwrender/renderingmanager.hpp @@ -59,7 +59,6 @@ namespace SceneUtil { class ShadowManager; class WorkQueue; - class UnrefQueue; } namespace DetourNavigator @@ -106,7 +105,6 @@ namespace MWRender Resource::ResourceSystem* getResourceSystem(); SceneUtil::WorkQueue* getWorkQueue(); - SceneUtil::UnrefQueue* getUnrefQueue(); Terrain::World* getTerrain(); void preloadCommonAssets(); @@ -262,7 +260,6 @@ namespace MWRender Resource::ResourceSystem* mResourceSystem; osg::ref_ptr mWorkQueue; - osg::ref_ptr mUnrefQueue; osg::ref_ptr mSunLight; diff --git a/apps/openmw/mwworld/cellpreloader.cpp b/apps/openmw/mwworld/cellpreloader.cpp index 590e1e9c00..fc09a6e9a8 100644 --- a/apps/openmw/mwworld/cellpreloader.cpp +++ b/apps/openmw/mwworld/cellpreloader.cpp @@ -12,7 +12,6 @@ #include #include #include -#include #include #include @@ -322,11 +321,10 @@ namespace MWWorld PreloadMap::iterator found = mPreloadCells.find(cell); if (found != mPreloadCells.end()) { - // do the deletion in the background thread if (found->second.mWorkItem) { found->second.mWorkItem->abort(); - mUnrefQueue->push(std::move(found->second.mWorkItem)); + found->second.mWorkItem = nullptr; } mPreloadCells.erase(found); @@ -340,7 +338,7 @@ namespace MWWorld if (it->second.mWorkItem) { it->second.mWorkItem->abort(); - mUnrefQueue->push(it->second.mWorkItem); + it->second.mWorkItem = nullptr; } mPreloadCells.erase(it++); @@ -356,7 +354,7 @@ namespace MWWorld if (it->second.mWorkItem) { it->second.mWorkItem->abort(); - mUnrefQueue->push(it->second.mWorkItem); + it->second.mWorkItem = nullptr; } mPreloadCells.erase(it++); } @@ -409,11 +407,6 @@ namespace MWWorld mWorkQueue = workQueue; } - void CellPreloader::setUnrefQueue(SceneUtil::UnrefQueue* unrefQueue) - { - mUnrefQueue = unrefQueue; - } - bool CellPreloader::syncTerrainLoad(const std::vector &positions, double timestamp, Loading::Listener& listener) { if (!mTerrainPreloadItem) @@ -455,11 +448,7 @@ namespace MWWorld else { if (mTerrainViews.size() > positions.size()) - { - for (unsigned int i=positions.size(); ipush(mTerrainViews[i]); mTerrainViews.resize(positions.size()); - } else if (mTerrainViews.size() < positions.size()) { for (unsigned int i=mTerrainViews.size(); i workQueue); - void setUnrefQueue(SceneUtil::UnrefQueue* unrefQueue); - typedef std::pair PositionCellGrid; void setTerrainPreloadPositions(const std::vector& positions); @@ -87,7 +80,6 @@ namespace MWWorld Terrain::World* mTerrain; MWRender::LandManager* mLandManager; osg::ref_ptr mWorkQueue; - osg::ref_ptr mUnrefQueue; double mExpiryDelay; unsigned int mMinCacheSize; unsigned int mMaxCacheSize; diff --git a/apps/openmw/mwworld/scene.cpp b/apps/openmw/mwworld/scene.cpp index e333e00623..d49e6c0e8b 100644 --- a/apps/openmw/mwworld/scene.cpp +++ b/apps/openmw/mwworld/scene.cpp @@ -15,7 +15,6 @@ #include #include #include -#include #include #include #include @@ -654,7 +653,6 @@ namespace MWWorld } mRendering.getResourceSystem()->updateCache(mRendering.getReferenceTime()); - mRendering.getUnrefQueue()->flush(mRendering.getWorkQueue()); loadingListener->increaseProgress (1); i++; @@ -701,7 +699,6 @@ namespace MWWorld } mRendering.getResourceSystem()->updateCache(mRendering.getReferenceTime()); - mRendering.getUnrefQueue()->flush(mRendering.getWorkQueue()); loadingListener->increaseProgress (1); i++; @@ -755,9 +752,6 @@ namespace MWWorld mPreloader.reset(new CellPreloader(rendering.getResourceSystem(), physics->getShapeManager(), rendering.getTerrain(), rendering.getLandManager())); mPreloader->setWorkQueue(mRendering.getWorkQueue()); - mPreloader->setUnrefQueue(rendering.getUnrefQueue()); - mPhysics->setUnrefQueue(rendering.getUnrefQueue()); - rendering.getResourceSystem()->setExpiryDelay(Settings::Manager::getFloat("cache expiry delay", "Cells")); mPreloader->setExpiryDelay(Settings::Manager::getFloat("preload cell expiry delay", "Cells")); diff --git a/components/sceneutil/unrefqueue.cpp b/components/sceneutil/unrefqueue.cpp deleted file mode 100644 index 50b23cae92..0000000000 --- a/components/sceneutil/unrefqueue.cpp +++ /dev/null @@ -1,39 +0,0 @@ -#include "unrefqueue.hpp" - -//#include - -//#include - -namespace SceneUtil -{ - void UnrefWorkItem::doWork() - { - mObjects.clear(); - } - - UnrefQueue::UnrefQueue() - { - mWorkItem = new UnrefWorkItem; - } - - void UnrefQueue::push(const osg::Referenced *obj) - { - mWorkItem->mObjects.emplace_back(obj); - } - - void UnrefQueue::flush(SceneUtil::WorkQueue *workQueue) - { - if (mWorkItem->mObjects.empty()) - return; - - workQueue->addWorkItem(mWorkItem, true); - - mWorkItem = new UnrefWorkItem; - } - - unsigned int UnrefQueue::getNumItems() const - { - return mWorkItem->mObjects.size(); - } - -} diff --git a/components/sceneutil/unrefqueue.hpp b/components/sceneutil/unrefqueue.hpp deleted file mode 100644 index 84372d28c0..0000000000 --- a/components/sceneutil/unrefqueue.hpp +++ /dev/null @@ -1,44 +0,0 @@ -#ifndef OPENMW_COMPONENTS_UNREFQUEUE_H -#define OPENMW_COMPONENTS_UNREFQUEUE_H - -#include - -#include -#include - -#include - -namespace SceneUtil -{ - class WorkQueue; - - class UnrefWorkItem : public SceneUtil::WorkItem - { - public: - std::deque > mObjects; - void doWork() override; - }; - - /// @brief Handles unreferencing of objects through the WorkQueue. Typical use scenario - /// would be the main thread pushing objects that are no longer needed, and the background thread deleting them. - class UnrefQueue : public osg::Referenced - { - public: - UnrefQueue(); - - /// Adds an object to the list of objects to be unreferenced. Call from the main thread. - void push(const osg::Referenced* obj); - - /// Adds a WorkItem to the given WorkQueue that will clear the list of objects in a worker thread, thus unreferencing them. - /// Call from the main thread. - void flush(SceneUtil::WorkQueue* workQueue); - - unsigned int getNumItems() const; - - private: - osg::ref_ptr mWorkItem; - }; - -} - -#endif diff --git a/components/terrain/compositemaprenderer.cpp b/components/terrain/compositemaprenderer.cpp index 2a3fa47daa..55da3d347a 100644 --- a/components/terrain/compositemaprenderer.cpp +++ b/components/terrain/compositemaprenderer.cpp @@ -4,9 +4,6 @@ #include #include -#include -#include - #include namespace Terrain @@ -21,8 +18,6 @@ CompositeMapRenderer::CompositeMapRenderer() mFBO = new osg::FrameBufferObject; - mUnrefQueue = new SceneUtil::UnrefQueue; - getOrCreateStateSet()->setMode(GL_LIGHTING, osg::StateAttribute::OFF); } @@ -30,11 +25,6 @@ CompositeMapRenderer::~CompositeMapRenderer() { } -void CompositeMapRenderer::setWorkQueue(SceneUtil::WorkQueue* workQueue) -{ - mWorkQueue = workQueue; -} - void CompositeMapRenderer::drawImplementation(osg::RenderInfo &renderInfo) const { double dt = mTimer.time_s(); @@ -45,9 +35,6 @@ void CompositeMapRenderer::drawImplementation(osg::RenderInfo &renderInfo) const double availableTime = std::max((targetFrameTime - dt)*conservativeTimeRatio, mMinimumTimeAvailable); - if (mWorkQueue) - mUnrefQueue->flush(mWorkQueue.get()); - std::lock_guard lock(mMutex); if (mImmediateCompileSet.empty() && mCompileSet.empty()) @@ -139,10 +126,6 @@ void CompositeMapRenderer::compile(CompositeMap &compositeMap, osg::RenderInfo & ++compositeMap.mCompiled; - if (mWorkQueue) - { - mUnrefQueue->push(compositeMap.mDrawables[i]); - } compositeMap.mDrawables[i] = nullptr; if (timeLeft) diff --git a/components/terrain/compositemaprenderer.hpp b/components/terrain/compositemaprenderer.hpp index 257173af46..f0021c88fe 100644 --- a/components/terrain/compositemaprenderer.hpp +++ b/components/terrain/compositemaprenderer.hpp @@ -13,12 +13,6 @@ namespace osg class Texture2D; } -namespace SceneUtil -{ - class UnrefQueue; - class WorkQueue; -} - namespace Terrain { @@ -45,9 +39,6 @@ namespace Terrain void compile(CompositeMap& compositeMap, osg::RenderInfo& renderInfo, double* timeLeft) const; - /// Set a WorkQueue to delete compiled composite map layers in the background thread - void setWorkQueue(SceneUtil::WorkQueue* workQueue); - /// Set the available time in seconds for compiling (non-immediate) composite maps each frame void setMinimumTimeAvailableForCompile(double time); @@ -67,9 +58,6 @@ namespace Terrain double mMinimumTimeAvailable; mutable osg::Timer mTimer; - osg::ref_ptr mUnrefQueue; - osg::ref_ptr mWorkQueue; - typedef std::set > CompileSet; mutable CompileSet mCompileSet; diff --git a/components/terrain/world.cpp b/components/terrain/world.cpp index 17a51913e5..582cc68b1b 100644 --- a/components/terrain/world.cpp +++ b/components/terrain/world.cpp @@ -84,11 +84,6 @@ World::~World() } } -void World::setWorkQueue(SceneUtil::WorkQueue* workQueue) -{ - mCompositeMapRenderer->setWorkQueue(workQueue); -} - void World::setBordersVisible(bool visible) { mBorderVisible = visible; diff --git a/components/terrain/world.hpp b/components/terrain/world.hpp index def7a2eccf..b9fa7c0160 100644 --- a/components/terrain/world.hpp +++ b/components/terrain/world.hpp @@ -28,11 +28,6 @@ namespace Resource class ResourceSystem; } -namespace SceneUtil -{ - class WorkQueue; -} - namespace Loading { class Reporter; @@ -115,9 +110,6 @@ namespace Terrain World(osg::Group* parent, Storage* storage, unsigned int nodeMask); virtual ~World(); - /// Set a WorkQueue to delete objects in the background thread. - void setWorkQueue(SceneUtil::WorkQueue* workQueue); - /// See CompositeMapRenderer::setTargetFrameRate void setTargetFrameRate(float rate);