diff --git a/Code/Framework/AzCore/AzCore/DOM/DomPatch.cpp b/Code/Framework/AzCore/AzCore/DOM/DomPatch.cpp index 963308e9c8..6e53de0689 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomPatch.cpp +++ b/Code/Framework/AzCore/AzCore/DOM/DomPatch.cpp @@ -8,8 +8,8 @@ #include #include -#include #include +#include namespace AZ::Dom { @@ -257,7 +257,8 @@ namespace AZ::Dom return AZ::Failure(sourceLoad.TakeError()); } - return AZ::Success(PatchOperation::CopyOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString()))); + return AZ::Success( + PatchOperation::CopyOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString()))); } else if (op == "move") { @@ -272,7 +273,8 @@ namespace AZ::Dom return AZ::Failure(sourceLoad.TakeError()); } - return AZ::Success(PatchOperation::MoveOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString()))); + return AZ::Success( + PatchOperation::MoveOperation(Path(destLoad.GetValue().GetString()), Path(sourceLoad.GetValue().GetString()))); } else if (op == "test") { @@ -319,8 +321,9 @@ namespace AZ::Dom const Value* existingValue = stateBeforeApplication.FindChild(m_domPath); if (existingValue == nullptr) { - return AZ::Failure( - AZStd::string::format("Unable to invert DOM remove patch, source path not found: %s", m_domPath.ToString().data())); + AZStd::string errorMessage = "Unable to invert DOM remove patch, source path not found: "; + m_domPath.AppendToString(errorMessage); + return AZ::Failure(AZStd::move(errorMessage)); } return AZ::Success(PatchOperation::AddOperation(m_domPath, *existingValue)); } @@ -330,8 +333,9 @@ namespace AZ::Dom const Value* existingValue = stateBeforeApplication.FindChild(m_domPath); if (existingValue == nullptr) { - return AZ::Failure(AZStd::string::format( - "Unable to invert DOM replace patch, source path not found: %s", m_domPath.ToString().data())); + AZStd::string errorMessage = "Unable to invert DOM replace patch, source path not found: "; + m_domPath.AppendToString(errorMessage); + return AZ::Failure(AZStd::move(errorMessage)); } return AZ::Success(PatchOperation::ReplaceOperation(m_domPath, *existingValue)); } @@ -341,8 +345,9 @@ namespace AZ::Dom const Value* existingValue = stateBeforeApplication.FindChild(m_domPath); if (existingValue == nullptr) { - return AZ::Failure( - AZStd::string::format("Unable to invert DOM copy patch, source path not found: %s", m_domPath.ToString().data())); + AZStd::string errorMessage = "Unable to invert DOM copy patch, source path not found: "; + m_domPath.AppendToString(errorMessage); + return AZ::Failure(AZStd::move(errorMessage)); } return AZ::Success(PatchOperation::ReplaceOperation(m_domPath, *existingValue)); } @@ -367,8 +372,9 @@ namespace AZ::Dom const Value* existingValue = stateBeforeApplication.FindChild(commonAncestor); if (existingValue == nullptr) { - return AZ::Failure(AZStd::string::format( - "Unable to invert DOM move patch, common ancestor path not found: %s", commonAncestor.ToString().data())); + AZStd::string errorMessage = "Unable to invert DOM copy patch, common ancestor path not found: "; + commonAncestor.AppendToString(errorMessage); + return AZ::Failure(AZStd::move(errorMessage)); } return AZ::Success(PatchOperation::ReplaceOperation(commonAncestor, *existingValue)); } @@ -383,10 +389,10 @@ namespace AZ::Dom } AZ::Outcome PatchOperation::LookupPath( - Value& rootElement, const Path& path, AZ::u8 existenceCheckFlags) + Value& rootElement, const Path& path, ExistenceCheckFlags flags) { - const bool verifyFullPath = existenceCheckFlags & VerifyFullPath; - const bool allowEndOfArray = existenceCheckFlags & AllowEndOfArray; + const bool verifyFullPath = (flags & ExistenceCheckFlags::VerifyFullPath) != ExistenceCheckFlags::DefaultExistenceCheck; + const bool allowEndOfArray = (flags & ExistenceCheckFlags::AllowEndOfArray) != ExistenceCheckFlags::DefaultExistenceCheck; Path target = path; if (target.Size() == 0) @@ -414,7 +420,9 @@ namespace AZ::Dom Value* targetValue = rootElement.FindMutableChild(target); if (targetValue == nullptr) { - return AZ::Failure(AZStd::string::format("Path not found (%s)", target.ToString().data())); + AZStd::string errorMessage = "Path not found: "; + target.AppendToString(errorMessage); + return AZ::Failure(AZStd::move(errorMessage)); } if (destinationIndex.IsIndex() || destinationIndex.IsEndOfArray()) @@ -450,7 +458,7 @@ namespace AZ::Dom PatchOperation::PatchOutcome PatchOperation::ApplyAdd(Value& rootElement) const { - auto pathLookup = LookupPath(rootElement, m_domPath, AllowEndOfArray); + auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::AllowEndOfArray); if (!pathLookup.IsSuccess()) { return AZ::Failure(pathLookup.TakeError()); @@ -481,7 +489,7 @@ namespace AZ::Dom PatchOperation::PatchOutcome PatchOperation::ApplyRemove(Value& rootElement) const { - auto pathLookup = LookupPath(rootElement, m_domPath, VerifyFullPath | AllowEndOfArray); + auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::VerifyFullPath | ExistenceCheckFlags::AllowEndOfArray); if (!pathLookup.IsSuccess()) { return AZ::Failure(pathLookup.TakeError()); @@ -505,7 +513,7 @@ namespace AZ::Dom PatchOperation::PatchOutcome PatchOperation::ApplyReplace(Value& rootElement) const { - auto pathLookup = LookupPath(rootElement, m_domPath, VerifyFullPath); + auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::VerifyFullPath); if (!pathLookup.IsSuccess()) { return AZ::Failure(pathLookup.TakeError()); @@ -517,13 +525,13 @@ namespace AZ::Dom PatchOperation::PatchOutcome PatchOperation::ApplyCopy(Value& rootElement) const { - auto sourceLookup = LookupPath(rootElement, GetSourcePath(), VerifyFullPath); + auto sourceLookup = LookupPath(rootElement, GetSourcePath(), ExistenceCheckFlags::VerifyFullPath); if (!sourceLookup.IsSuccess()) { return AZ::Failure(sourceLookup.TakeError()); } - auto destLookup = LookupPath(rootElement, m_domPath, AllowEndOfArray); + auto destLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::AllowEndOfArray); if (!destLookup.IsSuccess()) { return AZ::Failure(destLookup.TakeError()); @@ -535,13 +543,13 @@ namespace AZ::Dom PatchOperation::PatchOutcome PatchOperation::ApplyMove(Value& rootElement) const { - auto sourceLookup = LookupPath(rootElement, GetSourcePath(), VerifyFullPath); + auto sourceLookup = LookupPath(rootElement, GetSourcePath(), ExistenceCheckFlags::VerifyFullPath); if (!sourceLookup.IsSuccess()) { return AZ::Failure(sourceLookup.TakeError()); } - auto destLookup = LookupPath(rootElement, m_domPath, AllowEndOfArray); + auto destLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::AllowEndOfArray); if (!destLookup.IsSuccess()) { return AZ::Failure(destLookup.TakeError()); @@ -568,7 +576,7 @@ namespace AZ::Dom PatchOperation::PatchOutcome PatchOperation::ApplyTest(Value& rootElement) const { - auto pathLookup = LookupPath(rootElement, m_domPath, VerifyFullPath); + auto pathLookup = LookupPath(rootElement, m_domPath, ExistenceCheckFlags::VerifyFullPath); if (!pathLookup.IsSuccess()) { return AZ::Failure(pathLookup.TakeError()); @@ -670,32 +678,32 @@ namespace AZ::Dom return m_operations[index]; } - Patch::OperationsContainer::iterator Patch::begin() + auto Patch::begin() -> OperationsContainer::iterator { return m_operations.begin(); } - Patch::OperationsContainer::iterator Patch::end() + auto Patch::end() -> OperationsContainer::iterator { return m_operations.end(); } - Patch::OperationsContainer::const_iterator Patch::begin() const + auto Patch::begin() const -> OperationsContainer::const_iterator { return m_operations.cbegin(); } - Patch::OperationsContainer::const_iterator Patch::end() const + auto Patch::end() const -> OperationsContainer::const_iterator { return m_operations.cend(); } - Patch::OperationsContainer::const_iterator Patch::cbegin() const + auto Patch::cbegin() const -> OperationsContainer::const_iterator { return m_operations.cbegin(); } - Patch::OperationsContainer::const_iterator Patch::cend() const + auto Patch::cend() const -> OperationsContainer::const_iterator { return m_operations.cend(); } @@ -794,7 +802,8 @@ namespace AZ::Dom return PatchOperation(AZStd::move(testPath), PatchOperation::Type::Test, AZStd::move(value)); } - PatchInfo GenerateHierarchicalDeltaPatch(const Value& beforeState, const Value& afterState) + PatchInfo GenerateHierarchicalDeltaPatch( + const Value& beforeState, const Value& afterState, const DeltaPatchGenerationParameters& params) { PatchInfo patches; @@ -804,7 +813,7 @@ namespace AZ::Dom patches.m_inversePatches.PushFront(AZStd::move(inverse)); }; - AZStd::function CompareValues; + AZStd::function compareValues; struct PendingComparison { @@ -822,7 +831,7 @@ namespace AZ::Dom AZStd::queue entriesToCompare; AZStd::unordered_set desiredKeys; - auto CompareObjects = [&](const Path& path, const Value& before, const Value& after) + auto compareObjects = [&](const Path& path, const Value& before, const Value& after) { desiredKeys.clear(); Path subPath = path; @@ -853,22 +862,22 @@ namespace AZ::Dom } }; - auto CompareArrays = [&](const Path& path, const Value& before, const Value& after) + auto compareArrays = [&](const Path& path, const Value& before, const Value& after) { const size_t beforeSize = before.ArraySize(); const size_t afterSize = after.ArraySize(); // If more than replaceThreshold values differ, do a replace operation instead - constexpr size_t replaceThreshold = 3; - size_t changedValueCount = 0; - for (size_t i = 0; i < afterSize; ++i) + if (params.m_replaceThreshold != DeltaPatchGenerationParameters::NoReplace) { - if (i < beforeSize) + size_t changedValueCount = 0; + const size_t entriesToEnumerate = AZStd::min(beforeSize, afterSize); + for (size_t i = 0; i < entriesToEnumerate; ++i) { if (before[i] != after[i]) { ++changedValueCount; - if (changedValueCount >= replaceThreshold) + if (changedValueCount >= params.m_replaceThreshold) { AddPatch(PatchOperation::ReplaceOperation(path, after), PatchOperation::ReplaceOperation(path, before)); return; @@ -904,7 +913,7 @@ namespace AZ::Dom } }; - auto CompareNodes = [&](const Path& path, const Value& before, const Value& after) + auto compareNodes = [&](const Path& path, const Value& before, const Value& after) { if (before.GetNodeName() != after.GetNodeName()) { @@ -912,12 +921,12 @@ namespace AZ::Dom } else { - CompareObjects(path, before, after); - CompareArrays(path, before, after); + compareObjects(path, before, after); + compareArrays(path, before, after); } }; - CompareValues = [&](const Path& path, const Value& before, const Value& after) + compareValues = [&](const Path& path, const Value& before, const Value& after) { if (before.GetType() != after.GetType()) { @@ -931,15 +940,15 @@ namespace AZ::Dom } else if (before.IsObject()) { - CompareObjects(path, before, after); + compareObjects(path, before, after); } else if (before.IsArray()) { - CompareArrays(path, before, after); + compareArrays(path, before, after); } else if (before.IsNode()) { - CompareNodes(path, before, after); + compareNodes(path, before, after); } else { @@ -951,7 +960,7 @@ namespace AZ::Dom while (!entriesToCompare.empty()) { PendingComparison& comparison = entriesToCompare.front(); - CompareValues(comparison.m_path, comparison.m_before, comparison.m_after); + compareValues(comparison.m_path, comparison.m_before, comparison.m_after); entriesToCompare.pop(); } return patches; diff --git a/Code/Framework/AzCore/AzCore/DOM/DomPatch.h b/Code/Framework/AzCore/AzCore/DOM/DomPatch.h index 9d9ad7560a..633a611c73 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomPatch.h +++ b/Code/Framework/AzCore/AzCore/DOM/DomPatch.h @@ -35,9 +35,9 @@ namespace AZ::Dom PatchOperation(const PatchOperation&) = default; PatchOperation(PatchOperation&&) = default; - explicit PatchOperation(Path destinationPath, Type type, Value value); - explicit PatchOperation(Path destionationPath, Type type, Path sourcePath); - explicit PatchOperation(Path path, Type type); + PatchOperation(Path destionationPath, Type type, Value value); + PatchOperation(Path destionationPath, Type type, Path sourcePath); + PatchOperation(Path path, Type type); static PatchOperation AddOperation(Path destinationPath, Value value); static PatchOperation RemoveOperation(Path pathToRemove); @@ -72,6 +72,13 @@ namespace AZ::Dom AZ::Outcome GetInverse(Value stateBeforeApplication) const; + enum class ExistenceCheckFlags : AZ::u8 + { + DefaultExistenceCheck = 0x0, + VerifyFullPath = 0x1, + AllowEndOfArray = 0x2, + }; + private: struct PathContext { @@ -79,12 +86,8 @@ namespace AZ::Dom PathEntry m_key; }; - static constexpr AZ::u8 DefaultExistenceCheck = 0x0; - static constexpr AZ::u8 VerifyFullPath = 0x1; - static constexpr AZ::u8 AllowEndOfArray = 0x2; - static AZ::Outcome LookupPath( - Value& rootElement, const Path& path, AZ::u8 existenceCheckFlags = DefaultExistenceCheck); + Value& rootElement, const Path& path, ExistenceCheckFlags existenceCheckFlags = ExistenceCheckFlags::DefaultExistenceCheck); PatchOutcome ApplyAdd(Value& rootElement) const; PatchOutcome ApplyRemove(Value& rootElement) const; @@ -93,11 +96,13 @@ namespace AZ::Dom PatchOutcome ApplyMove(Value& rootElement) const; PatchOutcome ApplyTest(Value& rootElement) const; + AZStd::variant m_value; Path m_domPath; Type m_type; - AZStd::variant m_value; }; + AZ_DEFINE_ENUM_BITWISE_OPERATORS(PatchOperation::ExistenceCheckFlags); + class Patch; //! The current state of a Patch application operation. @@ -184,8 +189,18 @@ namespace AZ::Dom Patch m_inversePatches; }; + //! Parameters for GenerateHierarchicalDeltaPatch. + struct DeltaPatchGenerationParameters + { + static constexpr size_t NoReplace = AZStd::numeric_limits::max(); + + //! The threshold of changed values in a node or array which, if exceeded, will cause the generation to create an + //! entire "replace" oepration instead. If set to NoReplace, no replacement will occur. + size_t m_replaceThreshold = 3; + }; + //! Generates a set of patches such that m_forwardPatches.Apply(beforeState) shall produce a document equivalent to afterState, and //! a subsequent m_inversePatches.Apply(beforeState) shall produce the original document. This patch generation strategy does a //! hierarchical comparison and is not guaranteed to create the minimal set of patches required to transform between the two states. - PatchInfo GenerateHierarchicalDeltaPatch(const Value& beforeState, const Value& afterState); + PatchInfo GenerateHierarchicalDeltaPatch(const Value& beforeState, const Value& afterState, const DeltaPatchGenerationParameters& params = {}); } // namespace AZ::Dom diff --git a/Code/Framework/AzCore/AzCore/DOM/DomPath.cpp b/Code/Framework/AzCore/AzCore/DOM/DomPath.cpp index a5d130d48c..f98dad4adb 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomPath.cpp +++ b/Code/Framework/AzCore/AzCore/DOM/DomPath.cpp @@ -54,19 +54,19 @@ namespace AZ::Dom bool PathEntry::operator==(size_t value) const { const size_t* internalValue = AZStd::get_if(&m_value); - return internalValue == nullptr ? false : (*internalValue) == value; + return internalValue != nullptr && *internalValue == value; } bool PathEntry::operator==(const AZ::Name& key) const { const AZ::Name* internalValue = AZStd::get_if(&m_value); - return internalValue == nullptr ? false : (*internalValue) == key; + return internalValue != nullptr && *internalValue == key; } bool PathEntry::operator==(AZStd::string_view key) const { const AZ::Name* internalValue = AZStd::get_if(&m_value); - return internalValue == nullptr ? false : (*internalValue) == AZ::Name(key); + return internalValue != nullptr && *internalValue == AZ::Name(key); } bool PathEntry::operator!=(const PathEntry& other) const @@ -323,13 +323,13 @@ namespace AZ::Dom return size; } - void Path::FormatString(char* stringBuffer, size_t bufferSize) const + size_t Path::FormatString(char* stringBuffer, size_t bufferSize) const { size_t bufferIndex = 0; auto putChar = [&](char c) { - if (bufferIndex == bufferSize) + if (bufferIndex >= bufferSize) { return; } @@ -360,6 +360,11 @@ namespace AZ::Dom for (const PathEntry& entry : m_entries) { + if (bufferIndex >= bufferSize) + { + return bufferIndex; + } + putChar(PathSeparator); if (entry.IsEndOfArray()) { @@ -375,7 +380,10 @@ namespace AZ::Dom } } + size_t bytesWritten = bufferIndex; putChar('\0'); + + return bytesWritten; } AZStd::string Path::ToString() const diff --git a/Code/Framework/AzCore/AzCore/DOM/DomPath.h b/Code/Framework/AzCore/AzCore/DOM/DomPath.h index 39f7c7da98..f1a95b4e70 100644 --- a/Code/Framework/AzCore/AzCore/DOM/DomPath.h +++ b/Code/Framework/AzCore/AzCore/DOM/DomPath.h @@ -128,10 +128,19 @@ namespace AZ::Dom size_t GetStringLength() const; //! Formats a JSON-pointer style path string into the target buffer. //! This operation will fail if bufferSize < GetStringLength() + 1 - void FormatString(char* stringBuffer, size_t bufferSize) const; + //! \return The number of bytes written, excepting the null terminator. + size_t FormatString(char* stringBuffer, size_t bufferSize) const; //! Returns a JSON-pointer style path string for this path. AZStd::string ToString() const; + void AppendToString(AZStd::string& output) const; + template + void AppendToString(T& output) const + { + const size_t startIndex = output.length(); + output.resize_no_construct(startIndex + FormatString(output.data() + startIndex, output.capacity() - startIndex)); + } + //! Reads a JSON-pointer style path from pathString and replaces this path's contents. //! Paths are accepted in the following forms: //! "/path/to/foo/0" diff --git a/Code/Framework/AzCore/Tests/DOM/DomPathBenchmarks.cpp b/Code/Framework/AzCore/Tests/DOM/DomPathBenchmarks.cpp index 624b24cd20..c3c8139f6c 100644 --- a/Code/Framework/AzCore/Tests/DOM/DomPathBenchmarks.cpp +++ b/Code/Framework/AzCore/Tests/DOM/DomPathBenchmarks.cpp @@ -96,4 +96,25 @@ namespace AZ::Dom::Benchmark state.SetItemsProcessed(3 * state.iterations()); } BENCHMARK_REGISTER_F(DomPathBenchmark, DomPathEntry_IsEndOfArray); + + BENCHMARK_DEFINE_F(DomPathBenchmark, DomPathEntry_Comparison)(benchmark::State& state) + { + PathEntry name("name"); + PathEntry index(0); + PathEntry endOfArray; + endOfArray.SetEndOfArray(); + + for (auto _ : state) + { + name == name; + name == index; + name == endOfArray; + index == index; + index == endOfArray; + endOfArray == endOfArray; + } + + state.SetItemsProcessed(6 * state.iterations()); + } + BENCHMARK_REGISTER_F(DomPathBenchmark, DomPathEntry_Comparison); } diff --git a/Code/Framework/AzCore/Tests/DOM/DomPathTests.cpp b/Code/Framework/AzCore/Tests/DOM/DomPathTests.cpp index 54bff9f29b..5f3aa946b2 100644 --- a/Code/Framework/AzCore/Tests/DOM/DomPathTests.cpp +++ b/Code/Framework/AzCore/Tests/DOM/DomPathTests.cpp @@ -174,4 +174,23 @@ namespace AZ::Dom::Tests p.AppendToString(s); EXPECT_EQ(s, "/foo/0/foo/0"); } + + TEST_F(DomPathTests, MixedPath_AppendToFixedString) + { + Path p("/foo/0"); + + { + AZStd::fixed_string<7> s; + p.AppendToString(s); + EXPECT_EQ(s, "/foo/0"); + } + + { + AZStd::fixed_string<9> s; + p.AppendToString(s); + EXPECT_EQ(s, "/foo/0"); + p.AppendToString(s); + EXPECT_EQ(s, "/foo/0/fo"); + } + } } // namespace AZ::Dom::Tests