diff --git a/apps/openmw_test_suite/CMakeLists.txt b/apps/openmw_test_suite/CMakeLists.txt index c8679ab7f4..967511953d 100644 --- a/apps/openmw_test_suite/CMakeLists.txt +++ b/apps/openmw_test_suite/CMakeLists.txt @@ -95,6 +95,8 @@ file(GLOB UNITTEST_SRC_FILES nifosg/testnifloader.cpp esmterrain/testgridsampling.cpp + + resource/testobjectcache.cpp ) source_group(apps\\openmw_test_suite FILES openmw_test_suite.cpp ${UNITTEST_SRC_FILES}) diff --git a/apps/openmw_test_suite/resource/testobjectcache.cpp b/apps/openmw_test_suite/resource/testobjectcache.cpp new file mode 100644 index 0000000000..1c8cff6af0 --- /dev/null +++ b/apps/openmw_test_suite/resource/testobjectcache.cpp @@ -0,0 +1,349 @@ +#include + +#include +#include + +#include + +namespace Resource +{ + namespace + { + using namespace ::testing; + + TEST(ResourceGenericObjectCacheTest, getRefFromObjectCacheShouldReturnNullptrByDefault) + { + osg::ref_ptr> cache(new GenericObjectCache); + EXPECT_EQ(cache->getRefFromObjectCache(42), nullptr); + } + + TEST(ResourceGenericObjectCacheTest, getRefFromObjectCacheOrNoneShouldReturnNulloptByDefault) + { + osg::ref_ptr> cache(new GenericObjectCache); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(42), std::nullopt); + } + + struct Object : osg::Object + { + Object() = default; + + Object(const Object& other, const osg::CopyOp& copyOp = osg::CopyOp()) + : osg::Object(other, copyOp) + { + } + + META_Object(ResourceTest, Object) + }; + + TEST(ResourceGenericObjectCacheTest, shouldStoreValues) + { + osg::ref_ptr> cache(new GenericObjectCache); + const int key = 42; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + EXPECT_EQ(cache->getRefFromObjectCache(key), value); + } + + TEST(ResourceGenericObjectCacheTest, shouldStoreNullptrValues) + { + osg::ref_ptr> cache(new GenericObjectCache); + const int key = 42; + cache->addEntryToObjectCache(key, nullptr); + EXPECT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(nullptr)); + } + + TEST(ResourceGenericObjectCacheTest, updateShouldExtendLifetimeForItemsWithZeroTimestamp) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const int key = 42; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value, 0); + value = nullptr; + + const double referenceTime = 1000; + const double expiryDelay = 1; + cache->update(referenceTime, expiryDelay); + EXPECT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + } + + TEST(ResourceGenericObjectCacheTest, addEntryToObjectCacheShouldReplaceExistingItemByKey) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const int key = 42; + osg::ref_ptr value1(new Object); + osg::ref_ptr value2(new Object); + cache->addEntryToObjectCache(key, value1); + ASSERT_EQ(cache->getRefFromObjectCache(key), value1); + cache->addEntryToObjectCache(key, value2); + EXPECT_EQ(cache->getRefFromObjectCache(key), value2); + } + + TEST(ResourceGenericObjectCacheTest, addEntryToObjectCacheShouldMarkLifetime) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const double referenceTime = 1; + const double expiryDelay = 2; + + const int key = 42; + cache->addEntryToObjectCache(key, nullptr, referenceTime + expiryDelay); + + cache->update(referenceTime, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + 2 * expiryDelay, expiryDelay); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, updateShouldRemoveExpiredItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const double referenceTime = 1; + const double expiryDelay = 1; + + const int key = 42; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + value = nullptr; + + cache->update(referenceTime, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay, expiryDelay); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, updateShouldKeepExternallyReferencedItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const double referenceTime = 1; + const double expiryDelay = 1; + + const int key = 42; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + + cache->update(referenceTime, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay, expiryDelay); + EXPECT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(value)); + } + + TEST(ResourceGenericObjectCacheTest, updateShouldKeepNotExpiredItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const double referenceTime = 1; + const double expiryDelay = 2; + + const int key = 42; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + value = nullptr; + + cache->update(referenceTime + expiryDelay, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay / 2, expiryDelay); + EXPECT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + } + + TEST(ResourceGenericObjectCacheTest, updateShouldKeepNotExpiredNullptrItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const double referenceTime = 1; + const double expiryDelay = 2; + + const int key = 42; + cache->addEntryToObjectCache(key, nullptr); + + cache->update(referenceTime + expiryDelay, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay / 2, expiryDelay); + EXPECT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + } + + TEST(ResourceGenericObjectCacheTest, getRefFromObjectCacheOrNoneShouldNotExtendItemLifetime) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const double referenceTime = 1; + const double expiryDelay = 2; + + const int key = 42; + cache->addEntryToObjectCache(key, nullptr); + + cache->update(referenceTime, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay / 2, expiryDelay); + ASSERT_THAT(cache->getRefFromObjectCacheOrNone(key), Optional(_)); + + cache->update(referenceTime + expiryDelay, expiryDelay); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, lowerBoundShouldSupportHeterogeneousLookup) + { + osg::ref_ptr> cache(new GenericObjectCache); + cache->addEntryToObjectCache("a", nullptr); + cache->addEntryToObjectCache("c", nullptr); + EXPECT_THAT(cache->lowerBound(std::string_view("b")), Optional(Pair("c", _))); + } + + TEST(ResourceGenericObjectCacheTest, shouldSupportRemovingItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + const int key = 42; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + ASSERT_EQ(cache->getRefFromObjectCache(key), value); + cache->removeFromObjectCache(key); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, clearShouldRemoveAllItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + const int key1 = 42; + const int key2 = 13; + osg::ref_ptr value1(new Object); + osg::ref_ptr value2(new Object); + cache->addEntryToObjectCache(key1, value1); + cache->addEntryToObjectCache(key2, value2); + + ASSERT_EQ(cache->getRefFromObjectCache(key1), value1); + ASSERT_EQ(cache->getRefFromObjectCache(key2), value2); + + cache->clear(); + + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key1), std::nullopt); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key2), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, callShouldIterateOverAllItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + osg::ref_ptr value1(new Object); + osg::ref_ptr value2(new Object); + osg::ref_ptr value3(new Object); + cache->addEntryToObjectCache(1, value1); + cache->addEntryToObjectCache(2, value2); + cache->addEntryToObjectCache(3, value3); + + std::vector> actual; + cache->call([&](int key, osg::Object* value) { actual.emplace_back(key, value); }); + + EXPECT_THAT(actual, ElementsAre(Pair(1, value1.get()), Pair(2, value2.get()), Pair(3, value3.get()))); + } + + TEST(ResourceGenericObjectCacheTest, getCacheSizeShouldReturnNumberOrAddedItems) + { + osg::ref_ptr> cache(new GenericObjectCache); + + osg::ref_ptr value1(new Object); + osg::ref_ptr value2(new Object); + cache->addEntryToObjectCache(13, value1); + cache->addEntryToObjectCache(42, value2); + + EXPECT_EQ(cache->getCacheSize(), 2); + } + + TEST(ResourceGenericObjectCacheTest, lowerBoundShouldReturnFirstNotLessThatGivenKey) + { + osg::ref_ptr> cache(new GenericObjectCache); + + osg::ref_ptr value1(new Object); + osg::ref_ptr value2(new Object); + osg::ref_ptr value3(new Object); + cache->addEntryToObjectCache(1, value1); + cache->addEntryToObjectCache(2, value2); + cache->addEntryToObjectCache(4, value3); + + EXPECT_THAT(cache->lowerBound(3), Optional(Pair(4, value3))); + } + + TEST(ResourceGenericObjectCacheTest, lowerBoundShouldReturnNulloptWhenKeyIsGreaterThanAnyOther) + { + osg::ref_ptr> cache(new GenericObjectCache); + + osg::ref_ptr value1(new Object); + osg::ref_ptr value2(new Object); + osg::ref_ptr value3(new Object); + cache->addEntryToObjectCache(1, value1); + cache->addEntryToObjectCache(2, value2); + cache->addEntryToObjectCache(3, value3); + + EXPECT_EQ(cache->lowerBound(4), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, addEntryToObjectCacheShouldSupportHeterogeneousLookup) + { + osg::ref_ptr> cache(new GenericObjectCache); + const std::string key = "key"; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(std::string_view("key"), value); + EXPECT_EQ(cache->getRefFromObjectCache(key), value); + } + + TEST(ResourceGenericObjectCacheTest, addEntryToObjectCacheShouldKeyMoving) + { + osg::ref_ptr> cache(new GenericObjectCache); + std::string key(128, 'a'); + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(std::move(key), value); + EXPECT_EQ(key, ""); + EXPECT_EQ(cache->getRefFromObjectCache(std::string(128, 'a')), value); + } + + TEST(ResourceGenericObjectCacheTest, removeFromObjectCacheShouldSupportHeterogeneousLookup) + { + osg::ref_ptr> cache(new GenericObjectCache); + const std::string key = "key"; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + ASSERT_EQ(cache->getRefFromObjectCache(key), value); + cache->removeFromObjectCache(std::string_view("key")); + EXPECT_EQ(cache->getRefFromObjectCacheOrNone(key), std::nullopt); + } + + TEST(ResourceGenericObjectCacheTest, getRefFromObjectCacheShouldSupportHeterogeneousLookup) + { + osg::ref_ptr> cache(new GenericObjectCache); + const std::string key = "key"; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + EXPECT_EQ(cache->getRefFromObjectCache(std::string_view("key")), value); + } + + TEST(ResourceGenericObjectCacheTest, getRefFromObjectCacheOrNoneShouldSupportHeterogeneousLookup) + { + osg::ref_ptr> cache(new GenericObjectCache); + const std::string key = "key"; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + EXPECT_THAT(cache->getRefFromObjectCacheOrNone(std::string_view("key")), Optional(value)); + } + + TEST(ResourceGenericObjectCacheTest, checkInObjectCacheShouldSupportHeterogeneousLookup) + { + osg::ref_ptr> cache(new GenericObjectCache); + const std::string key = "key"; + osg::ref_ptr value(new Object); + cache->addEntryToObjectCache(key, value); + EXPECT_TRUE(cache->checkInObjectCache(std::string_view("key"), 0)); + } + } +} diff --git a/components/resource/objectcache.hpp b/components/resource/objectcache.hpp index 881729ffc4..dffa0e9fdb 100644 --- a/components/resource/objectcache.hpp +++ b/components/resource/objectcache.hpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -48,48 +49,25 @@ namespace Resource { } - /** For each object in the cache which has an reference count greater than 1 - * (and therefore referenced by elsewhere in the application) set the time stamp - * for that object in the cache to specified time. - * This would typically be called once per frame by applications which are doing database paging, - * and need to prune objects that are no longer required. - * The time used should be taken from the FrameStamp::getReferenceTime().*/ - void updateTimeStampOfObjectsInCacheWithExternalReferences(double referenceTime) - { - // look for objects with external references and update their time stamp. - std::lock_guard lock(_objectCacheMutex); - for (typename ObjectCacheMap::iterator itr = _objectCache.begin(); itr != _objectCache.end(); ++itr) - { - // If ref count is greater than 1, the object has an external reference. - // If the timestamp is yet to be initialized, it needs to be updated too. - if ((itr->second.mValue != nullptr && itr->second.mValue->referenceCount() > 1) - || itr->second.mLastUsage == 0.0) - itr->second.mLastUsage = referenceTime; - } - } - - /** Removed object in the cache which have a time stamp at or before the specified expiry time. - * This would typically be called once per frame by applications which are doing database paging, - * and need to prune objects that are no longer required, and called after the a called - * after the call to updateTimeStampOfObjectsInCacheWithExternalReferences(expirtyTime).*/ - void removeExpiredObjectsInCache(double expiryTime) + // Update last usage timestamp using referenceTime for each cache time if they are not nullptr and referenced + // from somewhere else. Remove items with last usage > expiryTime. Note: last usage might be updated from other + // places so nullptr or not references elsewhere items are not always removed. + void update(double referenceTime, double expiryDelay) { std::vector> objectsToRemove; { - std::lock_guard lock(_objectCacheMutex); - // Remove expired entries from object cache - typename ObjectCacheMap::iterator oitr = _objectCache.begin(); - while (oitr != _objectCache.end()) - { - if (oitr->second.mLastUsage <= expiryTime) - { - if (oitr->second.mValue != nullptr) - objectsToRemove.push_back(std::move(oitr->second.mValue)); - _objectCache.erase(oitr++); - } - else - ++oitr; - } + const double expiryTime = referenceTime - expiryDelay; + std::lock_guard lock(mMutex); + std::erase_if(mItems, [&](auto& v) { + Item& item = v.second; + if ((item.mValue != nullptr && item.mValue->referenceCount() > 1) || item.mLastUsage == 0) + item.mLastUsage = referenceTime; + if (item.mLastUsage > expiryTime) + return false; + if (item.mValue != nullptr) + objectsToRemove.push_back(std::move(item.mValue)); + return true; + }); } // note, actual unref happens outside of the lock objectsToRemove.clear(); @@ -98,52 +76,57 @@ namespace Resource /** Remove all objects in the cache regardless of having external references or expiry times.*/ void clear() { - std::lock_guard lock(_objectCacheMutex); - _objectCache.clear(); + std::lock_guard lock(mMutex); + mItems.clear(); } /** Add a key,object,timestamp triple to the Registry::ObjectCache.*/ - void addEntryToObjectCache(const KeyType& key, osg::Object* object, double timestamp = 0.0) + template + void addEntryToObjectCache(K&& key, osg::Object* object, double timestamp = 0.0) { - std::lock_guard lock(_objectCacheMutex); - _objectCache[key] = Item{ object, timestamp }; + std::lock_guard lock(mMutex); + const auto it = mItems.find(key); + if (it == mItems.end()) + mItems.emplace_hint(it, std::forward(key), Item{ object, timestamp }); + else + it->second = Item{ object, timestamp }; } /** Remove Object from cache.*/ - void removeFromObjectCache(const KeyType& key) + void removeFromObjectCache(const auto& key) { - std::lock_guard lock(_objectCacheMutex); - typename ObjectCacheMap::iterator itr = _objectCache.find(key); - if (itr != _objectCache.end()) - _objectCache.erase(itr); + std::lock_guard lock(mMutex); + const auto itr = mItems.find(key); + if (itr != mItems.end()) + mItems.erase(itr); } /** Get an ref_ptr from the object cache*/ - osg::ref_ptr getRefFromObjectCache(const KeyType& key) + osg::ref_ptr getRefFromObjectCache(const auto& key) { - std::lock_guard lock(_objectCacheMutex); - typename ObjectCacheMap::iterator itr = _objectCache.find(key); - if (itr != _objectCache.end()) + std::lock_guard lock(mMutex); + const auto itr = mItems.find(key); + if (itr != mItems.end()) return itr->second.mValue; else return nullptr; } - std::optional> getRefFromObjectCacheOrNone(const KeyType& key) + std::optional> getRefFromObjectCacheOrNone(const auto& key) { - const std::lock_guard lock(_objectCacheMutex); - const auto it = _objectCache.find(key); - if (it == _objectCache.end()) + const std::lock_guard lock(mMutex); + const auto it = mItems.find(key); + if (it == mItems.end()) return std::nullopt; return it->second.mValue; } /** Check if an object is in the cache, and if it is, update its usage time stamp. */ - bool checkInObjectCache(const KeyType& key, double timeStamp) + bool checkInObjectCache(const auto& key, double timeStamp) { - std::lock_guard lock(_objectCacheMutex); - typename ObjectCacheMap::iterator itr = _objectCache.find(key); - if (itr != _objectCache.end()) + std::lock_guard lock(mMutex); + const auto itr = mItems.find(key); + if (itr != mItems.end()) { itr->second.mLastUsage = timeStamp; return true; @@ -155,51 +138,43 @@ namespace Resource /** call releaseGLObjects on all objects attached to the object cache.*/ void releaseGLObjects(osg::State* state) { - std::lock_guard lock(_objectCacheMutex); - for (typename ObjectCacheMap::iterator itr = _objectCache.begin(); itr != _objectCache.end(); ++itr) - { - osg::Object* object = itr->second.mValue.get(); - object->releaseGLObjects(state); - } + std::lock_guard lock(mMutex); + for (const auto& [k, v] : mItems) + v.mValue->releaseGLObjects(state); } /** call node->accept(nv); for all nodes in the objectCache. */ void accept(osg::NodeVisitor& nv) { - std::lock_guard lock(_objectCacheMutex); - for (typename ObjectCacheMap::iterator itr = _objectCache.begin(); itr != _objectCache.end(); ++itr) - { - if (osg::Object* object = itr->second.mValue.get()) - { - osg::Node* node = dynamic_cast(object); - if (node) + std::lock_guard lock(mMutex); + for (const auto& [k, v] : mItems) + if (osg::Object* const object = v.mValue.get()) + if (osg::Node* const node = dynamic_cast(object)) node->accept(nv); - } - } } /** call operator()(KeyType, osg::Object*) for each object in the cache. */ template - void call(Functor& f) + void call(Functor&& f) { - std::lock_guard lock(_objectCacheMutex); - for (typename ObjectCacheMap::iterator it = _objectCache.begin(); it != _objectCache.end(); ++it) - f(it->first, it->second.mValue.get()); + std::lock_guard lock(mMutex); + for (const auto& [k, v] : mItems) + f(k, v.mValue.get()); } /** Get the number of objects in the cache. */ unsigned int getCacheSize() const { - std::lock_guard lock(_objectCacheMutex); - return _objectCache.size(); + std::lock_guard lock(mMutex); + return mItems.size(); } template std::optional>> lowerBound(K&& key) { - const std::lock_guard lock(_objectCacheMutex); - const auto it = _objectCache.lower_bound(std::forward(key)); - if (it == _objectCache.end()) + const std::lock_guard lock(mMutex); + const auto it = mItems.lower_bound(std::forward(key)); + if (it == mItems.end()) return std::nullopt; return std::pair(it->first, it->second.mValue); } @@ -211,12 +186,8 @@ namespace Resource double mLastUsage; }; - virtual ~GenericObjectCache() {} - - using ObjectCacheMap = std::map>; - - ObjectCacheMap _objectCache; - mutable std::mutex _objectCacheMutex; + std::map> mItems; + mutable std::mutex mMutex; }; class ObjectCache : public GenericObjectCache diff --git a/components/resource/resourcemanager.hpp b/components/resource/resourcemanager.hpp index 55e4e142b5..b2427c308a 100644 --- a/components/resource/resourcemanager.hpp +++ b/components/resource/resourcemanager.hpp @@ -49,11 +49,7 @@ namespace Resource virtual ~GenericResourceManager() = default; /// Clear cache entries that have not been referenced for longer than expiryDelay. - void updateCache(double referenceTime) override - { - mCache->updateTimeStampOfObjectsInCacheWithExternalReferences(referenceTime); - mCache->removeExpiredObjectsInCache(referenceTime - mExpiryDelay); - } + void updateCache(double referenceTime) override { mCache->update(referenceTime, mExpiryDelay); } /// Clear all cache entries. void clearCache() override { mCache->clear(); }