From 820085efb1aa7eb8d1bf0f955c38515dd203c136 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 21 Jan 2022 18:24:34 -0800 Subject: [PATCH 1/8] Refactor AggregatorStore to only use two concurrent dictionaries --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 84 +++++++------- .../Metrics/ObjectArrayEqualityComparer.cs | 68 ----------- .../Metrics/StringArrayEqualityComparer.cs | 69 ----------- src/OpenTelemetry/Metrics/Tags.cs | 107 ++++++++++++++++++ 4 files changed, 149 insertions(+), 179 deletions(-) delete mode 100644 src/OpenTelemetry/Metrics/ObjectArrayEqualityComparer.cs delete mode 100644 src/OpenTelemetry/Metrics/StringArrayEqualityComparer.cs create mode 100644 src/OpenTelemetry/Metrics/Tags.cs diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index a4b3df38aef..ebd016a8cc3 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -25,14 +25,15 @@ namespace OpenTelemetry.Metrics { internal sealed class AggregatorStore { - private static readonly ObjectArrayEqualityComparer ObjectArrayComparer = new ObjectArrayEqualityComparer(); private readonly object lockZeroTags = new object(); private readonly HashSet tagKeysInteresting; private readonly int tagsKeysInterestingCount; - // Two-Level lookup. TagKeys x [ TagValues x Metrics ] - private readonly ConcurrentDictionary> keyValue2MetricAggs = - new ConcurrentDictionary>(new StringArrayEqualityComparer()); + private readonly ConcurrentDictionary sortedTagsDictionary = + new ConcurrentDictionary(); + + private readonly ConcurrentDictionary tagsToMetricPointIndexDictionary = + new ConcurrentDictionary(); private readonly AggregationTemporality temporality; private readonly string name; @@ -178,27 +179,46 @@ private void InitializeZeroTagPointIfNotInitialized() [MethodImpl(MethodImplOptions.AggressiveInlining)] private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int length) { - int aggregatorIndex; - string[] seqKey = null; + var givenTags = new Tags(tagKeys, tagValues); - // GetOrAdd by TagKeys at 1st Level of 2-level dictionary structure. - // Get back a Dictionary of [ Values x Metrics[] ]. - if (!this.keyValue2MetricAggs.TryGetValue(tagKeys, out var value2metrics)) + // We only need to sort if there is more than one Tag Key. + if (!this.sortedTagsDictionary.TryGetValue(givenTags, out var sortedTags)) { // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. - seqKey = new string[length]; - tagKeys.CopyTo(seqKey, 0); + var givenKeys = new string[length]; + tagKeys.CopyTo(givenKeys, 0); + + var givenValues = new object[length]; + tagValues.CopyTo(givenValues, 0); + + givenTags = new Tags(givenKeys, givenValues); + + string[] sortedTagKeys; + object[] sortedTagValues; + + if (length > 1) + { + // Create a new array for the sorted Tag keys. + sortedTagKeys = new string[length]; + tagKeys.CopyTo(sortedTagKeys, 0); + + // Create a new array for the sorted Tag values. + sortedTagValues = new object[length]; + tagValues.CopyTo(sortedTagValues, 0); - value2metrics = new ConcurrentDictionary(ObjectArrayComparer); - if (!this.keyValue2MetricAggs.TryAdd(seqKey, value2metrics)) + Array.Sort(sortedTagKeys, sortedTagValues); + } + else { - this.keyValue2MetricAggs.TryGetValue(seqKey, out value2metrics); + sortedTagKeys = givenKeys; + sortedTagValues = givenValues; } + + sortedTags = new Tags(sortedTagKeys, sortedTagValues); + this.sortedTagsDictionary.TryAdd(givenTags, sortedTags); } - // GetOrAdd by TagValues at 2st Level of 2-level dictionary structure. - // Get back Metrics[]. - if (!value2metrics.TryGetValue(tagValues, out aggregatorIndex)) + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out var aggregatorIndex)) { aggregatorIndex = this.metricPointIndex; if (aggregatorIndex >= this.maxMetricPoints) @@ -210,12 +230,12 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng return -1; } - lock (value2metrics) + lock (this.tagsToMetricPointIndexDictionary) { // check again after acquiring lock. - if (!value2metrics.TryGetValue(tagValues, out aggregatorIndex)) + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) { - aggregatorIndex = Interlocked.Increment(ref this.metricPointIndex); + aggregatorIndex = ++this.metricPointIndex; if (aggregatorIndex >= this.maxMetricPoints) { // sorry! out of data points. @@ -225,24 +245,14 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng return -1; } - // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. - if (seqKey == null) - { - seqKey = new string[length]; - tagKeys.CopyTo(seqKey, 0); - } - - var seqVal = new object[length]; - tagValues.CopyTo(seqVal, 0); - ref var metricPoint = ref this.metricPoints[aggregatorIndex]; var dt = DateTimeOffset.UtcNow; - metricPoint = new MetricPoint(this.aggType, dt, seqKey, seqVal, this.histogramBounds); + metricPoint = new MetricPoint(this.aggType, dt, sortedTags.Keys, sortedTags.Values, this.histogramBounds); // Add to dictionary *after* initializing MetricPoint // as other threads can start writing to the // MetricPoint, if dictionary entry found. - value2metrics.TryAdd(seqVal, aggregatorIndex); + this.tagsToMetricPointIndexDictionary.TryAdd(sortedTags, aggregatorIndex); } } } @@ -355,11 +365,6 @@ private int FindMetricAggregatorsDefault(ReadOnlySpan 1) - { - Array.Sort(tagKeys, tagValues); - } - return this.LookupAggregatorStore(tagKeys, tagValues, tagLength); } @@ -388,11 +393,6 @@ private int FindMetricAggregatorsCustomTag(ReadOnlySpan 1) - { - Array.Sort(tagKeys, tagValues); - } - return this.LookupAggregatorStore(tagKeys, tagValues, actualLength); } } diff --git a/src/OpenTelemetry/Metrics/ObjectArrayEqualityComparer.cs b/src/OpenTelemetry/Metrics/ObjectArrayEqualityComparer.cs deleted file mode 100644 index 3d806ac751e..00000000000 --- a/src/OpenTelemetry/Metrics/ObjectArrayEqualityComparer.cs +++ /dev/null @@ -1,68 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using System.Collections.Generic; - -namespace OpenTelemetry.Metrics -{ - internal class ObjectArrayEqualityComparer : IEqualityComparer - { - public bool Equals(object[] obj1, object[] obj2) - { - if (ReferenceEquals(obj1, obj2)) - { - return true; - } - - if (ReferenceEquals(obj1, null) || ReferenceEquals(obj2, null)) - { - return false; - } - - var len1 = obj1.Length; - - if (len1 != obj2.Length) - { - return false; - } - - for (int i = 0; i < len1; i++) - { - if (!obj1[i].Equals(obj2[i])) - { - return false; - } - } - - return true; - } - - public int GetHashCode(object[] objs) - { - int hash = 17; - - unchecked - { - for (int i = 0; i < objs.Length; i++) - { - hash = (hash * 31) + objs[i].GetHashCode(); - } - } - - return hash; - } - } -} diff --git a/src/OpenTelemetry/Metrics/StringArrayEqualityComparer.cs b/src/OpenTelemetry/Metrics/StringArrayEqualityComparer.cs deleted file mode 100644 index 6a91f201eae..00000000000 --- a/src/OpenTelemetry/Metrics/StringArrayEqualityComparer.cs +++ /dev/null @@ -1,69 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using System; -using System.Collections.Generic; - -namespace OpenTelemetry.Metrics -{ - internal class StringArrayEqualityComparer : IEqualityComparer - { - public bool Equals(string[] strings1, string[] strings2) - { - if (ReferenceEquals(strings1, strings2)) - { - return true; - } - - if (ReferenceEquals(strings1, null) || ReferenceEquals(strings2, null)) - { - return false; - } - - var len1 = strings1.Length; - - if (len1 != strings2.Length) - { - return false; - } - - for (int i = 0; i < len1; i++) - { - if (!strings1[i].Equals(strings2[i], StringComparison.Ordinal)) - { - return false; - } - } - - return true; - } - - public int GetHashCode(string[] strings) - { - int hash = 17; - - unchecked - { - for (int i = 0; i < strings.Length; i++) - { - hash = (hash * 31) + strings[i].GetHashCode(); - } - } - - return hash; - } - } -} diff --git a/src/OpenTelemetry/Metrics/Tags.cs b/src/OpenTelemetry/Metrics/Tags.cs new file mode 100644 index 00000000000..f560d7d657e --- /dev/null +++ b/src/OpenTelemetry/Metrics/Tags.cs @@ -0,0 +1,107 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System; + +namespace OpenTelemetry.Metrics +{ + internal readonly struct Tags : IEquatable + { + public Tags(string[] keys, object[] values) + { + this.Keys = keys; + this.Values = values; + } + + public readonly string[] Keys { get; } + + public readonly object[] Values { get; } + + public static bool operator ==(Tags tag1, Tags tag2) => tag1.Equals(tag2); + + public static bool operator !=(Tags tag1, Tags tag2) => !tag1.Equals(tag2); + + public readonly override bool Equals(object obj) + { + if (!(obj is Tags)) + { + return false; + } + + var other = (Tags)obj; + return this.Equals(other); + } + + public readonly bool Equals(Tags other) + { + // Equality check for Keys + // Check if the two string[] are equal + var keysLength = this.Keys.Length; + + if (keysLength != other.Keys.Length) + { + return false; + } + + for (int i = 0; i < keysLength; i++) + { + if (!this.Keys[i].Equals(other.Keys[i], 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])) + { + return false; + } + } + + return true; + } + + public readonly override int GetHashCode() + { + int hash = 17; + + unchecked + { + for (int i = 0; i < this.Keys.Length; i++) + { + hash = (hash * 31) + this.Keys[i].GetHashCode(); + } + + for (int i = 0; i < this.Values.Length; i++) + { + hash = (hash * 31) + this.Values[i].GetHashCode(); + } + } + + return hash; + } + } +} From 99c9ef46ddb5d571efe185a7aec181ff5c61e467 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Thu, 27 Jan 2022 15:53:07 -0800 Subject: [PATCH 2/8] Addressing PR suggestions --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 110 +++++++++---------- src/OpenTelemetry/Metrics/Tags.cs | 8 +- 2 files changed, 56 insertions(+), 62 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index ebd016a8cc3..ee64628f26d 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -29,9 +29,6 @@ internal sealed class AggregatorStore private readonly HashSet tagKeysInteresting; private readonly int tagsKeysInterestingCount; - private readonly ConcurrentDictionary sortedTagsDictionary = - new ConcurrentDictionary(); - private readonly ConcurrentDictionary tagsToMetricPointIndexDictionary = new ConcurrentDictionary(); @@ -181,78 +178,81 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng { var givenTags = new Tags(tagKeys, tagValues); - // We only need to sort if there is more than one Tag Key. - if (!this.sortedTagsDictionary.TryGetValue(givenTags, out var sortedTags)) + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(givenTags, out var aggregatorIndex)) { - // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. - var givenKeys = new string[length]; - tagKeys.CopyTo(givenKeys, 0); - - var givenValues = new object[length]; - tagValues.CopyTo(givenValues, 0); - - givenTags = new Tags(givenKeys, givenValues); - - string[] sortedTagKeys; - object[] sortedTagValues; - + Tags sortedTags; if (length > 1) { + // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. // Create a new array for the sorted Tag keys. - sortedTagKeys = new string[length]; + var sortedTagKeys = new string[length]; tagKeys.CopyTo(sortedTagKeys, 0); // Create a new array for the sorted Tag values. - sortedTagValues = new object[length]; + var sortedTagValues = new object[length]; tagValues.CopyTo(sortedTagValues, 0); Array.Sort(sortedTagKeys, sortedTagValues); + + sortedTags = new Tags(sortedTagKeys, sortedTagValues); } else { - sortedTagKeys = givenKeys; - sortedTagValues = givenValues; + sortedTags = givenTags; } - sortedTags = new Tags(sortedTagKeys, sortedTagValues); - this.sortedTagsDictionary.TryAdd(givenTags, sortedTags); - } - - if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out var aggregatorIndex)) - { - aggregatorIndex = this.metricPointIndex; - if (aggregatorIndex >= this.maxMetricPoints) + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) { - // sorry! out of data points. - // TODO: Once we support cleanup of - // unused points (typically with delta) - // we can re-claim them here. - return -1; - } + // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. + var givenKeys = new string[length]; + tagKeys.CopyTo(givenKeys, 0); - lock (this.tagsToMetricPointIndexDictionary) - { - // check again after acquiring lock. - if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) + var givenValues = new object[length]; + tagValues.CopyTo(givenValues, 0); + + givenTags = new Tags(givenKeys, givenValues); + + aggregatorIndex = this.metricPointIndex; + if (aggregatorIndex >= this.maxMetricPoints) { - aggregatorIndex = ++this.metricPointIndex; - if (aggregatorIndex >= this.maxMetricPoints) + // sorry! out of data points. + // TODO: Once we support cleanup of + // unused points (typically with delta) + // we can re-claim them here. + return -1; + } + + lock (this.tagsToMetricPointIndexDictionary) + { + // check again after acquiring lock. + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) { - // sorry! out of data points. - // TODO: Once we support cleanup of - // unused points (typically with delta) - // we can re-claim them here. - return -1; + aggregatorIndex = ++this.metricPointIndex; + if (aggregatorIndex >= this.maxMetricPoints) + { + // sorry! out of data points. + // TODO: Once we support cleanup of + // unused points (typically with delta) + // we can re-claim them here. + return -1; + } + + ref var metricPoint = ref this.metricPoints[aggregatorIndex]; + var dt = DateTimeOffset.UtcNow; + metricPoint = new MetricPoint(this.aggType, dt, sortedTags.Keys, sortedTags.Values, this.histogramBounds); + + // Add to dictionary *after* initializing MetricPoint + // as other threads can start writing to the + // MetricPoint, if dictionary entry found. + if (length > 1) + { + // Add the sorted order only if tags length > 1 + this.tagsToMetricPointIndexDictionary.TryAdd(sortedTags, aggregatorIndex); + } + + // givenTags and sortedTags are the same when tags length < 1 + this.tagsToMetricPointIndexDictionary.TryAdd(givenTags, aggregatorIndex); } - - ref var metricPoint = ref this.metricPoints[aggregatorIndex]; - var dt = DateTimeOffset.UtcNow; - metricPoint = new MetricPoint(this.aggType, dt, sortedTags.Keys, sortedTags.Values, this.histogramBounds); - - // Add to dictionary *after* initializing MetricPoint - // as other threads can start writing to the - // MetricPoint, if dictionary entry found. - this.tagsToMetricPointIndexDictionary.TryAdd(sortedTags, aggregatorIndex); } } } diff --git a/src/OpenTelemetry/Metrics/Tags.cs b/src/OpenTelemetry/Metrics/Tags.cs index f560d7d657e..c112bcdbe52 100644 --- a/src/OpenTelemetry/Metrics/Tags.cs +++ b/src/OpenTelemetry/Metrics/Tags.cs @@ -36,13 +36,7 @@ public Tags(string[] keys, object[] values) public readonly override bool Equals(object obj) { - if (!(obj is Tags)) - { - return false; - } - - var other = (Tags)obj; - return this.Equals(other); + return obj is Tags other && this.Equals(other); } public readonly bool Equals(Tags other) From 27579dc1cb79fa64fa337127ebb575f52d0eac8c Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 28 Jan 2022 11:49:12 -0800 Subject: [PATCH 3/8] Address PR comments for Perf optimization --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 87 ++++++++++++++++---- src/OpenTelemetry/Metrics/Tags.cs | 7 +- 2 files changed, 70 insertions(+), 24 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index ee64628f26d..a54ba2a0d39 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -180,7 +180,6 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng if (!this.tagsToMetricPointIndexDictionary.TryGetValue(givenTags, out var aggregatorIndex)) { - Tags sortedTags; if (length > 1) { // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. @@ -194,20 +193,77 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng Array.Sort(sortedTagKeys, sortedTagValues); - sortedTags = new Tags(sortedTagKeys, sortedTagValues); + var sortedTags = new Tags(sortedTagKeys, sortedTagValues); + + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) + { + // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. + var givenKeys = new string[length]; + tagKeys.CopyTo(givenKeys, 0); + + var givenValues = new object[length]; + tagValues.CopyTo(givenValues, 0); + + givenTags = new Tags(givenKeys, givenValues); + + aggregatorIndex = this.metricPointIndex; + if (aggregatorIndex >= this.maxMetricPoints) + { + // sorry! out of data points. + // TODO: Once we support cleanup of + // unused points (typically with delta) + // we can re-claim them here. + return -1; + } + + lock (this.tagsToMetricPointIndexDictionary) + { + // check again after acquiring lock. + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) + { + aggregatorIndex = ++this.metricPointIndex; + if (aggregatorIndex >= this.maxMetricPoints) + { + // sorry! out of data points. + // TODO: Once we support cleanup of + // unused points (typically with delta) + // we can re-claim them here. + return -1; + } + + ref var metricPoint = ref this.metricPoints[aggregatorIndex]; + var dt = DateTimeOffset.UtcNow; + metricPoint = new MetricPoint(this.aggType, dt, sortedTags.Keys, sortedTags.Values, this.histogramBounds); + + // Add to dictionary *after* initializing MetricPoint + // as other threads can start writing to the + // MetricPoint, if dictionary entry found. + + // Add the sorted order along with the given order of tags + this.tagsToMetricPointIndexDictionary.TryAdd(sortedTags, aggregatorIndex); + this.tagsToMetricPointIndexDictionary.TryAdd(givenTags, aggregatorIndex); + } + } + } } else - { - sortedTags = givenTags; - } - - if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) { // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. - var givenKeys = new string[length]; - tagKeys.CopyTo(givenKeys, 0); + string[] givenKeys; + object[] givenValues; - var givenValues = new object[length]; + if (length == 0) + { + givenKeys = Array.Empty(); + givenValues = Array.Empty(); + } + else + { + givenKeys = new string[length]; + givenValues = new object[length]; + } + + tagKeys.CopyTo(givenKeys, 0); tagValues.CopyTo(givenValues, 0); givenTags = new Tags(givenKeys, givenValues); @@ -225,7 +281,7 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng lock (this.tagsToMetricPointIndexDictionary) { // check again after acquiring lock. - if (!this.tagsToMetricPointIndexDictionary.TryGetValue(sortedTags, out aggregatorIndex)) + if (!this.tagsToMetricPointIndexDictionary.TryGetValue(givenTags, out aggregatorIndex)) { aggregatorIndex = ++this.metricPointIndex; if (aggregatorIndex >= this.maxMetricPoints) @@ -239,18 +295,13 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng ref var metricPoint = ref this.metricPoints[aggregatorIndex]; var dt = DateTimeOffset.UtcNow; - metricPoint = new MetricPoint(this.aggType, dt, sortedTags.Keys, sortedTags.Values, this.histogramBounds); + metricPoint = new MetricPoint(this.aggType, dt, givenTags.Keys, givenTags.Values, this.histogramBounds); // Add to dictionary *after* initializing MetricPoint // as other threads can start writing to the // MetricPoint, if dictionary entry found. - if (length > 1) - { - // Add the sorted order only if tags length > 1 - this.tagsToMetricPointIndexDictionary.TryAdd(sortedTags, aggregatorIndex); - } - // givenTags and sortedTags are the same when tags length < 1 + // givenTags will alwayas be sorted when tags length < 1 this.tagsToMetricPointIndexDictionary.TryAdd(givenTags, aggregatorIndex); } } diff --git a/src/OpenTelemetry/Metrics/Tags.cs b/src/OpenTelemetry/Metrics/Tags.cs index c112bcdbe52..1357a2848d3 100644 --- a/src/OpenTelemetry/Metrics/Tags.cs +++ b/src/OpenTelemetry/Metrics/Tags.cs @@ -86,12 +86,7 @@ public readonly override int GetHashCode() { for (int i = 0; i < this.Keys.Length; i++) { - hash = (hash * 31) + this.Keys[i].GetHashCode(); - } - - for (int i = 0; i < this.Values.Length; i++) - { - hash = (hash * 31) + this.Values[i].GetHashCode(); + hash = (hash * 31) + this.Keys[i].GetHashCode() + this.Values[i].GetHashCode(); } } From 6eb6089544db18662d0a8c103fa297ae7884d0aa Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 28 Jan 2022 12:17:12 -0800 Subject: [PATCH 4/8] Add CHANGELOG.md --- src/OpenTelemetry/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index db0d25f5b90..c4ead81fbd9 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -8,6 +8,10 @@ * Fail-fast when using AddView with guaranteed conflict. ([2751](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2751)) +* Performance Improvement: When emitting metrics, users are strongly advised to + provide tags with same Key order, to achieve maximum performance. + ([2805](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2805)) + ## 1.2.0-rc1 Released 2021-Nov-29 From ad0410b4d60bb553eb338816fa27aad734edfa38 Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 28 Jan 2022 13:00:08 -0800 Subject: [PATCH 5/8] Update src/OpenTelemetry/CHANGELOG.md Co-authored-by: Cijo Thomas --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index c4ead81fbd9..c79f046737a 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -10,7 +10,7 @@ * Performance Improvement: When emitting metrics, users are strongly advised to provide tags with same Key order, to achieve maximum performance. - ([2805](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2805)) + ([2805](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2805/files)) ## 1.2.0-rc1 From 3a46d68956c9b94c4d3531290a5187c80d97a64c Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 28 Jan 2022 13:12:24 -0800 Subject: [PATCH 6/8] Update src/OpenTelemetry/Metrics/AggregatorStore.cs Co-authored-by: Cijo Thomas --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index a54ba2a0d39..b1846ede80a 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -301,7 +301,7 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng // as other threads can start writing to the // MetricPoint, if dictionary entry found. - // givenTags will alwayas be sorted when tags length < 1 + // givenTags will always be sorted when tags length == 1 this.tagsToMetricPointIndexDictionary.TryAdd(givenTags, aggregatorIndex); } } From 64b86e5edd103a80522fdcd08f59d340e7799dbb Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Fri, 28 Jan 2022 13:15:58 -0800 Subject: [PATCH 7/8] Remove unreachable code --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index b1846ede80a..c61f1a59273 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -249,19 +249,8 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng else { // Note: We are using storage from ThreadStatic, so need to make a deep copy for Dictionary storage. - string[] givenKeys; - object[] givenValues; - - if (length == 0) - { - givenKeys = Array.Empty(); - givenValues = Array.Empty(); - } - else - { - givenKeys = new string[length]; - givenValues = new object[length]; - } + var givenKeys = new string[length]; + var givenValues = new object[length]; tagKeys.CopyTo(givenKeys, 0); tagValues.CopyTo(givenValues, 0); From 333680a8e557d6e365da2b42813e648bdaaba18a Mon Sep 17 00:00:00 2001 From: Utkarsh Umesan Pillai Date: Mon, 31 Jan 2022 11:03:48 -0800 Subject: [PATCH 8/8] Update src/OpenTelemetry/CHANGELOG.md Co-authored-by: Reiley Yang --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index c79f046737a..db85ebe3f16 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -8,7 +8,7 @@ * Fail-fast when using AddView with guaranteed conflict. ([2751](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2751)) -* Performance Improvement: When emitting metrics, users are strongly advised to +* Performance improvement: when emitting metrics, users are strongly advised to provide tags with same Key order, to achieve maximum performance. ([2805](https://github.com/open-telemetry/opentelemetry-dotnet/pull/2805/files))