From acc6248ec98ef8cd24ea057f679d90ca6be812e5 Mon Sep 17 00:00:00 2001 From: Nicholas Van Sickle Date: Wed, 5 Jan 2022 11:31:09 -0800 Subject: [PATCH] Move deep comparison / copy to utils Signed-off-by: Nicholas Van Sickle --- Code/Framework/AzCore/AzCore/DOM/DomUtils.cpp | 148 +++++++++++++++++ Code/Framework/AzCore/AzCore/DOM/DomUtils.h | 3 + Code/Framework/AzCore/AzCore/DOM/DomValue.cpp | 155 +++--------------- Code/Framework/AzCore/AzCore/DOM/DomValue.h | 13 +- .../AzCore/Tests/DOM/DomValueBenchmarks.cpp | 3 +- .../AzCore/Tests/DOM/DomValueTests.cpp | 6 +- 6 files changed, 186 insertions(+), 142 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/DOM/DomUtils.cpp b/Code/Framework/AzCore/AzCore/DOM/DomUtils.cpp index 73c4bd2d76..c604373296 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomUtils.cpp +++ b/Code/Framework/AzCore/AzCore/DOM/DomUtils.cpp @@ -33,4 +33,152 @@ namespace AZ::Dom::Utils } return AZ::Success(AZStd::move(value)); } + + bool DeepCompareIsEqual(const Value& lhs, const Value& rhs) + { + const Value::ValueType& lhsValue = lhs.GetInternalValue(); + const Value::ValueType& rhsValue = rhs.GetInternalValue(); + + if (lhs.IsString() && rhs.IsString()) + { + // If we both hold the same ref counted string we don't need to do a full comparison + if (AZStd::holds_alternative(lhsValue) && lhsValue == rhsValue) + { + return true; + } + return lhs.GetString() == rhs.GetString(); + } + + return AZStd::visit( + [&](auto&& ourValue) -> bool + { + using Alternative = AZStd::decay_t; + + if constexpr (AZStd::is_same_v) + { + if (!rhs.IsObject()) + { + return false; + } + auto&& theirValue = AZStd::get>(rhsValue); + if (ourValue == theirValue) + { + return true; + } + + const Object::ContainerType& ourValues = ourValue->GetValues(); + const Object::ContainerType& theirValues = theirValue->GetValues(); + + if (ourValues.size() != theirValues.size()) + { + return false; + } + + for (size_t i = 0; i < ourValues.size(); ++i) + { + const Object::EntryType& lhsChild = ourValues[i]; + const Object::EntryType& rhsChild = theirValues[i]; + if (lhsChild.first != rhsChild.first || !DeepCompareIsEqual(lhsChild.second, rhsChild.second)) + { + return false; + } + } + + return true; + } + else if constexpr (AZStd::is_same_v) + { + if (!rhs.IsArray()) + { + return false; + } + auto&& theirValue = AZStd::get>(rhsValue); + if (ourValue == theirValue) + { + return true; + } + + const Array::ContainerType& ourValues = ourValue->GetValues(); + const Array::ContainerType& theirValues = theirValue->GetValues(); + + if (ourValues.size() != theirValues.size()) + { + return false; + } + + for (size_t i = 0; i < ourValues.size(); ++i) + { + const Value& lhsChild = ourValues[i]; + const Value& rhsChild = theirValues[i]; + if (!DeepCompareIsEqual(lhsChild, rhsChild)) + { + return false; + } + } + + return true; + } + else if constexpr (AZStd::is_same_v) + { + if (!rhs.IsNode()) + { + return false; + } + auto&& theirValue = AZStd::get>(rhsValue); + if (ourValue == theirValue) + { + return true; + } + + const Node& ourNode = *ourValue; + const Node& theirNode = *theirValue; + + const Object::ContainerType& ourProperties = ourNode.GetProperties(); + const Object::ContainerType& theirProperties = theirNode.GetProperties(); + + if (ourProperties.size() != theirProperties.size()) + { + return false; + } + + for (size_t i = 0; i < ourProperties.size(); ++i) + { + const Object::EntryType& lhsChild = ourProperties[i]; + const Object::EntryType& rhsChild = theirProperties[i]; + if (lhsChild.first != rhsChild.first || !DeepCompareIsEqual(lhsChild.second, rhsChild.second)) + { + return false; + } + } + + const Array::ContainerType& ourChildren = ourNode.GetChildren(); + const Array::ContainerType& theirChildren = theirNode.GetChildren(); + + for (size_t i = 0; i < ourChildren.size(); ++i) + { + const Value& lhsChild = ourChildren[i]; + const Value& rhsChild = theirChildren[i]; + if (!DeepCompareIsEqual(lhsChild, rhsChild)) + { + return false; + } + } + + return true; + } + else + { + return lhs == rhs; + } + }, + lhsValue); + } + + Value DeepCopy(const Value& value, bool copyStrings) + { + Value copiedValue; + AZStd::unique_ptr writer = copiedValue.GetWriteHandler(); + value.Accept(*writer, copyStrings); + return copiedValue; + } } // namespace AZ::Dom::Utils diff --git a/Code/Framework/AzCore/AzCore/DOM/DomUtils.h b/Code/Framework/AzCore/AzCore/DOM/DomUtils.h index ebe4273b48..5403b93714 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomUtils.h +++ b/Code/Framework/AzCore/AzCore/DOM/DomUtils.h @@ -17,4 +17,7 @@ namespace AZ::Dom::Utils Visitor::Result ReadFromStringInPlace(Backend& backend, AZStd::string& string, Visitor& visitor); AZ::Outcome WriteToValue(const Backend::WriteCallback& writeCallback); + + bool DeepCompareIsEqual(const Value& lhs, const Value& rhs); + Value DeepCopy(const Value& value, bool copyStrings = true); } // namespace AZ::Dom::Utils diff --git a/Code/Framework/AzCore/AzCore/DOM/DomValue.cpp b/Code/Framework/AzCore/AzCore/DOM/DomValue.cpp index 8c002b7091..af6d86d699 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomValue.cpp +++ b/Code/Framework/AzCore/AzCore/DOM/DomValue.cpp @@ -67,6 +67,16 @@ namespace AZ::Dom return Internal::ExtractTypeArgs::GetTypeIndex(); } + const Array::ContainerType& Array::GetValues() const + { + return m_values; + } + + const Object::ContainerType& Object::GetValues() const + { + return m_values; + } + Node::Node(AZ::Name name) : m_name(AZStd::move(name)) { @@ -130,8 +140,8 @@ namespace AZ::Dom } } - Value::Value(const AZStd::any& value) - : m_value(AZStd::allocate_shared(StdValueAllocator(), value)) + Value::Value(AZStd::any opaqueValue) + : m_value(AZStd::allocate_shared(StdValueAllocator(), AZStd::move(opaqueValue))) { } @@ -660,7 +670,9 @@ namespace AZ::Dom Value& Value::AddMember(KeyType name, const Value& value) { Object::ContainerType& object = GetObjectInternal(); - object.reserve((object.size() / Object::ReserveIncrement + 1) * Object::ReserveIncrement); + // Reserve in ReserveIncremenet chunks instead of the default vector doubling strategy + // Profiling has found that this is an aggregate performance gain for typical workflows + object.reserve(AZ_SIZE_ALIGN_UP(object.size() + 1, Object::ReserveIncrement)); if (auto memberIt = FindMutableMember(name); memberIt != object.end()) { memberIt->second = value; @@ -833,7 +845,9 @@ namespace AZ::Dom Value& Value::ArrayPushBack(Value value) { Array::ContainerType& array = GetArrayInternal(); - array.reserve((array.size() / Array::ReserveIncrement + 1) * Array::ReserveIncrement); + // Reserve in ReserveIncremenet chunks instead of the default vector doubling strategy + // Profiling has found that this is an aggregate performance gain for typical workflows + array.reserve(AZ_SIZE_ALIGN_UP(array.size() + 1, Array::ReserveIncrement)); array.push_back(AZStd::move(value)); return *this; } @@ -1231,137 +1245,8 @@ namespace AZ::Dom return AZStd::make_unique(*this); } - bool Value::DeepCompareIsEqual(const Value& other) const - { - if (IsString() && other.IsString()) - { - // If we both hold the same ref counted string we don't need to do a full comparison - if (AZStd::holds_alternative(m_value) && m_value == other.m_value) - { - return true; - } - return GetString() == other.GetString(); - } - - if (m_value.index() != other.m_value.index()) - { - return false; - } - - return AZStd::visit( - [&](auto&& ourValue) -> bool - { - using Alternative = AZStd::decay_t; - auto&& theirValue = AZStd::get>(other.m_value); - - if constexpr (AZStd::is_same_v) - { - return true; - } - else if constexpr (AZStd::is_same_v) - { - if (ourValue == theirValue) - { - return true; - } - - if (ourValue->m_values.size() != theirValue->m_values.size()) - { - return false; - } - - for (size_t i = 0; i < ourValue->m_values.size(); ++i) - { - const Object::EntryType& lhs = ourValue->m_values[i]; - const Object::EntryType& rhs = theirValue->m_values[i]; - if (lhs.first != rhs.first || !lhs.second.DeepCompareIsEqual(rhs.second)) - { - return false; - } - } - - return true; - } - else if constexpr (AZStd::is_same_v) - { - if (ourValue == theirValue) - { - return true; - } - - if (ourValue->m_values.size() != theirValue->m_values.size()) - { - return false; - } - - for (size_t i = 0; i < ourValue->m_values.size(); ++i) - { - const Value& lhs = ourValue->m_values[i]; - const Value& rhs = theirValue->m_values[i]; - if (!lhs.DeepCompareIsEqual(rhs)) - { - return false; - } - } - - return true; - } - else if constexpr (AZStd::is_same_v) - { - if (ourValue == theirValue) - { - return true; - } - - const Node& ourNode = *ourValue; - const Node& theirNode = *theirValue; - - const Object::ContainerType& ourProperties = ourNode.GetProperties(); - const Object::ContainerType& theirProperties = theirNode.GetProperties(); - - if (ourProperties.size() != theirProperties.size()) - { - return false; - } - - for (size_t i = 0; i < ourProperties.size(); ++i) - { - const Object::EntryType& lhs = ourProperties[i]; - const Object::EntryType& rhs = theirProperties[i]; - if (lhs.first != rhs.first || !lhs.second.DeepCompareIsEqual(rhs.second)) - { - return false; - } - } - - const Array::ContainerType& ourChildren = ourNode.GetChildren(); - const Array::ContainerType& theirChildren = theirNode.GetChildren(); - - for (size_t i = 0; i < ourChildren.size(); ++i) - { - const Value& lhs = ourChildren[i]; - const Value& rhs = theirChildren[i]; - if (!lhs.DeepCompareIsEqual(rhs)) - { - return false; - } - } - - return true; - } - else - { - return ourValue == theirValue; - } - }, - m_value); - } - - Value Value::DeepCopy(bool copyStrings) const + const Value::ValueType& Value::GetInternalValue() const { - Value newValue; - AZStd::unique_ptr writer = newValue.GetWriteHandler(); - Accept(*writer, copyStrings); - return newValue; + return m_value; } } // namespace AZ::Dom diff --git a/Code/Framework/AzCore/AzCore/DOM/DomValue.h b/Code/Framework/AzCore/AzCore/DOM/DomValue.h index d67947dd0a..a6990fcbce 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomValue.h +++ b/Code/Framework/AzCore/AzCore/DOM/DomValue.h @@ -67,6 +67,9 @@ namespace AZ::Dom using Iterator = ContainerType::iterator; using ConstIterator = ContainerType::const_iterator; static constexpr const size_t ReserveIncrement = 4; + static_assert((ReserveIncrement & (ReserveIncrement - 1)) == 0, "ReserveIncremenet must be a power of 2"); + + const ContainerType& GetValues() const; private: ContainerType m_values; @@ -86,6 +89,9 @@ namespace AZ::Dom using Iterator = ContainerType::iterator; using ConstIterator = ContainerType::const_iterator; static constexpr const size_t ReserveIncrement = 8; + static_assert((ReserveIncrement & (ReserveIncrement - 1)) == 0, "ReserveIncremenet must be a power of 2"); + + const ContainerType& GetValues() const; private: ContainerType m_values; @@ -374,8 +380,9 @@ namespace AZ::Dom Visitor::Result Accept(Visitor& visitor, bool copyStrings) const; AZStd::unique_ptr GetWriteHandler(); - bool DeepCompareIsEqual(const Value& other) const; - Value DeepCopy(bool copyStrings = true) const; + //! Gets the internal value of this Value. Note that this value's types may not correspond one-to-one with the Type enumeration, + //! as internally the same type might have different storage mechanisms. Where possible, prefer using the typed API. + const ValueType& GetInternalValue() const; private: const Node& GetNodeInternal() const; @@ -385,7 +392,7 @@ namespace AZ::Dom const Array::ContainerType& GetArrayInternal() const; Array::ContainerType& GetArrayInternal(); - explicit Value(const AZStd::any& opaqueValue); + explicit Value(AZStd::any opaqueValue); static_assert( sizeof(ValueType) == sizeof(ShortStringType) + sizeof(size_t), "ValueType should have no members larger than ShortStringType"); diff --git a/Code/Framework/AzCore/Tests/DOM/DomValueBenchmarks.cpp b/Code/Framework/AzCore/Tests/DOM/DomValueBenchmarks.cpp index 74eb78be00..b20091b232 100644 --- a/Code/Framework/AzCore/Tests/DOM/DomValueBenchmarks.cpp +++ b/Code/Framework/AzCore/Tests/DOM/DomValueBenchmarks.cpp @@ -7,6 +7,7 @@ */ #include +#include #include #include #include @@ -171,7 +172,7 @@ namespace AZ::Dom::Benchmark for (auto _ : state) { - Value copy = original.DeepCopy(); + Value copy = Utils::DeepCopy(original); TakeAndDiscardWithoutTimingDtor(AZStd::move(copy), state); } diff --git a/Code/Framework/AzCore/Tests/DOM/DomValueTests.cpp b/Code/Framework/AzCore/Tests/DOM/DomValueTests.cpp index 3cbd532b13..a98eb307a2 100644 --- a/Code/Framework/AzCore/Tests/DOM/DomValueTests.cpp +++ b/Code/Framework/AzCore/Tests/DOM/DomValueTests.cpp @@ -41,10 +41,10 @@ namespace AZ::Dom::Tests { Value shallowCopy = m_value; EXPECT_EQ(m_value, shallowCopy); - EXPECT_TRUE(m_value.DeepCompareIsEqual(shallowCopy)); + EXPECT_TRUE(Utils::DeepCompareIsEqual(m_value, shallowCopy)); - Value deepCopy = m_value.DeepCopy(); - EXPECT_TRUE(m_value.DeepCompareIsEqual(deepCopy)); + Value deepCopy = Utils::DeepCopy(m_value); + EXPECT_TRUE(Utils::DeepCompareIsEqual(m_value, deepCopy)); } Value m_value;