Mercurial > hg > orthanc

changeset 4457:789676a8c96a

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
Refactoring and improvements to the cache of DICOM files in ServerContext
author Sebastien Jodogne <s.jodogne@gmail.com>
date Tue, 19 Jan 2021 19:05:04 +0100
parents 3e4f7b7840f0
children e4dae17035b9
files NEWS OrthancFramework/Sources/Cache/MemoryObjectCache.cpp OrthancFramework/Sources/Cache/MemoryObjectCache.h OrthancFramework/Sources/DicomParsing/ParsedDicomCache.cpp OrthancFramework/Sources/DicomParsing/ParsedDicomCache.h OrthancFramework/UnitTestsSources/FromDcmtkTests.cpp OrthancFramework/UnitTestsSources/JobsTests.cpp OrthancServer/Sources/ServerContext.cpp OrthancServer/Sources/ServerContext.h
diffstat 9 files changed, 153 insertions(+), 66 deletions(-) [+]
line wrap: on
line diff
--- 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
--- 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__)
--- 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();
--- 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();
     }
--- 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);
--- 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());
 }
 
 
--- 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());
   }  
 }
--- 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 <dcmtk/dcmdata/dcfilefo.h>
 
 
+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<float>(dicomCache_.GetCurrentSize()) / static_cast<float>(1024 * 1024));
+    metricsRegistry_->SetValue("orthanc_dicom_cache_count",
+                               static_cast<float>(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<IDynamicObject> p;
-    p.reset(that_.provider_.Provide(instancePublicId));
-    dicom_ = dynamic_cast<ParsedDicomFile*>(p.get());
-#else
-    dicom_ = &dynamic_cast<ParsedDicomFile&>(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());
   }
 
--- 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<ParsedDicomCache::Accessor>  accessor_;
+      std::unique_ptr<ParsedDicomFile>             dicom_;
+      size_t                                       dicomSize_;
+      std::unique_ptr<Semaphore::Locker>           largeDicomLocker_;
 
     public:
-      DicomCacheLocker(ServerContext& that,
+      DicomCacheLocker(ServerContext& context,
                        const std::string& instancePublicId);
 
       ~DicomCacheLocker();
 
-      ParsedDicomFile& GetDicom()
-      {
-        return *dicom_;
-      }
+      ParsedDicomFile& GetDicom() const;
     };
 
     ServerContext(IDatabaseWrapper& database,