From 0868abf1fa07c2046ddd455fdd9748115fa50c2b Mon Sep 17 00:00:00 2001 From: Arjo Chakravarty Date: Fri, 6 Sep 2024 07:42:09 +0000 Subject: [PATCH 1/4] Attempting to see if adding a cache will speed up the `sdf::Root` construction I'm just checking if adding a cache makes any sense. Signed-off-by: Arjo Chakravarty --- include/sdf/Param.hh | 9 +++++++++ include/sdf/SDFImpl.hh | 3 ++- src/Element.cc | 4 ++-- src/Param.cc | 7 +++++++ src/Root.cc | 11 +++++++++-- src/SDF.cc | 5 ++++- src/SDFImplPrivate.hh | 9 ++++++++- 7 files changed, 41 insertions(+), 7 deletions(-) diff --git a/include/sdf/Param.hh b/include/sdf/Param.hh index d0a952f4c..5a6be379d 100644 --- a/include/sdf/Param.hh +++ b/include/sdf/Param.hh @@ -326,6 +326,15 @@ namespace sdf public: bool SetParentElement(ElementPtr _parentElement, sdf::Errors &_errors); + /// \brief Set the parent Element of this Param. + /// \param[in] _parentElement Pointer to new parent Element. A nullptr can + /// be provided to remove the current parent Element. + /// \param[out] _errors Vector of errors. + /// \return True if the parent Element was set and the value was reparsed + /// successfully. + public: bool SetParentElementNoReparse( + ElementPtr _parentElement); + /// \brief Reset the parameter to the default value. public: void Reset(); diff --git a/include/sdf/SDFImpl.hh b/include/sdf/SDFImpl.hh index fb7e96ea0..e4d57fc1e 100644 --- a/include/sdf/SDFImpl.hh +++ b/include/sdf/SDFImpl.hh @@ -166,8 +166,9 @@ namespace sdf class SDFORMAT_VISIBLE SDF { public: SDF(); + public: SDF(const SDF& other);//Copy constructor /// \brief Destructor - public: ~SDF(); + public: virtual ~SDF(); public: void PrintDescription(); public: void PrintDescription(sdf::Errors &_errors); public: void PrintDoc(); diff --git a/src/Element.cc b/src/Element.cc index fd8950d7f..7c95b6a9c 100644 --- a/src/Element.cc +++ b/src/Element.cc @@ -256,7 +256,7 @@ ElementPtr Element::Clone(sdf::Errors &_errors) const aiter != this->dataPtr->attributes.end(); ++aiter) { auto clonedAttribute = (*aiter)->Clone(); - SDF_ASSERT(clonedAttribute->SetParentElement(clone), + SDF_ASSERT(clonedAttribute->SetParentElementNoReparse(clone), "Cannot set parent Element of cloned attribute Param to cloned " "Element."); clone->dataPtr->attributes.push_back(clonedAttribute); @@ -279,7 +279,7 @@ ElementPtr Element::Clone(sdf::Errors &_errors) const if (this->dataPtr->value) { clone->dataPtr->value = this->dataPtr->value->Clone(); - SDF_ASSERT(clone->dataPtr->value->SetParentElement(clone), + SDF_ASSERT(clone->dataPtr->value->SetParentElementNoReparse(clone), "Cannot set parent Element of cloned value Param to cloned Element."); } diff --git a/src/Param.cc b/src/Param.cc index 2336f4296..317d47a77 100644 --- a/src/Param.cc +++ b/src/Param.cc @@ -1326,6 +1326,13 @@ bool Param::SetParentElement(ElementPtr _parentElement, sdf::Errors &_errors) return true; } +////////////////////////////////////////////////// +bool Param::SetParentElementNoReparse(ElementPtr _parentElement) +{ + this->dataPtr->parentElement = _parentElement; + return true; +} + ////////////////////////////////////////////////// void Param::Reset() { diff --git a/src/Root.cc b/src/Root.cc index 03349d2af..bd4120df8 100644 --- a/src/Root.cc +++ b/src/Root.cc @@ -213,12 +213,19 @@ Errors Root::LoadSdfString(const std::string &_sdf) return this->LoadSdfString(_sdf, ParserConfig::GlobalConfig()); } +static SDFPtr cachedRoot; +static bool cacheActive = false; + ///////////////////////////////////////////////// Errors Root::LoadSdfString(const std::string &_sdf, const ParserConfig &_config) { Errors errors; - SDFPtr sdfParsed(new SDF()); - init(sdfParsed); + if (!cacheActive) { + cacheActive = true; + cachedRoot = std::make_shared(); + init(cachedRoot); + } + SDFPtr sdfParsed = std::make_shared(*cachedRoot); // Read an SDF string, and store the result in sdfParsed. if (!readString(_sdf, _config, sdfParsed, errors)) diff --git a/src/SDF.cc b/src/SDF.cc index d9fa57dbd..87e982936 100644 --- a/src/SDF.cc +++ b/src/SDF.cc @@ -220,7 +220,10 @@ SDF::SDF() : dataPtr(new SDFPrivate) { } - +SDF::SDF(const SDF& other) +{ + this->dataPtr = std::make_unique(*other.dataPtr); +} ///////////////////////////////////////////////// SDF::~SDF() { diff --git a/src/SDFImplPrivate.hh b/src/SDFImplPrivate.hh index eefc57fcd..6d8183e07 100644 --- a/src/SDFImplPrivate.hh +++ b/src/SDFImplPrivate.hh @@ -34,7 +34,14 @@ namespace sdf { public: SDFPrivate() : root(new Element) { - }; + } + + public: SDFPrivate(const SDFPrivate& other) + { + this->path = other.path; + this->originalVersion = other.originalVersion; + this->root = other.root->Clone(); + } /// \brief Store the root element. /// \sa ElementPtr Root() From 91228de41c81de46b51bb13f6100dfa7b9e35564 Mon Sep 17 00:00:00 2001 From: Arjo Chakravarty Date: Tue, 1 Oct 2024 15:30:29 +0800 Subject: [PATCH 2/4] Remove cacheing code. Make faster. Signed-off-by: Arjo Chakravarty --- include/sdf/SDFImpl.hh | 3 +-- src/Root.cc | 13 +++---------- src/SDF.cc | 5 +---- src/SDFImplPrivate.hh | 7 ------- 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/include/sdf/SDFImpl.hh b/include/sdf/SDFImpl.hh index e4d57fc1e..fb7e96ea0 100644 --- a/include/sdf/SDFImpl.hh +++ b/include/sdf/SDFImpl.hh @@ -166,9 +166,8 @@ namespace sdf class SDFORMAT_VISIBLE SDF { public: SDF(); - public: SDF(const SDF& other);//Copy constructor /// \brief Destructor - public: virtual ~SDF(); + public: ~SDF(); public: void PrintDescription(); public: void PrintDescription(sdf::Errors &_errors); public: void PrintDoc(); diff --git a/src/Root.cc b/src/Root.cc index bd4120df8..a8213b68e 100644 --- a/src/Root.cc +++ b/src/Root.cc @@ -213,19 +213,12 @@ Errors Root::LoadSdfString(const std::string &_sdf) return this->LoadSdfString(_sdf, ParserConfig::GlobalConfig()); } -static SDFPtr cachedRoot; -static bool cacheActive = false; - ///////////////////////////////////////////////// Errors Root::LoadSdfString(const std::string &_sdf, const ParserConfig &_config) { Errors errors; - if (!cacheActive) { - cacheActive = true; - cachedRoot = std::make_shared(); - init(cachedRoot); - } - SDFPtr sdfParsed = std::make_shared(*cachedRoot); + SDFPtr sdfParsed(new SDF()); + init(sdfParsed); // Read an SDF string, and store the result in sdfParsed. if (!readString(_sdf, _config, sdfParsed, errors)) @@ -650,4 +643,4 @@ sdf::ElementPtr Root::ToElement(const OutputConfig &_config) const } return elem; -} +} \ No newline at end of file diff --git a/src/SDF.cc b/src/SDF.cc index 87e982936..d9fa57dbd 100644 --- a/src/SDF.cc +++ b/src/SDF.cc @@ -220,10 +220,7 @@ SDF::SDF() : dataPtr(new SDFPrivate) { } -SDF::SDF(const SDF& other) -{ - this->dataPtr = std::make_unique(*other.dataPtr); -} + ///////////////////////////////////////////////// SDF::~SDF() { diff --git a/src/SDFImplPrivate.hh b/src/SDFImplPrivate.hh index 6d8183e07..93fe93e51 100644 --- a/src/SDFImplPrivate.hh +++ b/src/SDFImplPrivate.hh @@ -36,13 +36,6 @@ namespace sdf { } - public: SDFPrivate(const SDFPrivate& other) - { - this->path = other.path; - this->originalVersion = other.originalVersion; - this->root = other.root->Clone(); - } - /// \brief Store the root element. /// \sa ElementPtr Root() /// \sa void Root(const ElementPtr _root) From 09770597d34f79cf57c65a2b1d6b646e247ee363 Mon Sep 17 00:00:00 2001 From: Arjo Chakravarty Date: Tue, 1 Oct 2024 15:50:18 +0800 Subject: [PATCH 3/4] Style Signed-off-by: Arjo Chakravarty --- src/Root.cc | 2 +- src/SDFImplPrivate.hh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Root.cc b/src/Root.cc index a8213b68e..03349d2af 100644 --- a/src/Root.cc +++ b/src/Root.cc @@ -643,4 +643,4 @@ sdf::ElementPtr Root::ToElement(const OutputConfig &_config) const } return elem; -} \ No newline at end of file +} diff --git a/src/SDFImplPrivate.hh b/src/SDFImplPrivate.hh index 93fe93e51..eefc57fcd 100644 --- a/src/SDFImplPrivate.hh +++ b/src/SDFImplPrivate.hh @@ -34,7 +34,7 @@ namespace sdf { public: SDFPrivate() : root(new Element) { - } + }; /// \brief Store the root element. /// \sa ElementPtr Root() From 08f157ddf45c1a356b90d5f5b8b7c4d2b9c97f90 Mon Sep 17 00:00:00 2001 From: Arjo Chakravarty Date: Thu, 10 Oct 2024 09:41:22 +0800 Subject: [PATCH 4/4] Docstring Signed-off-by: Arjo Chakravarty --- include/sdf/Param.hh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/sdf/Param.hh b/include/sdf/Param.hh index 5a6be379d..924849ab9 100644 --- a/include/sdf/Param.hh +++ b/include/sdf/Param.hh @@ -326,12 +326,12 @@ namespace sdf public: bool SetParentElement(ElementPtr _parentElement, sdf::Errors &_errors); - /// \brief Set the parent Element of this Param. + /// \brief Set the parent Element of this Param without reparsing. + /// This is meant for internal consumption when cloning elements. /// \param[in] _parentElement Pointer to new parent Element. A nullptr can /// be provided to remove the current parent Element. /// \param[out] _errors Vector of errors. - /// \return True if the parent Element was set and the value was reparsed - /// successfully. + /// \return True if the parent Element was set. public: bool SetParentElementNoReparse( ElementPtr _parentElement);