GH-454: Fix activating topology takes long with assets (GH-454)

dds-topology: Fix activating topology takes too long when task assets are used. (GH-454)
FairRootGroup · Jun 29, 2022 · 8bf53f5 · 8bf53f5
1 parent 061474b
commit 8bf53f5
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 9 deletions.
diff --git a/ReleaseNotes.md b/ReleaseNotes.md
@@ -37,6 +37,7 @@ Added: The command learned a new argument --inline-config. Content of this strin
 ### dds-topology
 Fixed: Stability improvements.     
 Fixed: A bug which caused dds::topology_api::CTopoCreator to ignore task assets. (GH-452)      
+Fixed: Activating topology takes too long when task assets are used. (GH-454)    
 Added: A new groupName requirement. It can be used on task and collection. (GH-407)   
 Added: Open API to read/update/add topology variable. The CTopoVars class.    
 Added: Support for Task Assets. (GH-406)     

diff --git a/dds-topology-lib/src/TopoAsset.cpp b/dds-topology-lib/src/TopoAsset.cpp
@@ -15,7 +15,7 @@ using namespace boost::property_tree;
 using namespace dds;
 using namespace topology_api;
 
-CTopoAsset::CTopoAsset(const std::string& _name)
+CTopoAsset::CTopoAsset(const string& _name)
     : CTopoBase(_name)
 {
     setType(CTopoBase::EType::ASSET);
@@ -30,9 +30,14 @@ void CTopoAsset::initFromPropertyTree(const boost::property_tree::ptree& _pt)
     try
     {
         const ptree& assetPT = FindElementInPropertyTree(CTopoBase::EType::ASSET, getName(), _pt.get_child("topology"));
-        setAssetType(TagToAssetType(assetPT.get<std::string>("<xmlattr>.type", "")));
-        setAssetVisibility(TagToAssetVisibility(assetPT.get<std::string>("<xmlattr>.visibility", "")));
-        setValue(assetPT.get<std::string>("<xmlattr>.value", ""));
+        setAssetType(TagToAssetType(assetPT.get<string>("<xmlattr>.type", "")));
+        setAssetVisibility(TagToAssetVisibility(assetPT.get<string>("<xmlattr>.visibility", "")));
+
+        // Add asset value if the asset container doesn't have it.
+        if (getNameToValueCache() && !getNameToValueCache()->exists(getName()))
+        {
+            getNameToValueCache()->insertValue(getName(), assetPT.get<string>("<xmlattr>.value", ""));
+        }
     }
     catch (exception& error) // ptree_error, runtime_error
     {
@@ -44,7 +49,7 @@ void CTopoAsset::saveToPropertyTree(boost::property_tree::ptree& _pt)
 {
     try
     {
-        std::string tag("topology.asset.<xmlattr>");
+        string tag("topology.asset.<xmlattr>");
         _pt.put(tag + ".name", getName());
         _pt.put(tag + ".type", AssetTypeToTag(getAssetType()));
         _pt.put(tag + ".visibility", AssetVisibilityToTag(getAssetVisibility()));
@@ -56,12 +61,17 @@ void CTopoAsset::saveToPropertyTree(boost::property_tree::ptree& _pt)
     }
 }
 
-std::string CTopoAsset::getValue() const
+string CTopoAsset::getValue() const
 {
-    return m_value;
+    CNameToValueCachePtr_t assetCache{ getNameToValueCache() };
+
+    if (!assetCache)
+        return string();
+
+    return (assetCache->getValue(getName()));
 }
 
-void CTopoAsset::setValue(const std::string& _val)
+void CTopoAsset::setValue(const string& _val)
 {
     m_value = _val;
 }
@@ -102,8 +112,14 @@ ostream& operator<<(ostream& _strm, const CTopoAsset& _requirement)
 
 string CTopoAsset::hashString() const
 {
+    // WORKAROUND:
+    // Assets value can be big.
+    // We therefore can't use its value in hash calculations as it significantly drops the performance.
+    // see GH-454
+    // That means, if an asset value is changed, DDS will be not able to detect changes.
+    // Therefore, if you need to change the asset value and request a topology update, change its name too.
     stringstream ss;
     ss << "|Asset|" << getName() << "|" << AssetTypeToTag(getAssetType()) << "|"
-       << AssetVisibilityToTag(getAssetVisibility()) << "|" << getValue() << "|";
+       << AssetVisibilityToTag(getAssetVisibility()) << "|" /*<< getValue() << "|"*/;
     return ss.str();
 }
diff --git a/dds-topology-lib/src/TopoBase.cpp b/dds-topology-lib/src/TopoBase.cpp
@@ -38,6 +38,11 @@ void CTopoBase::setParent(CTopoBase* _parent)
     m_parent = _parent;
 }
 
+void CTopoBase::setNameToValueCache(CNameToValueCachePtr_t _container)
+{
+    m_nameToValueCache = _container;
+}
+
 const string& CTopoBase::getName() const
 {
     return m_name;
@@ -53,6 +58,11 @@ CTopoBase* CTopoBase::getParent() const
     return m_parent;
 }
 
+dds::topology_api::CTopoBase::CNameToValueCachePtr_t CTopoBase::getNameToValueCache() const
+{
+    return m_nameToValueCache;
+}
+
 string CTopoBase::getPath() const
 {
     if (getParent() == nullptr)

diff --git a/dds-topology-lib/src/TopoBase.h b/dds-topology-lib/src/TopoBase.h
@@ -17,6 +17,38 @@ namespace dds
 {
     namespace topology_api
     {
+        class CNameToValueCache
+        {
+            using container_t = std::map<std::string, std::string>;
+
+          public:
+            void insertValue(const std::string& _name, const std::string _value)
+            {
+                if (exists(_name))
+                    return;
+
+                m_container.insert(std::make_pair(_name, _value));
+            }
+
+            std::string getValue(const std::string& _name)
+            {
+                auto found = m_container.find(_name);
+                if (found == m_container.end())
+                    return std::string();
+
+                return found->second;
+            }
+
+            bool exists(const std::string& _name)
+            {
+                auto found = m_container.find(_name);
+                return (found != m_container.end());
+            }
+
+          private:
+            container_t m_container;
+        };
+
         class CTopoBase
         {
           public:
@@ -36,15 +68,18 @@ namespace dds
 
             using Ptr_t = std::shared_ptr<CTopoBase>;
             using PtrVector_t = std::vector<CTopoBase::Ptr_t>;
+            using CNameToValueCachePtr_t = std::shared_ptr<CNameToValueCache>;
 
             /// Modifiers
             void setName(const std::string& _name);
             void setParent(CTopoBase* _parent);
+            void setNameToValueCache(CNameToValueCachePtr_t _container);
 
             /// Accessors
             const std::string& getName() const;
             CTopoBase::EType getType() const;
             CTopoBase* getParent() const;
+            CNameToValueCachePtr_t getNameToValueCache() const;
 
             /// \brief Return full path to topo element or property.
             std::string getPath() const;
@@ -147,6 +182,7 @@ namespace dds
             std::string m_name;                          ///< Name of topology element
             CTopoBase::EType m_type{ EType::TOPO_BASE }; ///< Type of the topology element
             CTopoBase* m_parent{ nullptr };              ///< Pointer to parent element
+            CNameToValueCachePtr_t m_nameToValueCache;
         };
     } // namespace topology_api
 } // namespace dds

diff --git a/dds-topology-lib/src/TopoContainer.cpp b/dds-topology-lib/src/TopoContainer.cpp
@@ -23,6 +23,10 @@ using namespace topology_api;
 CTopoContainer::CTopoContainer(const std::string& _name)
     : CTopoElement(_name)
 {
+    if (_name == "main")
+    {
+        setNameToValueCache(make_shared<CTopoBase::CNameToValueCachePtr_t::element_type>());
+    }
 }
 
 CTopoContainer::~CTopoContainer()

diff --git a/dds-topology-lib/src/TopoContainer.h b/dds-topology-lib/src/TopoContainer.h
@@ -82,6 +82,7 @@ namespace dds
             {
                 auto element{ std::make_shared<Object_t>(_name) };
                 element->setParent(this);
+                element->setNameToValueCache(getNameToValueCache());
                 m_elements.push_back(element);
                 return element;
             }

diff --git a/dds-topology-lib/src/TopoTask.cpp b/dds-topology-lib/src/TopoTask.cpp
@@ -77,6 +77,7 @@ CTopoAsset::Ptr_t CTopoTask::addAsset(const string& _name)
 {
     auto asset{ make_shared<CTopoAsset>(_name) };
     asset->setParent(this);
+    asset->setNameToValueCache(getNameToValueCache());
     m_assets.push_back(asset);
     return asset;
 }