From 9e2ec414794bbc133520a000c69a637425767e9a Mon Sep 17 00:00:00 2001 From: Anar Manafov Date: Tue, 28 Jun 2022 15:16:59 +0200 Subject: [PATCH] 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) --- ReleaseNotes.md | 1 + dds-topology-lib/src/TopoAsset.cpp | 34 +++++++++++++++++------- dds-topology-lib/src/TopoBase.cpp | 10 +++++++ dds-topology-lib/src/TopoBase.h | 36 ++++++++++++++++++++++++++ dds-topology-lib/src/TopoContainer.cpp | 7 ++++- dds-topology-lib/src/TopoContainer.h | 1 + dds-topology-lib/src/TopoTask.cpp | 1 + 7 files changed, 80 insertions(+), 10 deletions(-) diff --git a/ReleaseNotes.md b/ReleaseNotes.md index 34e60ca2..d2adf911 100644 --- 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 index 8688dd3d..e4f088b7 100644 --- 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(".type", ""))); - setAssetVisibility(TagToAssetVisibility(assetPT.get(".visibility", ""))); - setValue(assetPT.get(".value", "")); + setAssetType(TagToAssetType(assetPT.get(".type", ""))); + setAssetVisibility(TagToAssetVisibility(assetPT.get(".visibility", ""))); + + // Add asset value if the asset container doesn't have it. + if (getNameToValueCache() && !getNameToValueCache()->exists(getName())) + { + getNameToValueCache()->insertValue(getName(), assetPT.get(".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."); + string tag("topology.asset."); _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 index 6681eb05..b915efac 100644 --- 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 index be1fa74f..cc1fca05 100644 --- 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; + + 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; using PtrVector_t = std::vector; + using CNameToValueCachePtr_t = std::shared_ptr; /// 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 index 50a1c6da..e457cd89 100644 --- 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()); + } } CTopoContainer::~CTopoContainer() @@ -51,7 +55,8 @@ CTopoTask::Ptr_t CTopoContainer::addElement(const std::string& _name) { if (!canAddElement(CTopoBase::EType::TASK)) throw runtime_error("Can't add task to TopoContainer"); - return makeElement(_name); + auto task{ makeElement(_name) }; + return task; } template <> diff --git a/dds-topology-lib/src/TopoContainer.h b/dds-topology-lib/src/TopoContainer.h index 79050370..df08d101 100644 --- 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(_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 index dbc7e6a2..9d1a98c5 100644 --- 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(_name) }; asset->setParent(this); + asset->setNameToValueCache(getNameToValueCache()); m_assets.push_back(asset); return asset; }