changeset 3012:af1530b45290

Optimization: On finds, do not read JSON (disk) if main DICOM tags (DB) are sufficient
author Sebastien Jodogne <s.jodogne@gmail.com>
date Thu, 13 Dec 2018 17:54:06 +0100
parents 9f859a18cbc2
children 8812b1e27f24
files NEWS OrthancServer/OrthancFindRequestHandler.cpp OrthancServer/OrthancRestApi/OrthancRestResources.cpp OrthancServer/Search/LookupResource.cpp OrthancServer/Search/LookupResource.h OrthancServer/ServerContext.cpp
diffstat 6 files changed, 171 insertions(+), 30 deletions(-) [+]
line wrap: on
line diff
--- a/NEWS	Thu Dec 13 17:16:32 2018 +0100
+++ b/NEWS	Thu Dec 13 17:54:06 2018 +0100
@@ -2,6 +2,11 @@
 ===============================
 
 
+General
+-------
+
+* Optimization: On finds, do not read JSON (disk) if main DICOM tags (DB) are sufficient
+
 Maintenance
 -----------
 
--- a/OrthancServer/OrthancFindRequestHandler.cpp	Thu Dec 13 17:16:32 2018 +0100
+++ b/OrthancServer/OrthancFindRequestHandler.cpp	Thu Dec 13 17:54:06 2018 +0100
@@ -365,7 +365,8 @@
 
 
   static void AddAnswer(DicomFindAnswers& answers,
-                        const Json::Value& resource,
+                        const DicomMap& mainDicomTags,
+                        const Json::Value* dicomAsJson,  // only used for sequences
                         const DicomArray& query,
                         const std::list<DicomTag>& sequencesToReturn,
                         const DicomMap* counters)
@@ -383,13 +384,13 @@
       {
         // Do not include the encoding, this is handled by class ParsedDicomFile
       }
-      else
+      else if (dicomAsJson != NULL)
       {
         std::string tag = query.GetElement(i).GetTag().Format();
         std::string value;
-        if (resource.isMember(tag))
+        if (dicomAsJson->isMember(tag))
         {
-          value = resource.get(tag, Json::arrayValue).get("Value", "").asString();
+          value = dicomAsJson->get(tag, Json::arrayValue).get("Value", "").asString();
           result.SetValue(query.GetElement(i).GetTag(), value, false);
         }
         else
@@ -397,6 +398,12 @@
           result.SetValue(query.GetElement(i).GetTag(), "", false);
         }
       }
+      else
+      {
+        // Best-effort
+        // TODO
+        throw OrthancException(ErrorCode_NotImplemented);
+      }
     }
 
     if (counters != NULL)
@@ -413,7 +420,8 @@
     {
       LOG(WARNING) << "The C-FIND request does not return any DICOM tag";
     }
-    else if (sequencesToReturn.empty())
+    else if (sequencesToReturn.empty() ||
+             dicomAsJson == NULL)
     {
       answers.Add(result);
     }
@@ -424,7 +432,8 @@
       for (std::list<DicomTag>::const_iterator tag = sequencesToReturn.begin();
            tag != sequencesToReturn.end(); ++tag)
       {
-        const Json::Value& source = resource[tag->Format()];
+        assert(dicomAsJson != NULL);
+        const Json::Value& source = (*dicomAsJson) [tag->Format()];
 
         if (source.type() == Json::objectValue &&
             source.isMember("Type") &&
@@ -546,6 +555,30 @@
     {
       answers_.SetComplete(false);
     }
+
+    virtual bool IsDicomAsJsonNeeded() const
+    {
+#if 1
+      return true;
+
+#else
+      // TODO
+      
+      // Ask the "DICOM-as-JSON" attachment only if sequences are to
+      // be returned OR if "query_" contains non-main DICOM tags!
+
+      // TODO - configuration option
+      bool findFromDatabase;
+      
+      {
+        // New configuration option in 1.5.1
+        OrthancConfiguration::ReaderLock lock;
+        findFromDatabase = lock.GetConfiguration().GetUnsignedIntegerParameter("FindFromDatabase", false);
+      }      
+
+      return !sequencesToReturn_.empty();
+#endif
+    }
       
     virtual void MarkAsComplete()
     {
@@ -554,10 +587,11 @@
 
     virtual void Visit(const std::string& publicId,
                        const std::string& instanceId,
-                       const Json::Value& dicom) 
+                       const DicomMap& mainDicomTags,
+                       const Json::Value* dicomAsJson) 
     {
       std::auto_ptr<DicomMap> counters(ComputeCounters(context_, instanceId, level_, filteredInput_));
-      AddAnswer(answers_, dicom, query_, sequencesToReturn_, counters.get());
+      AddAnswer(answers_, mainDicomTags, dicomAsJson, query_, sequencesToReturn_, counters.get());
     }
   };
 
@@ -690,6 +724,7 @@
 
     size_t limit = (level == ResourceType_Instance) ? maxInstances_ : maxResults_;
 
+
     LookupVisitor visitor(answers, context_, level, *filteredInput, sequencesToReturn);
     context_.Apply(visitor, lookup, 0 /* "since" is not relevant to C-FIND */, limit);
   }
--- a/OrthancServer/OrthancRestApi/OrthancRestResources.cpp	Thu Dec 13 17:16:32 2018 +0100
+++ b/OrthancServer/OrthancRestApi/OrthancRestResources.cpp	Thu Dec 13 17:54:06 2018 +0100
@@ -1283,14 +1283,20 @@
       {
       }
 
+      virtual bool IsDicomAsJsonNeeded() const
+      {
+        return false;   // (*)
+      }
+      
       virtual void MarkAsComplete()
       {
         isComplete_ = true;  // Unused information as of Orthanc 1.5.0
       }
 
       virtual void Visit(const std::string& publicId,
-                         const std::string& instanceId  /* unused */,  
-                         const Json::Value& dicom       /* unused */)
+                         const std::string& instanceId   /* unused     */,
+                         const DicomMap& mainDicomTags   /* unused     */,
+                         const Json::Value* dicomAsJson  /* unused (*) */) 
       {
         resources_.push_back(publicId);
       }
--- a/OrthancServer/Search/LookupResource.cpp	Thu Dec 13 17:16:32 2018 +0100
+++ b/OrthancServer/Search/LookupResource.cpp	Thu Dec 13 17:54:06 2018 +0100
@@ -42,6 +42,19 @@
 
 namespace Orthanc
 {
+  static bool DoesDicomMapMatch(const DicomMap& dicom,
+                                const DicomTag& tag,
+                                const IFindConstraint& constraint)
+  {
+    const DicomValue* value = dicom.TestAndGetValue(tag);
+
+    return (value != NULL &&
+            !value->IsNull() &&
+            !value->IsBinary() &&
+            constraint.Match(value->GetContent()));
+  }
+
+  
   LookupResource::Level::Level(ResourceType level) : level_(level)
   {
     const DicomTag* tags = NULL;
@@ -119,6 +132,34 @@
   }
 
 
+  bool LookupResource::Level::IsMatch(const DicomMap& dicom) const
+  {
+    for (Constraints::const_iterator it = identifiersConstraints_.begin();
+         it != identifiersConstraints_.end(); ++it)
+    {
+      assert(it->second != NULL);
+
+      if (!DoesDicomMapMatch(dicom, it->first, *it->second))
+      {
+        return false;
+      }
+    }
+
+    for (Constraints::const_iterator it = mainTagsConstraints_.begin();
+         it != mainTagsConstraints_.end(); ++it)
+    {
+      assert(it->second != NULL);
+
+      if (!DoesDicomMapMatch(dicom, it->first, *it->second))
+      {
+        return false;
+      }
+    }
+
+    return true;
+  }
+  
+
   LookupResource::LookupResource(ResourceType level) : level_(level)
   {
     switch (level)
@@ -283,22 +324,22 @@
 
 
 
-  bool LookupResource::IsMatch(const Json::Value& dicomAsJson) const
+  bool LookupResource::IsMatch(const DicomMap& dicom) const
   {
+    for (Levels::const_iterator it = levels_.begin(); it != levels_.end(); ++it)
+    {
+      if (!it->second->IsMatch(dicom))
+      {
+        return false;
+      }
+    }
+
     for (Constraints::const_iterator it = unoptimizedConstraints_.begin(); 
          it != unoptimizedConstraints_.end(); ++it)
     {
-      std::string tag = it->first.Format();
-      if (dicomAsJson.isMember(tag) &&
-          dicomAsJson[tag]["Type"] == "String")
-      {
-        std::string value = dicomAsJson[tag]["Value"].asString();
-        if (!it->second->Match(value))
-        {
-          return false;
-        }
-      }
-      else
+      assert(it->second != NULL);
+
+      if (!DoesDicomMapMatch(dicom, it->first, *it->second))
       {
         return false;
       }
--- a/OrthancServer/Search/LookupResource.h	Thu Dec 13 17:16:32 2018 +0100
+++ b/OrthancServer/Search/LookupResource.h	Thu Dec 13 17:54:06 2018 +0100
@@ -64,6 +64,8 @@
 
       void Apply(SetOfResources& candidates,
                  IDatabaseWrapper& database) const;
+
+      bool IsMatch(const DicomMap& dicom) const;
     };
 
     typedef std::map<ResourceType, Level*>  Levels;
@@ -89,11 +91,14 @@
       {
       }
 
+      virtual bool IsDicomAsJsonNeeded() const = 0;
+      
       virtual void MarkAsComplete() = 0;
 
       virtual void Visit(const std::string& publicId,
                          const std::string& instanceId,
-                         const Json::Value& dicom) = 0;
+                         const DicomMap& mainDicomTags,
+                         const Json::Value* dicomAsJson) = 0;
     };
 
     LookupResource(ResourceType level);
@@ -117,6 +122,11 @@
     void FindCandidates(std::list<int64_t>& result,
                         IDatabaseWrapper& database) const;
 
-    bool IsMatch(const Json::Value& dicomAsJson) const;
+    bool HasOnlyMainDicomTags() const
+    {
+      return unoptimizedConstraints_.empty();
+    }
+
+    bool IsMatch(const DicomMap& dicom) const;
   };
 }
--- a/OrthancServer/ServerContext.cpp	Thu Dec 13 17:16:32 2018 +0100
+++ b/OrthancServer/ServerContext.cpp	Thu Dec 13 17:54:06 2018 +0100
@@ -788,13 +788,36 @@
     size_t countResults = 0;
     size_t skipped = 0;
     bool complete = true;
-
+    
     for (size_t i = 0; i < instances.size(); i++)
     {
-      // TODO - Don't read the full JSON from the disk if only "main
-      // DICOM tags" are to be returned
-      Json::Value dicom;
-      ReadDicomAsJson(dicom, instances[i]);
+      // Optimization in Orthanc 1.5.1 - Don't read the full JSON from
+      // the disk if only "main DICOM tags" are to be returned
+
+      std::auto_ptr<Json::Value> dicomAsJson;
+
+      bool hasOnlyMainDicomTags;
+      DicomMap dicom;
+      
+      if (lookup.HasOnlyMainDicomTags() &&
+          GetIndex().GetAllMainDicomTags(dicom, instances[i]))
+      {
+        // Case (1): The main DICOM tags, as stored in the database,
+        // are sufficient to look for match
+        hasOnlyMainDicomTags = true;
+      }
+      else
+      {
+        // Case (2): Need to read the "DICOM-as-JSON" attachment from
+        // the storage area
+        dicomAsJson.reset(new Json::Value);
+        ReadDicomAsJson(*dicomAsJson, instances[i]);
+
+        dicom.FromDicomAsJson(*dicomAsJson);
+
+        // This map contains the entire JSON, i.e. more than the main DICOM tags
+        hasOnlyMainDicomTags = false;   
+      }
       
       if (lookup.IsMatch(dicom))
       {
@@ -811,7 +834,28 @@
         }
         else
         {
-          visitor.Visit(resources[i], instances[i], dicom);
+          if (dicomAsJson.get() == NULL &&
+              visitor.IsDicomAsJsonNeeded())
+          {
+            dicomAsJson.reset(new Json::Value);
+            ReadDicomAsJson(*dicomAsJson, instances[i]);
+          }
+
+          if (hasOnlyMainDicomTags)
+          {
+            // This is Case (1): The variable "dicom" only contains the main DICOM tags
+            visitor.Visit(resources[i], instances[i], dicom, dicomAsJson.get());
+          }
+          else
+          {
+            // Remove the non-main DICOM tags from "dicom" if Case (2)
+            // was used, for consistency with Case (1)
+
+            DicomMap mainDicomTags;
+            mainDicomTags.ExtractMainDicomTags(dicom);
+            visitor.Visit(resources[i], instances[i], mainDicomTags, dicomAsJson.get());            
+          }
+            
           countResults ++;
         }
       }