# HG changeset patch # User Sebastien Jodogne # Date 1611079504 -3600 # Node ID 789676a8c96a0455110f1981cda37da517bfd467 # Parent 3e4f7b7840f0a192f45c93145685a4dea4235d85 Refactoring and improvements to the cache of DICOM files in ServerContext diff -r 3e4f7b7840f0 -r 789676a8c96a NEWS --- a/NEWS Tue Jan 19 16:11:23 2021 +0100 +++ b/NEWS Tue Jan 19 19:05:04 2021 +0100 @@ -24,6 +24,8 @@ Maintenance ----------- +* Refactoring and improvements to the cache of DICOM files (it can now hold many files) +* New Prometheus metrics "orthanc_dicom_cache_count" and "orthanc_dicom_cache_size" * Fix upload of multiple DICOM files using one single POST call to "multipart/form-data" Could be the final resolution of issue #21 (DICOM files missing after uploading with Firefox) * Partial fix of issue #48 (Windows service not stopped properly), cf. comments 4 and 5 diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancFramework/Sources/Cache/MemoryObjectCache.cpp --- a/OrthancFramework/Sources/Cache/MemoryObjectCache.cpp Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancFramework/Sources/Cache/MemoryObjectCache.cpp Tue Jan 19 19:05:04 2021 +0100 @@ -99,6 +99,16 @@ } + size_t MemoryObjectCache::GetNumberOfItems() + { +#if !defined(__EMSCRIPTEN__) + boost::mutex::scoped_lock lock(cacheMutex_); +#endif + + return content_.GetSize(); + } + + size_t MemoryObjectCache::GetCurrentSize() { #if !defined(__EMSCRIPTEN__) diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancFramework/Sources/Cache/MemoryObjectCache.h --- a/OrthancFramework/Sources/Cache/MemoryObjectCache.h Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancFramework/Sources/Cache/MemoryObjectCache.h Tue Jan 19 19:05:04 2021 +0100 @@ -64,6 +64,8 @@ ~MemoryObjectCache(); + size_t GetNumberOfItems(); // For unit tests only + size_t GetCurrentSize(); // For unit tests only size_t GetMaximumSize(); diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancFramework/Sources/DicomParsing/ParsedDicomCache.cpp --- a/OrthancFramework/Sources/DicomParsing/ParsedDicomCache.cpp Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancFramework/Sources/DicomParsing/ParsedDicomCache.cpp Tue Jan 19 19:05:04 2021 +0100 @@ -69,7 +69,24 @@ } - size_t ParsedDicomCache::GetCurrentSize() // For unit tests only + size_t ParsedDicomCache::GetNumberOfItems() + { + boost::mutex::scoped_lock lock(mutex_); + + if (cache_.get() == NULL) + { + return (largeDicom_.get() == NULL ? 0 : 1); + } + else + { + assert(largeDicom_.get() == NULL); + assert(largeSize_ == 0); + return cache_->GetNumberOfItems(); + } + } + + + size_t ParsedDicomCache::GetCurrentSize() { boost::mutex::scoped_lock lock(mutex_); @@ -79,6 +96,7 @@ } else { + assert(largeDicom_.get() == NULL); assert(largeSize_ == 0); return cache_->GetCurrentSize(); } diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancFramework/Sources/DicomParsing/ParsedDicomCache.h --- a/OrthancFramework/Sources/DicomParsing/ParsedDicomCache.h Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancFramework/Sources/DicomParsing/ParsedDicomCache.h Tue Jan 19 19:05:04 2021 +0100 @@ -27,7 +27,7 @@ namespace Orthanc { - class ParsedDicomCache : public boost::noncopyable + class ORTHANC_PUBLIC ParsedDicomCache : public boost::noncopyable { private: class Item; @@ -42,6 +42,8 @@ public: explicit ParsedDicomCache(size_t size); + size_t GetNumberOfItems(); // For unit tests only + size_t GetCurrentSize(); // For unit tests only void Invalidate(const std::string& id); diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp --- a/OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp Tue Jan 19 19:05:04 2021 +0100 @@ -2088,12 +2088,14 @@ TEST(ParsedDicomCache, Basic) { ParsedDicomCache cache(10); - ASSERT_EQ(0, cache.GetCurrentSize()); + ASSERT_EQ(0u, cache.GetCurrentSize()); + ASSERT_EQ(0u, cache.GetNumberOfItems()); DicomMap tags; tags.SetValue(DICOM_TAG_PATIENT_ID, "patient1", false); cache.Acquire("a", new ParsedDicomFile(tags, Encoding_Latin1, true), 20); - ASSERT_EQ(20, cache.GetCurrentSize()); + ASSERT_EQ(20u, cache.GetCurrentSize()); + ASSERT_EQ(1u, cache.GetNumberOfItems()); { ParsedDicomCache::Accessor accessor(cache, "b"); @@ -2113,10 +2115,12 @@ tags.SetValue(DICOM_TAG_PATIENT_ID, "patient2", false); cache.Acquire("b", new ParsedDicomFile(tags, Encoding_Latin1, true), 5); - ASSERT_EQ(5, cache.GetCurrentSize()); + ASSERT_EQ(5u, cache.GetCurrentSize()); + ASSERT_EQ(1u, cache.GetNumberOfItems()); cache.Acquire("c", new ParsedDicomFile(true), 5); - ASSERT_EQ(10, cache.GetCurrentSize()); + ASSERT_EQ(10u, cache.GetCurrentSize()); + ASSERT_EQ(2u, cache.GetNumberOfItems()); { ParsedDicomCache::Accessor accessor(cache, "b"); @@ -2128,18 +2132,31 @@ } cache.Acquire("d", new ParsedDicomFile(true), 5); - ASSERT_EQ(10, cache.GetCurrentSize()); + ASSERT_EQ(10u, cache.GetCurrentSize()); + ASSERT_EQ(2u, cache.GetNumberOfItems()); ASSERT_TRUE(ParsedDicomCache::Accessor(cache, "b").IsValid()); ASSERT_FALSE(ParsedDicomCache::Accessor(cache, "c").IsValid()); // recycled by LRU ASSERT_TRUE(ParsedDicomCache::Accessor(cache, "d").IsValid()); + cache.Invalidate("d"); + ASSERT_EQ(5u, cache.GetCurrentSize()); + ASSERT_EQ(1u, cache.GetNumberOfItems()); + ASSERT_TRUE(ParsedDicomCache::Accessor(cache, "b").IsValid()); + ASSERT_FALSE(ParsedDicomCache::Accessor(cache, "d").IsValid()); + cache.Acquire("e", new ParsedDicomFile(true), 15); - ASSERT_EQ(15, cache.GetCurrentSize()); + ASSERT_EQ(15u, cache.GetCurrentSize()); + ASSERT_EQ(1u, cache.GetNumberOfItems()); ASSERT_FALSE(ParsedDicomCache::Accessor(cache, "c").IsValid()); ASSERT_FALSE(ParsedDicomCache::Accessor(cache, "d").IsValid()); ASSERT_TRUE(ParsedDicomCache::Accessor(cache, "e").IsValid()); + + cache.Invalidate("e"); + ASSERT_EQ(0u, cache.GetCurrentSize()); + ASSERT_EQ(0u, cache.GetNumberOfItems()); + ASSERT_FALSE(ParsedDicomCache::Accessor(cache, "e").IsValid()); } diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancFramework/UnitTestsSources/JobsTests.cpp --- a/OrthancFramework/UnitTestsSources/JobsTests.cpp Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancFramework/UnitTestsSources/JobsTests.cpp Tue Jan 19 19:05:04 2021 +0100 @@ -1442,7 +1442,7 @@ a.SerializeJob(v); ASSERT_EQ(Json::objectValue, v.type()); ASSERT_EQ("ORTHANC", v["LocalAet"].asString()); - ASSERT_EQ(DicomAssociationParameters::GetDefaultTimeout(), v["Timeout"].asInt()); + ASSERT_EQ(DicomAssociationParameters::GetDefaultTimeout(), v["Timeout"].asUInt()); ASSERT_TRUE(v.isMember("Remote")); ASSERT_TRUE(v.isMember("MaximumPduLength")); @@ -1489,6 +1489,6 @@ ASSERT_EQ("key", b.GetOwnPrivateKeyPath()); ASSERT_EQ("crt", b.GetOwnCertificatePath()); ASSERT_EQ("trusted", b.GetTrustedCertificatesPath()); - ASSERT_EQ(131072, b.GetMaximumPduLength()); + ASSERT_EQ(131072u, b.GetMaximumPduLength()); } } diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancServer/Sources/ServerContext.cpp --- a/OrthancServer/Sources/ServerContext.cpp Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancServer/Sources/ServerContext.cpp Tue Jan 19 19:05:04 2021 +0100 @@ -58,10 +58,8 @@ #include +static size_t DICOM_CACHE_SIZE = 128 * 1024 * 1024; // 128 MB -#define ENABLE_DICOM_CACHE 1 - -static const size_t DICOM_CACHE_SIZE = 2; /** * IMPORTANT: We make the assumption that the same instance of @@ -272,6 +270,15 @@ } + void ServerContext::PublishDicomCacheMetrics() + { + metricsRegistry_->SetValue("orthanc_dicom_cache_size", + static_cast(dicomCache_.GetCurrentSize()) / static_cast(1024 * 1024)); + metricsRegistry_->SetValue("orthanc_dicom_cache_count", + static_cast(dicomCache_.GetNumberOfItems())); + } + + ServerContext::ServerContext(IDatabaseWrapper& database, IStorageArea& area, bool unitTesting, @@ -280,8 +287,8 @@ area_(area), compressionEnabled_(false), storeMD5_(true), - provider_(*this), - dicomCache_(provider_, DICOM_CACHE_SIZE), + largeDicomThrottler_(1), + dicomCache_(DICOM_CACHE_SIZE), mainLua_(*this), filterLua_(*this), luaListener_(*this), @@ -528,12 +535,10 @@ return StoreStatus_FilteredOut; } - { - // Remove the file from the DicomCache (useful if - // "OverwriteInstances" is set to "true") - boost::mutex::scoped_lock lock(dicomCacheMutex_); - dicomCache_.Invalidate(resultPublicId); - } + // Remove the file from the DicomCache (useful if + // "OverwriteInstances" is set to "true") + dicomCache_.Invalidate(resultPublicId); + PublishDicomCacheMetrics(); // TODO Should we use "gzip" instead? CompressionType compression = (compressionEnabled_ ? CompressionType_ZlibWithSize : CompressionType_None); @@ -878,34 +883,69 @@ } - IDynamicObject* ServerContext::DicomCacheProvider::Provide(const std::string& instancePublicId) + ServerContext::DicomCacheLocker::DicomCacheLocker(ServerContext& context, + const std::string& instancePublicId) : + context_(context), + instancePublicId_(instancePublicId) { - std::string content; - context_.ReadDicom(content, instancePublicId); - return new ParsedDicomFile(content); - } - + accessor_.reset(new ParsedDicomCache::Accessor(context_.dicomCache_, instancePublicId)); + + if (!accessor_->IsValid()) + { + accessor_.reset(NULL); - ServerContext::DicomCacheLocker::DicomCacheLocker(ServerContext& that, - const std::string& instancePublicId) : - that_(that), - lock_(that_.dicomCacheMutex_) - { -#if ENABLE_DICOM_CACHE == 0 - static std::unique_ptr p; - p.reset(that_.provider_.Provide(instancePublicId)); - dicom_ = dynamic_cast(p.get()); -#else - dicom_ = &dynamic_cast(that_.dicomCache_.Access(instancePublicId)); -#endif + // Throttle to avoid loading several large DICOM files simultaneously + largeDicomLocker_.reset(new Semaphore::Locker(context.largeDicomThrottler_)); + + std::string content; + context_.ReadDicom(content, instancePublicId); + + // Release the throttle if loading "small" DICOM files (under + // 50MB, which is an arbitrary value) + if (content.size() < 50 * 1024 * 1024) + { + largeDicomLocker_.reset(NULL); + } + + dicom_.reset(new ParsedDicomFile(content)); + dicomSize_ = content.size(); + } + + assert(accessor_.get() != NULL || + dicom_.get() != NULL); } ServerContext::DicomCacheLocker::~DicomCacheLocker() { + if (dicom_.get() != NULL) + { + try + { + context_.dicomCache_.Acquire(instancePublicId_, dicom_.release(), dicomSize_); + context_.PublishDicomCacheMetrics(); + } + catch (OrthancException&) + { + } + } } + ParsedDicomFile& ServerContext::DicomCacheLocker::GetDicom() const + { + if (dicom_.get() != NULL) + { + return *dicom_; + } + else + { + assert(accessor_.get() != NULL); + return accessor_->GetDicom(); + } + } + + void ServerContext::SetStoreMD5ForAttachments(bool storeMD5) { LOG(INFO) << "Storing MD5 for attachments: " << (storeMD5 ? "yes" : "no"); @@ -946,8 +986,8 @@ if (expectedType == ResourceType_Instance) { // remove the file from the DicomCache - boost::mutex::scoped_lock lock(dicomCacheMutex_); dicomCache_.Invalidate(uuid); + PublishDicomCacheMetrics(); } return index_.DeleteResource(target, uuid, expectedType); @@ -956,6 +996,13 @@ void ServerContext::SignalChange(const ServerIndexChange& change) { + if (change.GetResourceType() == ResourceType_Instance && + change.GetChangeType() == ChangeType_Deleted) + { + dicomCache_.Invalidate(change.GetPublicId()); + PublishDicomCacheMetrics(); + } + pendingChanges_.Enqueue(change.Clone()); } diff -r 3e4f7b7840f0 -r 789676a8c96a OrthancServer/Sources/ServerContext.h --- a/OrthancServer/Sources/ServerContext.h Tue Jan 19 16:11:23 2021 +0100 +++ b/OrthancServer/Sources/ServerContext.h Tue Jan 19 19:05:04 2021 +0100 @@ -39,10 +39,11 @@ #include "ServerIndex.h" #include "ServerJobs/IStorageCommitmentFactory.h" -#include "../../OrthancFramework/Sources/Cache/MemoryCache.h" #include "../../OrthancFramework/Sources/DicomFormat/DicomElement.h" #include "../../OrthancFramework/Sources/DicomParsing/DicomModification.h" #include "../../OrthancFramework/Sources/DicomParsing/IDicomTranscoder.h" +#include "../../OrthancFramework/Sources/DicomParsing/ParsedDicomCache.h" +#include "../../OrthancFramework/Sources/MultiThreading/Semaphore.h" namespace Orthanc @@ -122,19 +123,6 @@ } }; - class DicomCacheProvider : public Deprecated::ICachePageProvider // TODO - { - private: - ServerContext& context_; - - public: - explicit DicomCacheProvider(ServerContext& context) : context_(context) - { - } - - virtual IDynamicObject* Provide(const std::string& id) ORTHANC_OVERRIDE; - }; - class ServerListener { private: @@ -185,10 +173,9 @@ bool compressionEnabled_; bool storeMD5_; - - DicomCacheProvider provider_; - boost::mutex dicomCacheMutex_; - Deprecated::MemoryCache dicomCache_; // TODO + + Semaphore largeDicomThrottler_; // New in Orthanc 1.9.0 (notably for very large DICOM files in WSI) + ParsedDicomCache dicomCache_; LuaScripting mainLua_; LuaScripting filterLua_; @@ -249,6 +236,8 @@ size_t since, size_t limit); + void PublishDicomCacheMetrics(); + // This DicomModification object is intended to be used as a // "rules engine" when de-identifying logs for C-Find, C-Get, and // C-Move queries (new in Orthanc 1.8.2) @@ -259,20 +248,20 @@ class DicomCacheLocker : public boost::noncopyable { private: - ServerContext& that_; - ParsedDicomFile *dicom_; - boost::mutex::scoped_lock lock_; + ServerContext& context_; + std::string instancePublicId_; + std::unique_ptr accessor_; + std::unique_ptr dicom_; + size_t dicomSize_; + std::unique_ptr largeDicomLocker_; public: - DicomCacheLocker(ServerContext& that, + DicomCacheLocker(ServerContext& context, const std::string& instancePublicId); ~DicomCacheLocker(); - ParsedDicomFile& GetDicom() - { - return *dicom_; - } + ParsedDicomFile& GetDicom() const; }; ServerContext(IDatabaseWrapper& database,