From c719031763362991b205d29640d22bfdafba68fe Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 29 Dec 2022 23:35:05 -0800 Subject: [PATCH 1/7] Update Aggregator store use a KeyValuePair array --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 66 ++++++++------- src/OpenTelemetry/Metrics/MetricPoint.cs | 6 +- src/OpenTelemetry/Metrics/Tags.cs | 47 ++++------- .../Metrics/ThreadStaticStorage.cs | 81 ++++++------------- src/OpenTelemetry/ReadOnlyTagCollection.cs | 18 ++--- 5 files changed, 85 insertions(+), 133 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index a53780470c5..c05f21bbca4 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -23,6 +23,7 @@ namespace OpenTelemetry.Metrics internal sealed class AggregatorStore { private static readonly string MetricPointCapHitFixMessage = "Modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; + private static readonly Comparison> KeyValueComparisonDelegate = KeyValueComparison; private readonly object lockZeroTags = new(); private readonly HashSet tagKeysInteresting; private readonly int tagsKeysInterestingCount; @@ -152,6 +153,11 @@ internal void SnapshotCumulative(int indexSnapshot) internal MetricPointsAccessor GetMetricPoints() => new(this.metricPoints, this.currentMetricPointBatch, this.batchSize); + private static int KeyValueComparison(KeyValuePair x, KeyValuePair y) + { + return x.Key.CompareTo(y.Key); + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] private void InitializeZeroTagPointIfNotInitialized() { @@ -161,7 +167,7 @@ private void InitializeZeroTagPointIfNotInitialized() { if (!this.zeroTagMetricPointInitialized) { - this.metricPoints[0] = new MetricPoint(this, this.aggType, null, null, this.histogramBounds); + this.metricPoints[0] = new MetricPoint(this, this.aggType, null, this.histogramBounds); this.zeroTagMetricPointInitialized = true; } } @@ -169,9 +175,9 @@ private void InitializeZeroTagPointIfNotInitialized() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int length) + private int LookupAggregatorStore(KeyValuePair[] tagKeysAndValues, int length) { - var givenTags = new Tags(tagKeys, tagValues); + var givenTags = new Tags(tagKeysAndValues); if (!this.tagsToMetricPointIndexDictionary.TryGetValue(givenTags, out var aggregatorIndex)) { @@ -180,11 +186,11 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. // Create or obtain new arrays to temporarily hold the sorted tag Keys and Values var storage = ThreadStaticStorage.GetStorage(); - storage.CloneKeysAndValues(tagKeys, tagValues, length, out var tempSortedTagKeys, out var tempSortedTagValues); + storage.CloneKeysAndValues(tagKeysAndValues, length, out var tempSortedTagKeysAndValues); - Array.Sort(tempSortedTagKeys, tempSortedTagValues); + Array.Sort(tempSortedTagKeysAndValues, KeyValueComparisonDelegate); - var sortedTags = new Tags(tempSortedTagKeys, tempSortedTagValues); + var sortedTags = new Tags(tempSortedTagKeysAndValues); if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) { @@ -202,20 +208,14 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng // so we need to make a deep copy for Dictionary storage. if (length <= ThreadStaticStorage.MaxTagCacheSize) { - var givenKeys = new string[length]; - tagKeys.CopyTo(givenKeys, 0); + var givenTagKeysAndValues = new KeyValuePair[length]; + tagKeysAndValues.CopyTo(givenTagKeysAndValues.AsSpan()); - var givenValues = new object[length]; - tagValues.CopyTo(givenValues, 0); + var sortedTagKeysAndValues = new KeyValuePair[length]; + tempSortedTagKeysAndValues.CopyTo(sortedTagKeysAndValues.AsSpan()); - var sortedTagKeys = new string[length]; - tempSortedTagKeys.CopyTo(sortedTagKeys, 0); - - var sortedTagValues = new object[length]; - tempSortedTagValues.CopyTo(sortedTagValues, 0); - - givenTags = new Tags(givenKeys, givenValues); - sortedTags = new Tags(sortedTagKeys, sortedTagValues); + givenTags = new Tags(givenTagKeysAndValues); + sortedTags = new Tags(sortedTagKeysAndValues); } lock (this.tagsToMetricPointIndexDictionary) @@ -234,7 +234,7 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng } ref var metricPoint = ref this.metricPoints[aggregatorIndex]; - metricPoint = new MetricPoint(this, this.aggType, sortedTags.Keys, sortedTags.Values, this.histogramBounds); + metricPoint = new MetricPoint(this, this.aggType, sortedTags.KeyValuePairs, this.histogramBounds); // Add to dictionary *after* initializing MetricPoint // as other threads can start writing to the @@ -261,13 +261,11 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng } // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. - var givenKeys = new string[length]; - var givenValues = new object[length]; + var givenTagKeysAndValues = new KeyValuePair[length]; - tagKeys.CopyTo(givenKeys, 0); - tagValues.CopyTo(givenValues, 0); + tagKeysAndValues.CopyTo(givenTagKeysAndValues.AsSpan()); - givenTags = new Tags(givenKeys, givenValues); + givenTags = new Tags(givenTagKeysAndValues); lock (this.tagsToMetricPointIndexDictionary) { @@ -285,7 +283,7 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng } ref var metricPoint = ref this.metricPoints[aggregatorIndex]; - metricPoint = new MetricPoint(this, this.aggType, givenTags.Keys, givenTags.Values, this.histogramBounds); + metricPoint = new MetricPoint(this, this.aggType, givenTags.KeyValuePairs, this.histogramBounds); // Add to dictionary *after* initializing MetricPoint // as other threads can start writing to the @@ -404,9 +402,9 @@ private int FindMetricAggregatorsDefault(ReadOnlySpan> tags) @@ -423,7 +421,7 @@ private int FindMetricAggregatorsCustomTag(ReadOnlySpan> + { + public static KeyValuePairComparer Instance = new KeyValuePairComparer(); + + public int Compare(KeyValuePair x, KeyValuePair y) + { + return x.Key.CompareTo(y.Key); + } } } } diff --git a/src/OpenTelemetry/Metrics/MetricPoint.cs b/src/OpenTelemetry/Metrics/MetricPoint.cs index 0795b682e97..1cbe5580971 100644 --- a/src/OpenTelemetry/Metrics/MetricPoint.cs +++ b/src/OpenTelemetry/Metrics/MetricPoint.cs @@ -41,16 +41,14 @@ public struct MetricPoint internal MetricPoint( AggregatorStore aggregatorStore, AggregationType aggType, - string[] keys, - object[] values, + KeyValuePair[] tagKeysAndValues, double[] histogramExplicitBounds) { Debug.Assert(aggregatorStore != null, "AggregatorStore was null."); - Debug.Assert((keys?.Length ?? 0) == (values?.Length ?? 0), "Key and value array lengths did not match."); Debug.Assert(histogramExplicitBounds != null, "Histogram explicit Bounds was null."); this.aggType = aggType; - this.Tags = new ReadOnlyTagCollection(keys, values); + this.Tags = new ReadOnlyTagCollection(tagKeysAndValues); this.runningValue = default; this.snapshotValue = default; this.deltaLastValue = default; diff --git a/src/OpenTelemetry/Metrics/Tags.cs b/src/OpenTelemetry/Metrics/Tags.cs index ba754fd7954..7e6c3c5416c 100644 --- a/src/OpenTelemetry/Metrics/Tags.cs +++ b/src/OpenTelemetry/Metrics/Tags.cs @@ -20,26 +20,23 @@ namespace OpenTelemetry.Metrics { private readonly int hashCode; - public Tags(string[] keys, object[] values) + public Tags(KeyValuePair[] keyValuePairs) { - this.Keys = keys; - this.Values = values; + this.KeyValuePairs = keyValuePairs; - unchecked + var hash = 17; + for (int i = 0; i < this.KeyValuePairs.Length; i++) { - var hash = 17; - for (int i = 0; i < this.Keys.Length; i++) + unchecked { - hash = (hash * 31) + this.Keys[i].GetHashCode() + this.Values[i]?.GetHashCode() ?? 0; + hash = (hash * 31) + this.KeyValuePairs[i].Key.GetHashCode() + this.KeyValuePairs[i].Value?.GetHashCode() ?? 0; } - - this.hashCode = hash; } - } - public readonly string[] Keys { get; } + this.hashCode = hash; + } - public readonly object[] Values { get; } + public readonly KeyValuePair[] KeyValuePairs { get; } public static bool operator ==(Tags tag1, Tags tag2) => tag1.Equals(tag2); @@ -52,35 +49,23 @@ public override readonly bool Equals(object obj) public readonly bool Equals(Tags other) { - // Equality check for Keys - // Check if the two string[] are equal - var keysLength = this.Keys.Length; + var length = this.KeyValuePairs.Length; - if (keysLength != other.Keys.Length) + if (length != other.KeyValuePairs.Length) { return false; } - for (int i = 0; i < keysLength; i++) + for (int i = 0; i < length; i++) { - if (!this.Keys[i].Equals(other.Keys[i], StringComparison.Ordinal)) + // Equality check for Keys + if (!this.KeyValuePairs[i].Key.Equals(other.KeyValuePairs[i].Key, StringComparison.Ordinal)) { return false; } - } - - // Equality check for Values - // Check if the two object[] are equal - var valuesLength = this.Values.Length; - if (valuesLength != other.Values.Length) - { - return false; - } - - for (int i = 0; i < valuesLength; i++) - { - if (!this.Values[i].Equals(other.Values[i])) + // Equality check for Values + if (!this.KeyValuePairs[i].Value?.Equals(other.KeyValuePairs[i].Value) ?? other.KeyValuePairs[i].Value != null) { return false; } diff --git a/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs b/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs index 023ba174d70..1811cd88999 100644 --- a/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs +++ b/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs @@ -50,58 +50,47 @@ internal static ThreadStaticStorage GetStorage() } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void SplitToKeysAndValues(ReadOnlySpan> tags, int tagLength, out string[] tagKeys, out object[] tagValues) + internal void SplitToKeysAndValues(ReadOnlySpan> tags, int tagLength, out KeyValuePair[] tagKeysAndValues) { Guard.ThrowIfZero(tagLength, $"There must be at least one tag to use {nameof(ThreadStaticStorage)}"); if (tagLength <= MaxTagCacheSize) { - tagKeys = this.primaryTagStorage[tagLength - 1].TagKeys; - tagValues = this.primaryTagStorage[tagLength - 1].TagValues; + tagKeysAndValues = this.primaryTagStorage[tagLength - 1].TagKeysAndValues; } else { - tagKeys = new string[tagLength]; - tagValues = new object[tagLength]; + tagKeysAndValues = new KeyValuePair[tagLength]; } - for (var n = 0; n < tagLength; n++) - { - tagKeys[n] = tags[n].Key; - tagValues[n] = tags[n].Value; - } + tags.CopyTo(tagKeysAndValues); } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void SplitToKeysAndValues(ReadOnlySpan> tags, int tagLength, HashSet tagKeysInteresting, out string[] tagKeys, out object[] tagValues, out int actualLength) + internal void SplitToKeysAndValues(ReadOnlySpan> tags, int tagLength, HashSet tagKeysInteresting, out KeyValuePair[] tagKeysAndValues, out int actualLength) { // We do not know ahead the actual length, so start with max possible length. var maxLength = Math.Min(tagKeysInteresting.Count, tagLength); if (maxLength == 0) { - tagKeys = null; - tagValues = null; + tagKeysAndValues = null; } else if (maxLength <= MaxTagCacheSize) { - tagKeys = this.primaryTagStorage[maxLength - 1].TagKeys; - tagValues = this.primaryTagStorage[maxLength - 1].TagValues; + tagKeysAndValues = this.primaryTagStorage[maxLength - 1].TagKeysAndValues; } else { - tagKeys = new string[maxLength]; - tagValues = new object[maxLength]; + tagKeysAndValues = new KeyValuePair[maxLength]; } actualLength = 0; for (var n = 0; n < tagLength; n++) { // Copy only interesting tags, and keep count. - var tag = tags[n]; - if (tagKeysInteresting.Contains(tag.Key)) + if (tagKeysInteresting.Contains(tags[n].Key)) { - tagKeys[actualLength] = tag.Key; - tagValues[actualLength] = tag.Value; + tagKeysAndValues[actualLength] = tags[n]; actualLength++; } } @@ -118,73 +107,53 @@ internal void SplitToKeysAndValues(ReadOnlySpan> ta { if (actualLength == 0) { - tagKeys = null; - tagValues = null; + tagKeysAndValues = null; return; } else if (actualLength <= MaxTagCacheSize) { - var tmpKeys = this.primaryTagStorage[actualLength - 1].TagKeys; - var tmpValues = this.primaryTagStorage[actualLength - 1].TagValues; - for (var n = 0; n < actualLength; n++) - { - tmpKeys[n] = tagKeys[n]; - tmpValues[n] = tagValues[n]; - } - - tagKeys = tmpKeys; - tagValues = tmpValues; + var tmpTagKeysAndValues = this.primaryTagStorage[actualLength - 1].TagKeysAndValues; + + Array.Copy(tagKeysAndValues, 0, tmpTagKeysAndValues, 0, actualLength); + + tagKeysAndValues = tmpTagKeysAndValues; } else { - var tmpKeys = new string[actualLength]; - var tmpValues = new object[actualLength]; + var tmpTagKeyAndValues = new KeyValuePair[actualLength]; - for (var n = 0; n < actualLength; n++) - { - tmpKeys[n] = tagKeys[n]; - tmpValues[n] = tagValues[n]; - } + Array.Copy(tagKeysAndValues, 0, tmpTagKeyAndValues, 0, actualLength); - tagKeys = tmpKeys; - tagValues = tmpValues; + tagKeysAndValues = tmpTagKeyAndValues; } } } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void CloneKeysAndValues(string[] inputTagKeys, object[] inputTagValues, int tagLength, out string[] clonedTagKeys, out object[] clonedTagValues) + internal void CloneKeysAndValues(KeyValuePair[] inputTagKeysAndValues, int tagLength, out KeyValuePair[] clonedTagKeysAndValues) { Guard.ThrowIfZero(tagLength, $"There must be at least one tag to use {nameof(ThreadStaticStorage)}", $"{nameof(tagLength)}"); if (tagLength <= MaxTagCacheSize) { - clonedTagKeys = this.secondaryTagStorage[tagLength - 1].TagKeys; - clonedTagValues = this.secondaryTagStorage[tagLength - 1].TagValues; + clonedTagKeysAndValues = this.secondaryTagStorage[tagLength - 1].TagKeysAndValues; } else { - clonedTagKeys = new string[tagLength]; - clonedTagValues = new object[tagLength]; + clonedTagKeysAndValues = new KeyValuePair[tagLength]; } - for (int i = 0; i < tagLength; i++) - { - clonedTagKeys[i] = inputTagKeys[i]; - clonedTagValues[i] = inputTagValues[i]; - } + inputTagKeysAndValues.CopyTo(clonedTagKeysAndValues.AsSpan()); } internal sealed class TagStorage { // Used to split into Key sequence, Value sequence. - internal readonly string[] TagKeys; - internal readonly object[] TagValues; + internal readonly KeyValuePair[] TagKeysAndValues; internal TagStorage(int n) { - this.TagKeys = new string[n]; - this.TagValues = new object[n]; + this.TagKeysAndValues = new KeyValuePair[n]; } } } diff --git a/src/OpenTelemetry/ReadOnlyTagCollection.cs b/src/OpenTelemetry/ReadOnlyTagCollection.cs index d1c63c5bee7..ab451626e1d 100644 --- a/src/OpenTelemetry/ReadOnlyTagCollection.cs +++ b/src/OpenTelemetry/ReadOnlyTagCollection.cs @@ -16,8 +16,6 @@ #nullable enable -using System.Diagnostics; - namespace OpenTelemetry { /// @@ -27,21 +25,17 @@ namespace OpenTelemetry // prevent accidental boxing. public readonly struct ReadOnlyTagCollection { - private readonly string[] keys; - private readonly object[] values; + private readonly KeyValuePair[] keyAndValues; - internal ReadOnlyTagCollection(string[]? keys, object[]? values) + internal ReadOnlyTagCollection(KeyValuePair[]? keyAndValues) { - this.keys = keys ?? Array.Empty(); - this.values = values ?? Array.Empty(); - - Debug.Assert(this.keys.Length == this.values.Length, "Array length mismatch"); + this.keyAndValues = keyAndValues ?? Array.Empty>(); } /// /// Gets the number of tags in the collection. /// - public int Count => this.keys.Length; + public int Count => this.keyAndValues.Length; /// /// Returns an enumerator that iterates through the tags. @@ -84,9 +78,7 @@ public bool MoveNext() if (index < this.source.Count) { - this.Current = new KeyValuePair( - this.source.keys[index], - this.source.values[index]); + this.Current = this.source.keyAndValues[index]; this.index++; return true; From 8a3abbc94e8bbe971f8b7689fef5a9e94f6b555e Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 5 Jan 2023 16:03:40 -0800 Subject: [PATCH 2/7] Update Benchmarks --- .../Benchmarks/Metrics/HistogramBenchmarks.cs | 58 +++++++++---------- test/Benchmarks/Metrics/MetricsBenchmarks.cs | 33 +++++------ .../Metrics/AggregatorTest.cs | 8 +-- 3 files changed, 49 insertions(+), 50 deletions(-) diff --git a/test/Benchmarks/Metrics/HistogramBenchmarks.cs b/test/Benchmarks/Metrics/HistogramBenchmarks.cs index 4443f9f7726..cd63c3b3a55 100644 --- a/test/Benchmarks/Metrics/HistogramBenchmarks.cs +++ b/test/Benchmarks/Metrics/HistogramBenchmarks.cs @@ -22,35 +22,35 @@ using OpenTelemetry.Tests; /* -BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.1706 (21H2) -AMD Ryzen 9 3900X, 1 CPU, 24 logical and 12 physical cores -.NET SDK=6.0.203 - [Host] : .NET 6.0.5 (6.0.522.21309), X64 RyuJIT - DefaultJob : .NET 6.0.5 (6.0.522.21309), X64 RyuJIT - - -| Method | BoundCount | Mean | Error | StdDev | -|---------------------------- |----------- |----------:|----------:|----------:| -| HistogramHotPath | 10 | 45.19 ns | 0.321 ns | 0.285 ns | -| HistogramWith1LabelHotPath | 10 | 97.21 ns | 0.129 ns | 0.114 ns | -| HistogramWith3LabelsHotPath | 10 | 179.77 ns | 0.270 ns | 0.239 ns | -| HistogramWith5LabelsHotPath | 10 | 263.30 ns | 2.423 ns | 2.267 ns | -| HistogramWith7LabelsHotPath | 10 | 338.42 ns | 3.121 ns | 2.919 ns | -| HistogramHotPath | 49 | 56.18 ns | 0.593 ns | 0.554 ns | -| HistogramWith1LabelHotPath | 49 | 110.60 ns | 0.815 ns | 0.762 ns | -| HistogramWith3LabelsHotPath | 49 | 193.30 ns | 1.048 ns | 0.980 ns | -| HistogramWith5LabelsHotPath | 49 | 281.55 ns | 1.638 ns | 1.532 ns | -| HistogramWith7LabelsHotPath | 49 | 343.88 ns | 2.148 ns | 2.010 ns | -| HistogramHotPath | 50 | 57.46 ns | 0.264 ns | 0.234 ns | -| HistogramWith1LabelHotPath | 50 | 121.73 ns | 0.372 ns | 0.348 ns | -| HistogramWith3LabelsHotPath | 50 | 227.95 ns | 1.074 ns | 1.004 ns | -| HistogramWith5LabelsHotPath | 50 | 313.15 ns | 1.068 ns | 0.999 ns | -| HistogramWith7LabelsHotPath | 50 | 377.04 ns | 1.191 ns | 0.930 ns | -| HistogramHotPath | 1000 | 78.33 ns | 0.441 ns | 0.391 ns | -| HistogramWith1LabelHotPath | 1000 | 127.57 ns | 0.457 ns | 0.428 ns | -| HistogramWith3LabelsHotPath | 1000 | 494.19 ns | 4.490 ns | 3.980 ns | -| HistogramWith5LabelsHotPath | 1000 | 608.75 ns | 11.306 ns | 10.576 ns | -| HistogramWith7LabelsHotPath | 1000 | 649.16 ns | 3.273 ns | 2.555 ns | +BenchmarkDotNet=v0.13.3, OS=Windows 11 (10.0.22621.963) +Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores +.NET SDK=7.0.101 + [Host] : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2 + DefaultJob : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2 + + +| Method | BoundCount | Mean | Error | StdDev | Allocated | +|---------------------------- |----------- |----------:|----------:|----------:|----------:| +| HistogramHotPath | 10 | 47.80 ns | 0.111 ns | 0.098 ns | - | +| HistogramWith1LabelHotPath | 10 | 99.18 ns | 0.448 ns | 0.419 ns | - | +| HistogramWith3LabelsHotPath | 10 | 189.60 ns | 0.872 ns | 0.815 ns | - | +| HistogramWith5LabelsHotPath | 10 | 263.10 ns | 2.813 ns | 2.494 ns | - | +| HistogramWith7LabelsHotPath | 10 | 309.09 ns | 1.603 ns | 1.421 ns | - | +| HistogramHotPath | 49 | 61.80 ns | 0.282 ns | 0.235 ns | - | +| HistogramWith1LabelHotPath | 49 | 111.22 ns | 0.347 ns | 0.290 ns | - | +| HistogramWith3LabelsHotPath | 49 | 203.53 ns | 2.263 ns | 2.006 ns | - | +| HistogramWith5LabelsHotPath | 49 | 278.45 ns | 2.401 ns | 2.005 ns | - | +| HistogramWith7LabelsHotPath | 49 | 331.96 ns | 4.160 ns | 3.892 ns | - | +| HistogramHotPath | 50 | 62.30 ns | 0.385 ns | 0.342 ns | - | +| HistogramWith1LabelHotPath | 50 | 108.47 ns | 0.132 ns | 0.111 ns | - | +| HistogramWith3LabelsHotPath | 50 | 237.33 ns | 3.291 ns | 3.079 ns | - | +| HistogramWith5LabelsHotPath | 50 | 316.26 ns | 1.989 ns | 1.763 ns | - | +| HistogramWith7LabelsHotPath | 50 | 359.67 ns | 2.359 ns | 2.091 ns | - | +| HistogramHotPath | 1000 | 82.72 ns | 0.366 ns | 0.286 ns | - | +| HistogramWith1LabelHotPath | 1000 | 134.84 ns | 1.502 ns | 1.331 ns | - | +| HistogramWith3LabelsHotPath | 1000 | 555.89 ns | 3.501 ns | 2.923 ns | - | +| HistogramWith5LabelsHotPath | 1000 | 645.98 ns | 11.965 ns | 9.991 ns | - | +| HistogramWith7LabelsHotPath | 1000 | 700.72 ns | 13.467 ns | 12.597 ns | - | */ namespace Benchmarks.Metrics diff --git a/test/Benchmarks/Metrics/MetricsBenchmarks.cs b/test/Benchmarks/Metrics/MetricsBenchmarks.cs index 50ec54d66c5..743071347dd 100644 --- a/test/Benchmarks/Metrics/MetricsBenchmarks.cs +++ b/test/Benchmarks/Metrics/MetricsBenchmarks.cs @@ -24,28 +24,27 @@ /* // * Summary * -BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000 +BenchmarkDotNet=v0.13.3, OS=Windows 11 (10.0.22621.963) Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores -.NET SDK=6.0.200 - [Host] : .NET 6.0.2 (6.0.222.6406), X64 RyuJIT - DefaultJob : .NET 6.0.2 (6.0.222.6406), X64 RyuJIT +.NET SDK=7.0.101 + [Host] : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2 + DefaultJob : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2 | Method | AggregationTemporality | Mean | Error | StdDev | Allocated | |-------------------------- |----------------------- |----------:|---------:|---------:|----------:| -| CounterHotPath | Cumulative | 16.60 ns | 0.120 ns | 0.094 ns | - | -| CounterWith1LabelsHotPath | Cumulative | 56.42 ns | 0.413 ns | 0.367 ns | - | -| CounterWith3LabelsHotPath | Cumulative | 138.44 ns | 1.153 ns | 1.079 ns | - | -| CounterWith5LabelsHotPath | Cumulative | 229.78 ns | 3.422 ns | 3.201 ns | - | -| CounterWith6LabelsHotPath | Cumulative | 251.65 ns | 0.954 ns | 0.892 ns | - | -| CounterWith7LabelsHotPath | Cumulative | 282.55 ns | 2.009 ns | 1.781 ns | - | -| CounterHotPath | Delta | 16.48 ns | 0.116 ns | 0.108 ns | - | -| CounterWith1LabelsHotPath | Delta | 57.38 ns | 0.322 ns | 0.285 ns | - | -| CounterWith3LabelsHotPath | Delta | 140.44 ns | 1.155 ns | 0.964 ns | - | -| CounterWith5LabelsHotPath | Delta | 224.01 ns | 2.034 ns | 1.699 ns | - | -| CounterWith6LabelsHotPath | Delta | 249.92 ns | 1.548 ns | 1.372 ns | - | -| CounterWith7LabelsHotPath | Delta | 281.87 ns | 1.979 ns | 1.852 ns | - | - +| CounterHotPath | Cumulative | 14.45 ns | 0.082 ns | 0.068 ns | - | +| CounterWith1LabelsHotPath | Cumulative | 60.54 ns | 0.473 ns | 0.442 ns | - | +| CounterWith3LabelsHotPath | Cumulative | 134.96 ns | 1.543 ns | 1.368 ns | - | +| CounterWith5LabelsHotPath | Cumulative | 206.31 ns | 1.219 ns | 1.081 ns | - | +| CounterWith6LabelsHotPath | Cumulative | 230.43 ns | 0.898 ns | 0.840 ns | - | +| CounterWith7LabelsHotPath | Cumulative | 254.62 ns | 1.156 ns | 1.082 ns | - | +| CounterHotPath | Delta | 13.79 ns | 0.021 ns | 0.019 ns | - | +| CounterWith1LabelsHotPath | Delta | 58.06 ns | 0.227 ns | 0.212 ns | - | +| CounterWith3LabelsHotPath | Delta | 131.89 ns | 0.304 ns | 0.285 ns | - | +| CounterWith5LabelsHotPath | Delta | 207.92 ns | 1.050 ns | 0.982 ns | - | +| CounterWith6LabelsHotPath | Delta | 232.65 ns | 1.011 ns | 0.896 ns | - | +| CounterWith7LabelsHotPath | Delta | 266.57 ns | 5.200 ns | 5.988 ns | - | */ namespace Benchmarks.Metrics diff --git a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs index c3efc722c10..e05cbcda414 100644 --- a/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs +++ b/test/OpenTelemetry.Tests/Metrics/AggregatorTest.cs @@ -25,7 +25,7 @@ public class AggregatorTest [Fact] public void HistogramDistributeToAllBucketsDefault() { - var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, null, Metric.DefaultHistogramBounds); + var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, Metric.DefaultHistogramBounds); histogramPoint.Update(-1); histogramPoint.Update(0); histogramPoint.Update(2); @@ -76,7 +76,7 @@ public void HistogramDistributeToAllBucketsDefault() public void HistogramDistributeToAllBucketsCustom() { var boundaries = new double[] { 10, 20 }; - var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, null, boundaries); + var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, boundaries); // 5 recordings <=10 histogramPoint.Update(-10); @@ -124,7 +124,7 @@ public void HistogramBinaryBucketTest() boundaries[i] = i; } - var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, null, boundaries); + var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.HistogramWithBuckets, null, boundaries); // Act histogramPoint.Update(-1); @@ -157,7 +157,7 @@ public void HistogramBinaryBucketTest() public void HistogramWithOnlySumCount() { var boundaries = Array.Empty(); - var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.Histogram, null, null, boundaries); + var histogramPoint = new MetricPoint(this.aggregatorStore, AggregationType.Histogram, null, boundaries); histogramPoint.Update(-10); histogramPoint.Update(0); From 1bb346b2dfc3e60cc4be682e233a1c0482e32fa6 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 5 Jan 2023 17:04:16 -0800 Subject: [PATCH 3/7] Rename comparison delegate --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index c05f21bbca4..c82c741bc33 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -23,7 +23,7 @@ namespace OpenTelemetry.Metrics internal sealed class AggregatorStore { private static readonly string MetricPointCapHitFixMessage = "Modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; - private static readonly Comparison> KeyValueComparisonDelegate = KeyValueComparison; + private static readonly Comparison> DimensionComparisonDelegate = DimensionComparison; private readonly object lockZeroTags = new(); private readonly HashSet tagKeysInteresting; private readonly int tagsKeysInterestingCount; @@ -153,7 +153,7 @@ internal void SnapshotCumulative(int indexSnapshot) internal MetricPointsAccessor GetMetricPoints() => new(this.metricPoints, this.currentMetricPointBatch, this.batchSize); - private static int KeyValueComparison(KeyValuePair x, KeyValuePair y) + private static int DimensionComparison(KeyValuePair x, KeyValuePair y) { return x.Key.CompareTo(y.Key); } @@ -188,7 +188,7 @@ private int LookupAggregatorStore(KeyValuePair[] tagKeysAndValue var storage = ThreadStaticStorage.GetStorage(); storage.CloneKeysAndValues(tagKeysAndValues, length, out var tempSortedTagKeysAndValues); - Array.Sort(tempSortedTagKeysAndValues, KeyValueComparisonDelegate); + Array.Sort(tempSortedTagKeysAndValues, DimensionComparisonDelegate); var sortedTags = new Tags(tempSortedTagKeysAndValues); From 986b56cd42ce36b151256941aafc29e97c087fff Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 5 Jan 2023 17:07:54 -0800 Subject: [PATCH 4/7] Remove unnecessary code --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index c82c741bc33..712a791c464 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -434,15 +434,5 @@ private int FindMetricAggregatorsCustomTag(ReadOnlySpan> - { - public static KeyValuePairComparer Instance = new KeyValuePairComparer(); - - public int Compare(KeyValuePair x, KeyValuePair y) - { - return x.Key.CompareTo(y.Key); - } - } } } From 6a34047ec8a1cf3c05750f2ec0a9c9daf193a470 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 6 Jan 2023 11:56:06 -0800 Subject: [PATCH 5/7] Address PR comments --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 7 +----- src/OpenTelemetry/Metrics/Tags.cs | 12 +++++++--- test/Benchmarks/Metrics/MetricsBenchmarks.cs | 24 ++++++++++---------- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index 712a791c464..5b8909ef836 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -23,7 +23,7 @@ namespace OpenTelemetry.Metrics internal sealed class AggregatorStore { private static readonly string MetricPointCapHitFixMessage = "Modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; - private static readonly Comparison> DimensionComparisonDelegate = DimensionComparison; + private static readonly Comparison> DimensionComparisonDelegate = (x, y) => x.Key.CompareTo(y.Key); private readonly object lockZeroTags = new(); private readonly HashSet tagKeysInteresting; private readonly int tagsKeysInterestingCount; @@ -153,11 +153,6 @@ internal void SnapshotCumulative(int indexSnapshot) internal MetricPointsAccessor GetMetricPoints() => new(this.metricPoints, this.currentMetricPointBatch, this.batchSize); - private static int DimensionComparison(KeyValuePair x, KeyValuePair y) - { - return x.Key.CompareTo(y.Key); - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private void InitializeZeroTagPointIfNotInitialized() { diff --git a/src/OpenTelemetry/Metrics/Tags.cs b/src/OpenTelemetry/Metrics/Tags.cs index 7e6c3c5416c..6ee28cf68b5 100644 --- a/src/OpenTelemetry/Metrics/Tags.cs +++ b/src/OpenTelemetry/Metrics/Tags.cs @@ -27,9 +27,12 @@ public Tags(KeyValuePair[] keyValuePairs) var hash = 17; for (int i = 0; i < this.KeyValuePairs.Length; i++) { + ref var item = ref this.KeyValuePairs[i]; + unchecked { - hash = (hash * 31) + this.KeyValuePairs[i].Key.GetHashCode() + this.KeyValuePairs[i].Value?.GetHashCode() ?? 0; + hash = (hash * 31) + item.Key.GetHashCode(); + hash = (hash * 31) + item.Value?.GetHashCode() ?? 0; } } @@ -58,14 +61,17 @@ public readonly bool Equals(Tags other) for (int i = 0; i < length; i++) { + ref var left = ref this.KeyValuePairs[i]; + ref var right = ref other.KeyValuePairs[i]; + // Equality check for Keys - if (!this.KeyValuePairs[i].Key.Equals(other.KeyValuePairs[i].Key, StringComparison.Ordinal)) + if (!left.Key.Equals(right.Key, StringComparison.Ordinal)) { return false; } // Equality check for Values - if (!this.KeyValuePairs[i].Value?.Equals(other.KeyValuePairs[i].Value) ?? other.KeyValuePairs[i].Value != null) + if (!left.Value?.Equals(other.KeyValuePairs[i].Value) ?? right.Value != null) { return false; } diff --git a/test/Benchmarks/Metrics/MetricsBenchmarks.cs b/test/Benchmarks/Metrics/MetricsBenchmarks.cs index 743071347dd..658a8cf4817 100644 --- a/test/Benchmarks/Metrics/MetricsBenchmarks.cs +++ b/test/Benchmarks/Metrics/MetricsBenchmarks.cs @@ -33,18 +33,18 @@ | Method | AggregationTemporality | Mean | Error | StdDev | Allocated | |-------------------------- |----------------------- |----------:|---------:|---------:|----------:| -| CounterHotPath | Cumulative | 14.45 ns | 0.082 ns | 0.068 ns | - | -| CounterWith1LabelsHotPath | Cumulative | 60.54 ns | 0.473 ns | 0.442 ns | - | -| CounterWith3LabelsHotPath | Cumulative | 134.96 ns | 1.543 ns | 1.368 ns | - | -| CounterWith5LabelsHotPath | Cumulative | 206.31 ns | 1.219 ns | 1.081 ns | - | -| CounterWith6LabelsHotPath | Cumulative | 230.43 ns | 0.898 ns | 0.840 ns | - | -| CounterWith7LabelsHotPath | Cumulative | 254.62 ns | 1.156 ns | 1.082 ns | - | -| CounterHotPath | Delta | 13.79 ns | 0.021 ns | 0.019 ns | - | -| CounterWith1LabelsHotPath | Delta | 58.06 ns | 0.227 ns | 0.212 ns | - | -| CounterWith3LabelsHotPath | Delta | 131.89 ns | 0.304 ns | 0.285 ns | - | -| CounterWith5LabelsHotPath | Delta | 207.92 ns | 1.050 ns | 0.982 ns | - | -| CounterWith6LabelsHotPath | Delta | 232.65 ns | 1.011 ns | 0.896 ns | - | -| CounterWith7LabelsHotPath | Delta | 266.57 ns | 5.200 ns | 5.988 ns | - | +| CounterHotPath | Cumulative | 13.82 ns | 0.020 ns | 0.018 ns | - | +| CounterWith1LabelsHotPath | Cumulative | 61.06 ns | 0.276 ns | 0.231 ns | - | +| CounterWith3LabelsHotPath | Cumulative | 135.11 ns | 0.609 ns | 0.540 ns | - | +| CounterWith5LabelsHotPath | Cumulative | 207.05 ns | 0.232 ns | 0.181 ns | - | +| CounterWith6LabelsHotPath | Cumulative | 235.28 ns | 0.513 ns | 0.480 ns | - | +| CounterWith7LabelsHotPath | Cumulative | 261.48 ns | 0.665 ns | 0.589 ns | - | +| CounterHotPath | Delta | 13.88 ns | 0.110 ns | 0.103 ns | - | +| CounterWith1LabelsHotPath | Delta | 58.28 ns | 0.375 ns | 0.351 ns | - | +| CounterWith3LabelsHotPath | Delta | 128.66 ns | 0.246 ns | 0.230 ns | - | +| CounterWith5LabelsHotPath | Delta | 210.62 ns | 1.246 ns | 1.166 ns | - | +| CounterWith6LabelsHotPath | Delta | 233.51 ns | 0.656 ns | 0.614 ns | - | +| CounterWith7LabelsHotPath | Delta | 263.35 ns | 0.865 ns | 0.766 ns | - | */ namespace Benchmarks.Metrics From 17a758bc244f00bdb584acf24ac4119e17554ec3 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 6 Jan 2023 14:33:36 -0800 Subject: [PATCH 6/7] Use Array.Copy instead of Array.CopyTo --- src/OpenTelemetry/Metrics/ThreadStaticStorage.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs b/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs index 1811cd88999..315d05766fa 100644 --- a/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs +++ b/src/OpenTelemetry/Metrics/ThreadStaticStorage.cs @@ -120,11 +120,11 @@ internal void SplitToKeysAndValues(ReadOnlySpan> ta } else { - var tmpTagKeyAndValues = new KeyValuePair[actualLength]; + var tmpTagKeysAndValues = new KeyValuePair[actualLength]; - Array.Copy(tagKeysAndValues, 0, tmpTagKeyAndValues, 0, actualLength); + Array.Copy(tagKeysAndValues, 0, tmpTagKeysAndValues, 0, actualLength); - tagKeysAndValues = tmpTagKeyAndValues; + tagKeysAndValues = tmpTagKeysAndValues; } } } @@ -143,7 +143,7 @@ internal void CloneKeysAndValues(KeyValuePair[] inputTagKeysAndV clonedTagKeysAndValues = new KeyValuePair[tagLength]; } - inputTagKeysAndValues.CopyTo(clonedTagKeysAndValues.AsSpan()); + Array.Copy(inputTagKeysAndValues, 0, clonedTagKeysAndValues, 0, tagLength); } internal sealed class TagStorage From d3e32374e94f16a329447c2fa87681a1b2242dc0 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 6 Jan 2023 15:19:09 -0800 Subject: [PATCH 7/7] Update CHANGELOG --- src/OpenTelemetry/CHANGELOG.md | 37 ++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 5898fecdfdd..fbd56bc6b99 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +* Performance Improvement: Update the internal structure used to store metric + dimensions from a combination of `string[]` and `object[]` to a + `KeyValuePair[]`. This results in faster copying of the metric + dimensions required for `MetricPoint` lookup on the hot path. + ([#4059](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4059)) + ## 1.4.0-rc.1 Released 2022-Dec-12 @@ -146,26 +152,27 @@ Released 2022-June-1 `PeriodicExportingMetricReader`. ([#3291](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3291)) * Add `ConfigureResource` which can replace SetResourceBuilder more succinctly - in most cases and has greater flexibility (applies to - TracerProviderBuilder, MeterProviderBuilder, OpenTelemetryLoggingOptions). + in most cases and has greater flexibility (applies to TracerProviderBuilder, + MeterProviderBuilder, OpenTelemetryLoggingOptions). ([#3307](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3307)) ## 1.3.0-beta.2 Released 2022-May-16 -* Exposed public setters for `LogRecord.State`, `LogRecord.StateValues`, - and `LogRecord.FormattedMessage`. +* Exposed public setters for `LogRecord.State`, `LogRecord.StateValues`, and + `LogRecord.FormattedMessage`. ([#3217](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3217)) ## 1.3.0-beta.1 Released 2022-Apr-15 -* Removes .NET Framework 4.6.1. The minimum .NET Framework - version supported is .NET 4.6.2. ([#3190](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3190)) -* Bumped minimum required version of `Microsoft.Extensions.Logging` - and `Microsoft.Extensions.Logging.Configuration` to 3.1.0 +* Removes .NET Framework 4.6.1. The minimum .NET Framework version supported is + .NET 4.6.2. + ([#3190](https://github.com/open-telemetry/opentelemetry-dotnet/issues/3190)) +* Bumped minimum required version of `Microsoft.Extensions.Logging` and + `Microsoft.Extensions.Logging.Configuration` to 3.1.0 ([#2582](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3196)) ## 1.2.0 @@ -181,15 +188,15 @@ Released 2022-Apr-15 Released 2022-Apr-12 * Removed the `Temporality` setting on `MetricReader` and replaced it with - `TemporalityPreference`. This is a breaking change. - `TemporalityPreference` is used to determine the `AggregationTemporality` - used on a per-instrument kind basis. Currently, there are two preferences: + `TemporalityPreference`. This is a breaking change. `TemporalityPreference` is + used to determine the `AggregationTemporality` used on a per-instrument kind + basis. Currently, there are two preferences: * `Cumulative`: Measurements from all instrument kinds are aggregated using `AggregationTemporality.Cumulative`. * `Delta`: Measurements from `Counter`, `ObservableCounter`, and `Histogram` instruments are aggregated using `AggregationTemporality.Delta`. When - UpDownCounters are supported with - [DiagnosticSource version 7.0 onwards](https://www.nuget.org/packages/System.Diagnostics.DiagnosticSource/7.0.0-preview.2.22152.2), + UpDownCounters are supported with [DiagnosticSource version 7.0 + onwards](https://www.nuget.org/packages/System.Diagnostics.DiagnosticSource/7.0.0-preview.2.22152.2), they will be aggregated using `AggregationTemporality.Cumulative`. ([#3153](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3153)) * Fix issue where `ExplicitBucketHistogramConfiguration` could be used to @@ -243,8 +250,8 @@ Released 2022-Mar-04 differ in some respect - different type, description, or unit - will result in a separate metric stream for each distinct instrument. ([#2916](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2916)) -* The `Meter` property on `OpenTelemetry.Metrics.Metric` has been removed. - It now has `MeterName` and `MeterVersion` properties. +* The `Meter` property on `OpenTelemetry.Metrics.Metric` has been removed. It + now has `MeterName` and `MeterVersion` properties. ([#2916](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2916)) * Added support for implementing custom `ResourceDetector`. ([#2949](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2949/)