From a6ddf4164f36c9687119dd2034d6fa8dc00b8e66 Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Mon, 7 Feb 2022 10:57:39 -0600 Subject: [PATCH] SurfaceTagWeights optimization (#7436) * Add comparison operators to SurfaceTagWeight. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Changed AddSurfaceTagWeight to always combine weights. This simplifies the API a bit and defines the behavior if someone ever tries to add a duplicate tag. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Added benchmarks for measuring the performance-critical APIs. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Changed SurfaceTagWeights to a fixed_vector. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> --- .../AzFramework/SurfaceData/SurfaceData.h | 13 +++ .../GradientSurfaceDataComponent.cpp | 2 +- .../Include/SurfaceData/SurfaceDataTypes.h | 77 ++++++++++++----- .../SurfaceDataColliderComponent.cpp | 2 +- .../Components/SurfaceDataShapeComponent.cpp | 2 +- .../Code/Source/SurfaceDataTypes.cpp | 62 +++++++------- .../Code/Tests/SurfaceDataBenchmarks.cpp | 83 +++++++++++++++++++ .../Code/Tests/SurfaceDataTest.cpp | 2 +- .../Code/Source/AreaSystemComponent.cpp | 2 +- 9 files changed, 191 insertions(+), 54 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/SurfaceData/SurfaceData.h b/Code/Framework/AzFramework/AzFramework/SurfaceData/SurfaceData.h index 77d9ae4239..9faa03f921 100644 --- a/Code/Framework/AzFramework/AzFramework/SurfaceData/SurfaceData.h +++ b/Code/Framework/AzFramework/AzFramework/SurfaceData/SurfaceData.h @@ -29,6 +29,19 @@ namespace AzFramework::SurfaceData { } + //! Equality comparison operator for SurfaceTagWeight. + bool operator==(const SurfaceTagWeight& rhs) const + { + return (m_surfaceType == rhs.m_surfaceType) && (m_weight == rhs.m_weight); + } + + //! Inequality comparison operator for SurfaceTagWeight. + bool operator!=(const SurfaceTagWeight& rhs) const + { + return !(*this == rhs); + } + + AZ::Crc32 m_surfaceType = AZ::Crc32(Constants::s_unassignedTagName); float m_weight = 0.0f; //! A Value in the range [0.0f .. 1.0f] diff --git a/Gems/GradientSignal/Code/Source/Components/GradientSurfaceDataComponent.cpp b/Gems/GradientSignal/Code/Source/Components/GradientSurfaceDataComponent.cpp index cd87e3044c..86bcbf2a2d 100644 --- a/Gems/GradientSignal/Code/Source/Components/GradientSurfaceDataComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Components/GradientSurfaceDataComponent.cpp @@ -251,7 +251,7 @@ namespace GradientSignal const float value = m_gradientSampler.GetValue(sampleParams); if (value >= m_configuration.m_thresholdMin && value <= m_configuration.m_thresholdMax) { - weights.AddSurfaceWeightsIfGreater(m_configuration.m_modifierTags, value); + weights.AddSurfaceTagWeights(m_configuration.m_modifierTags, value); } } }); diff --git a/Gems/SurfaceData/Code/Include/SurfaceData/SurfaceDataTypes.h b/Gems/SurfaceData/Code/Include/SurfaceData/SurfaceDataTypes.h index c892bc8c09..943c45bb6a 100644 --- a/Gems/SurfaceData/Code/Include/SurfaceData/SurfaceDataTypes.h +++ b/Gems/SurfaceData/Code/Include/SurfaceData/SurfaceDataTypes.h @@ -24,9 +24,17 @@ namespace SurfaceData using SurfaceTagVector = AZStd::vector; //! SurfaceTagWeights stores a collection of surface tags and weights. + //! A surface tag can only appear once in the collection. Attempting to add it multiple times will always preserve the + //! highest weight value. class SurfaceTagWeights { public: + //! The maximum number of surface weights that we can store. + //! For performance reasons, we want to limit this so that we can preallocate the max size in advance. + //! The current number is chosen to be higher than expected needs, but small enough to avoid being excessively wasteful. + //! (Dynamic structures would end up taking more memory than what we're preallocating) + static inline constexpr size_t MaxSurfaceWeights = 16; + SurfaceTagWeights() = default; //! Construct a collection of SurfaceTagWeights from the given SurfaceTagWeightList. @@ -45,42 +53,68 @@ namespace SurfaceData //! @param weight - The weight to assign to each tag. void AssignSurfaceTagWeights(const SurfaceTagVector& tags, float weight); - //! Add a surface tag weight to this collection. - //! @param tag - The surface tag. - //! @param weight - The surface tag weight. - void AddSurfaceTagWeight(const AZ::Crc32 tag, const float weight); - - //! Replace the surface tag weight with the new one if it's higher, or add it if the tag isn't found. + //! Add a surface tag weight to this collection. If the tag already exists, the higher weight will be preserved. //! (This method is intentionally inlined for its performance impact) //! @param tag - The surface tag. //! @param weight - The surface tag weight. - void AddSurfaceWeightIfGreater(const AZ::Crc32 tag, const float weight) + void AddSurfaceTagWeight(const AZ::Crc32 tag, const float weight) { - const auto maskItr = m_weights.find(tag); - const float previousValue = maskItr != m_weights.end() ? maskItr->second : 0.0f; - m_weights[tag] = AZ::GetMax(weight, previousValue); + for (auto weightItr = m_weights.begin(); weightItr != m_weights.end(); ++weightItr) + { + // Since we need to scan for duplicate surface types, store the entries sorted by surface type so that we can + // early-out once we pass the location for the entry instead of always searching every entry. + if (weightItr->m_surfaceType > tag) + { + if (m_weights.size() != MaxSurfaceWeights) + { + // We didn't find the surface type, so add the new entry in sorted order. + m_weights.insert(weightItr, { tag, weight }); + } + else + { + AZ_Assert(false, "SurfaceTagWeights has reached max capacity, it cannot add a new tag / weight."); + } + return; + } + else if (weightItr->m_surfaceType == tag) + { + // We found the surface type, so just keep the higher of the two weights. + weightItr->m_weight = AZ::GetMax(weight, weightItr->m_weight); + return; + } + } + + // We didn't find the surface weight, and the sort order for it is at the end, so add it to the back of the list. + if (m_weights.size() != MaxSurfaceWeights) + { + m_weights.emplace_back(tag, weight); + } + else + { + AZ_Assert(false, "SurfaceTagWeights has reached max capacity, it cannot add a new tag / weight."); + } } - //! Replace the surface tag weight with the new one if it's higher, or add it if the tag isn't found. + //! Add surface tags and weights to this collection. If a tag already exists, the higher weight will be preserved. //! (This method is intentionally inlined for its performance impact) //! @param tags - The surface tags to replace/add. //! @param weight - The surface tag weight to use for each tag. - void AddSurfaceWeightsIfGreater(const SurfaceTagVector& tags, const float weight) + void AddSurfaceTagWeights(const SurfaceTagVector& tags, const float weight) { for (const auto& tag : tags) { - AddSurfaceWeightIfGreater(tag, weight); + AddSurfaceTagWeight(tag, weight); } } - //! Replace the surface tag weight with the new one if it's higher, or add it if the tag isn't found. + //! Add surface tags and weights to this collection. If a tag already exists, the higher weight will be preserved. //! (This method is intentionally inlined for its performance impact) //! @param weights - The surface tags and weights to replace/add. - void AddSurfaceWeightsIfGreater(const SurfaceTagWeights& weights) + void AddSurfaceTagWeights(const SurfaceTagWeights& weights) { for (const auto& [tag, weight] : weights.m_weights) { - AddSurfaceWeightIfGreater(tag, weight); + AddSurfaceTagWeight(tag, weight); } } @@ -125,7 +159,7 @@ namespace SurfaceData //! Check to see if the collection contains the given tag. //! @param sampleTag - The tag to look for. //! @return True if the tag is found, false if it isn't. - bool HasMatchingTag(const AZ::Crc32& sampleTag) const; + bool HasMatchingTag(AZ::Crc32 sampleTag) const; //! Check to see if the collection contains the given tag with the given weight range. //! The range check is inclusive on both sides of the range: [weightMin, weightMax] @@ -133,7 +167,7 @@ namespace SurfaceData //! @param weightMin - The minimum weight for this tag. //! @param weightMax - The maximum weight for this tag. //! @return True if the tag is found, false if it isn't. - bool HasMatchingTag(const AZ::Crc32& sampleTag, float weightMin, float weightMax) const; + bool HasMatchingTag(AZ::Crc32 sampleTag, float weightMin, float weightMax) const; //! Check to see if the collection contains any of the given tags. //! @param sampleTags - The tags to look for. @@ -149,7 +183,12 @@ namespace SurfaceData bool HasAnyMatchingTags(const SurfaceTagVector& sampleTags, float weightMin, float weightMax) const; private: - AZStd::unordered_map m_weights; + //! Search for the given tag entry. + //! @param tag - The tag to search for. + //! @return The pointer to the tag that's found, or end() if it wasn't found. + const AzFramework::SurfaceData::SurfaceTagWeight* FindTag(AZ::Crc32 tag) const; + + AZStd::fixed_vector m_weights; }; //! SurfacePointList stores a collection of surface point data, which consists of positions, normals, and surface tag weights. diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp index e51fbc09b2..86a165b7d8 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp @@ -259,7 +259,7 @@ namespace SurfaceData if (DoRayTrace(position, queryPointOnly, hitPosition, hitNormal)) { // If the query point collides with the volume, add all our modifier tags with a weight of 1.0f. - weights.AddSurfaceWeightsIfGreater(m_configuration.m_modifierTags, 1.0f); + weights.AddSurfaceTagWeights(m_configuration.m_modifierTags, 1.0f); } } }); diff --git a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp index b84607782e..481372b7dc 100644 --- a/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp +++ b/Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp @@ -179,7 +179,7 @@ namespace SurfaceData if (m_shapeBounds.Contains(position) && shape->IsPointInside(position)) { // If the point is inside our shape, add all our modifier tags with a weight of 1.0f. - weights.AddSurfaceWeightsIfGreater(m_configuration.m_modifierTags, 1.0f); + weights.AddSurfaceTagWeights(m_configuration.m_modifierTags, 1.0f); } }); }); diff --git a/Gems/SurfaceData/Code/Source/SurfaceDataTypes.cpp b/Gems/SurfaceData/Code/Source/SurfaceDataTypes.cpp index 3071fd02ee..04c1ada677 100644 --- a/Gems/SurfaceData/Code/Source/SurfaceDataTypes.cpp +++ b/Gems/SurfaceData/Code/Source/SurfaceDataTypes.cpp @@ -14,28 +14,21 @@ namespace SurfaceData void SurfaceTagWeights::AssignSurfaceTagWeights(const AzFramework::SurfaceData::SurfaceTagWeightList& weights) { m_weights.clear(); - m_weights.reserve(weights.size()); for (auto& weight : weights) { - m_weights.emplace(weight.m_surfaceType, weight.m_weight); + AddSurfaceTagWeight(weight.m_surfaceType, weight.m_weight); } } void SurfaceTagWeights::AssignSurfaceTagWeights(const SurfaceTagVector& tags, float weight) { m_weights.clear(); - m_weights.reserve(tags.size()); for (auto& tag : tags) { - m_weights[tag] = weight; + AddSurfaceTagWeight(tag.operator AZ::Crc32(), weight); } } - void SurfaceTagWeights::AddSurfaceTagWeight(const AZ::Crc32 tag, const float value) - { - m_weights[tag] = value; - } - void SurfaceTagWeights::Clear() { m_weights.clear(); @@ -52,7 +45,7 @@ namespace SurfaceData weights.reserve(m_weights.size()); for (auto& weight : m_weights) { - weights.emplace_back(weight.first, weight.second); + weights.emplace_back(weight); } return weights; } @@ -65,17 +58,8 @@ namespace SurfaceData return false; } - for (auto& weight : m_weights) - { - auto rhsWeight = rhs.m_weights.find(weight.first); - if ((rhsWeight == rhs.m_weights.end()) || (rhsWeight->second != weight.second)) - { - return false; - } - } - - // All the entries matched, and the lists are the same size, so they're equal. - return true; + // The lists are stored in sorted order, so we can compare every entry in order for equivalence. + return (m_weights == rhs.m_weights); } bool SurfaceTagWeights::SurfaceWeightsAreEqual(const AzFramework::SurfaceData::SurfaceTagWeightList& compareWeights) const @@ -92,7 +76,7 @@ namespace SurfaceData compareWeights.begin(), compareWeights.end(), [weight](const AzFramework::SurfaceData::SurfaceTagWeight& compareWeight) -> bool { - return (weight.first == compareWeight.m_surfaceType) && (weight.second == compareWeight.m_weight); + return (weight == compareWeight); }); // If we didn't find a match, they're not equal. @@ -119,9 +103,9 @@ namespace SurfaceData bool SurfaceTagWeights::HasValidTags() const { - for (const auto& sourceTag : m_weights) + for (const auto& weight : m_weights) { - if (sourceTag.first != Constants::s_unassignedTagCrc) + if (weight.m_surfaceType != Constants::s_unassignedTagCrc) { return true; } @@ -129,9 +113,9 @@ namespace SurfaceData return false; } - bool SurfaceTagWeights::HasMatchingTag(const AZ::Crc32& sampleTag) const + bool SurfaceTagWeights::HasMatchingTag(AZ::Crc32 sampleTag) const { - return m_weights.find(sampleTag) != m_weights.end(); + return FindTag(sampleTag) != m_weights.end(); } bool SurfaceTagWeights::HasAnyMatchingTags(const SurfaceTagVector& sampleTags) const @@ -147,10 +131,10 @@ namespace SurfaceData return false; } - bool SurfaceTagWeights::HasMatchingTag(const AZ::Crc32& sampleTag, float weightMin, float weightMax) const + bool SurfaceTagWeights::HasMatchingTag(AZ::Crc32 sampleTag, float weightMin, float weightMax) const { - auto maskItr = m_weights.find(sampleTag); - return maskItr != m_weights.end() && weightMin <= maskItr->second && weightMax >= maskItr->second; + auto weightEntry = FindTag(sampleTag); + return weightEntry != m_weights.end() && weightMin <= weightEntry->m_weight && weightMax >= weightEntry->m_weight; } bool SurfaceTagWeights::HasAnyMatchingTags(const SurfaceTagVector& sampleTags, float weightMin, float weightMax) const @@ -166,7 +150,25 @@ namespace SurfaceData return false; } + const AzFramework::SurfaceData::SurfaceTagWeight* SurfaceTagWeights::FindTag(AZ::Crc32 tag) const + { + for (auto weightItr = m_weights.begin(); weightItr != m_weights.end(); ++weightItr) + { + if (weightItr->m_surfaceType == tag) + { + // Found the tag, return a pointer to the entry. + return weightItr; + } + else if (weightItr->m_surfaceType > tag) + { + // Our list is stored in sorted order by surfaceType, so early-out if our values get too high. + break; + } + } + // The tag wasn't found, so return end(). + return m_weights.end(); + } SurfacePointList::SurfacePointList(AZStd::initializer_list surfacePoints) @@ -192,7 +194,7 @@ namespace SurfaceData if (m_surfacePositionList[index].IsClose(position) && m_surfaceNormalList[index].IsClose(normal)) { // consolidate points with similar attributes by adding masks/weights to the similar point instead of adding a new one. - m_surfaceWeightsList[index].AddSurfaceWeightsIfGreater(masks); + m_surfaceWeightsList[index].AddSurfaceTagWeights(masks); return; } else if (m_surfacePositionList[index].GetZ() < position.GetZ()) diff --git a/Gems/SurfaceData/Code/Tests/SurfaceDataBenchmarks.cpp b/Gems/SurfaceData/Code/Tests/SurfaceDataBenchmarks.cpp index 226a711cff..f8b4c675a5 100644 --- a/Gems/SurfaceData/Code/Tests/SurfaceDataBenchmarks.cpp +++ b/Gems/SurfaceData/Code/Tests/SurfaceDataBenchmarks.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -270,6 +271,88 @@ namespace UnitTest ->Arg( 2048 ) ->Unit(::benchmark::kMillisecond); + BENCHMARK_DEFINE_F(SurfaceDataBenchmark, BM_AddSurfaceTagWeight)(benchmark::State& state) + { + AZ_PROFILE_FUNCTION(Entity); + + AZ::Crc32 tags[SurfaceData::SurfaceTagWeights::MaxSurfaceWeights]; + AZ::SimpleLcgRandom randomGenerator(1234567); + + // Declare this outside the loop so that we aren't benchmarking creation and destruction. + SurfaceData::SurfaceTagWeights weights; + + bool clearEachTime = state.range(0) > 0; + + // Create a list of randomly-generated tag values. + for (auto& tag : tags) + { + tag = randomGenerator.GetRandom(); + } + + for (auto _ : state) + { + // We'll benchmark this two ways: + // 1. We clear each time, which means each AddSurfaceWeightIfGreater call will search the whole list then add. + // 2. We don't clear, which means that after the first run, AddSurfaceWeightIfGreater will always try to replace values. + if (clearEachTime) + { + weights.Clear(); + } + + // For each tag, try to add it with a random weight. + for (auto& tag : tags) + { + weights.AddSurfaceTagWeight(tag, randomGenerator.GetRandomFloat()); + } + } + } + + BENCHMARK_REGISTER_F(SurfaceDataBenchmark, BM_AddSurfaceTagWeight) + ->Arg(false) + ->Arg(true) + ->ArgName("ClearEachTime"); + + BENCHMARK_DEFINE_F(SurfaceDataBenchmark, BM_HasAnyMatchingTags_NoMatches)(benchmark::State& state) + { + AZ_PROFILE_FUNCTION(Entity); + + AZ::Crc32 tags[SurfaceData::SurfaceTagWeights::MaxSurfaceWeights]; + AZ::SimpleLcgRandom randomGenerator(1234567); + + // Declare this outside the loop so that we aren't benchmarking creation and destruction. + SurfaceData::SurfaceTagWeights weights; + + // Create a list of randomly-generated tag values. + for (auto& tag : tags) + { + // Specifically always set the last bit so that we can create comparison tags that won't match. + tag = randomGenerator.GetRandom() | 0x01; + + // Add the tag to our weights list with a random weight. + weights.AddSurfaceTagWeight(tag, randomGenerator.GetRandomFloat()); + } + + // Create a set of similar comparison tags that won't match. We still want a random distribution of values though, + // because the SurfaceTagWeights might behave differently with ordered lists. + SurfaceData::SurfaceTagVector comparisonTags; + for (auto& tag : tags) + { + comparisonTags.emplace_back(tag ^ 0x01); + } + + for (auto _ : state) + { + // Test to see if any of our tags match. + // All of comparison tags should get compared against all of the added tags. + bool result = weights.HasAnyMatchingTags(comparisonTags); + benchmark::DoNotOptimize(result); + } + } + + BENCHMARK_REGISTER_F(SurfaceDataBenchmark, BM_HasAnyMatchingTags_NoMatches); + + + #endif } diff --git a/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp b/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp index 39659313f5..f4ff0cb04c 100644 --- a/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp +++ b/Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp @@ -169,7 +169,7 @@ class MockSurfaceProvider if (surfacePoints != m_GetSurfacePoints.end()) { - weights.AddSurfaceWeightsIfGreater(m_tags, 1.0f); + weights.AddSurfaceTagWeights(m_tags, 1.0f); } }); } diff --git a/Gems/Vegetation/Code/Source/AreaSystemComponent.cpp b/Gems/Vegetation/Code/Source/AreaSystemComponent.cpp index e575480ff4..f4ef3bd6f1 100644 --- a/Gems/Vegetation/Code/Source/AreaSystemComponent.cpp +++ b/Gems/Vegetation/Code/Source/AreaSystemComponent.cpp @@ -1137,7 +1137,7 @@ namespace Vegetation claimPoint.m_position = position; claimPoint.m_normal = normal; claimPoint.m_masks = masks; - sectorInfo.m_baseContext.m_masks.AddSurfaceWeightsIfGreater(masks); + sectorInfo.m_baseContext.m_masks.AddSurfaceTagWeights(masks); return true; }); }