Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize unnecessarily allocation when initializing MetricProvider #1685

Merged
merged 11 commits into from
Jan 29, 2021
Merged

Optimize unnecessarily allocation when initializing MetricProvider #1685

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5d7d56a
Optimize unnecessary allocation when using GetOrAdd() on Collection o…
victlu Jan 4, 2021
3d2d246
Refactor SortAndDedup method for LabelSets
victlu Jan 4, 2021
eeb03c7
Add Tests for Double versions of Counter and Measures
victlu Jan 5, 2021
a398d7e
Refactor to use TryGetValue/TryGetOrAdd instead of Func<> due to conc…
victlu Jan 8, 2021
75ea5e3
Fix CR/LF to LF
victlu Jan 8, 2021
3f9d070
Refactor to avoid allocation of Func<>
victlu Jan 14, 2021
e661b9b
Fix styling issue
victlu Jan 14, 2021
8a1ca50
Merge branch 'main' into Optimize-GetOrAdd
cijothomas Jan 28, 2021
e57e30d
kick build to start
victlu Jan 28, 2021
65f72c2
Merge branch 'main' into Optimize-GetOrAdd
victlu Jan 28, 2021
0505585
Merge branch 'main' into Optimize-GetOrAdd
cijothomas Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ internal BoundCounterMetric<T> Bind(LabelSet labelset, bool isShortLived)
lock (this.bindUnbindLock)
{
var recStatus = isShortLived ? RecordStatus.UpdatePending : RecordStatus.Bound;
boundInstrument = this.counterBoundInstruments.GetOrAdd(labelset, this.CreateMetric(recStatus));
if (!this.counterBoundInstruments.TryGetValue(labelset, out boundInstrument))
{
boundInstrument = this.counterBoundInstruments.GetOrAdd(labelset, this.CreateMetric(recStatus));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to be:

	            if (!this.counterBoundInstruments.TryGetValue(labelset, out boundInstrument))
                {
                    boundInstrument = this.CreateMetric(recStatus);
                    this.counterBoundInstruments.TryAdd(labelset, boundInstrument);
                }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the TryAdd fail, we will return the wrong boundInstrument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetOrAdd doesn't guarantee anything different, does it? From the doc:

Since a key/value can be inserted by another thread while valueFactory is generating a value, you cannot trust that just because valueFactory executed, its produced value will be inserted into the dictionary and returned. If you call GetOrAdd simultaneously on different threads, valueFactory may be called multiple times, but only one key/value pair will be added to the dictionary.

If we need a guarantee why not use a Dictionary in a lock?

}
}

switch (boundInstrument.Status)
Expand Down
8 changes: 6 additions & 2 deletions src/OpenTelemetry/Metrics/DoubleObserverMetricSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,12 @@ public DoubleObserverMetricSdk(string name, Action<DoubleObserverMetric> callbac
public override void Observe(double value, LabelSet labelset)
{
// TODO cleanup of handle/aggregator. Issue #530
var boundInstrument =
this.observerHandles.GetOrAdd(labelset, new DoubleObserverMetricHandleSdk());

DoubleObserverMetricHandleSdk boundInstrument;
if (!this.observerHandles.TryGetValue(labelset, out boundInstrument))
{
boundInstrument = this.observerHandles.GetOrAdd(labelset, new DoubleObserverMetricHandleSdk());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These other ones that don't need a closure you could use a static delegate:

        private static readonly Func<LabelSet, DoubleObserverMetricHandleSdk> CreateMetricFunc = (labelSet) => new DoubleObserverMetricHandleSdk();

        public override void Observe(double value, LabelSet labelset)
        {
            // TODO cleanup of handle/aggregator.   Issue #530
            var boundInstrument =
                this.observerHandles.GetOrAdd(labelset, CreateMetricFunc);

            boundInstrument.Observe(value);
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is necessary as we already called TryGetValue prior. Thus, the expectation is that we will need to alloc the new item. Thus, converting to a static delegate is just adding 1 extra layer.

If your concern is about atomicity, then, both these are equivalent. In the case of a race condition, the GetOrAdd(Func<>) form does not guarantee atomicity either. Thus, the functionally is no different than current code. In both cases, we will wind up "leaking" the new object regardless.

IMHO, In this code base, I would prefer using the GetOrAdd(Func<>) syntax rather than break into TryGetValue/TryGetOrAdd pattern. Simply because the code is more readable and understandable. Plus, the compiler has the ability to optimize to "static" delegates as it sees fit. Also, going forward in newer .NET, we have the GetOrAdd syntax with passing in TArg, which addresses the performance with closure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is necessary as we already called TryGetValue prior. Thus, the expectation is that we will need to alloc the new item. Thus, converting to a static delegate is just adding 1 extra layer.

Don't call TryGetValue for the case where a static delegate will work, just call TryGetValue passing the delegate ref (most places). Only call TryGetValue/TryAdd when you need the closure (the one place).

IMHO, In this code base, I would prefer using the GetOrAdd(Func<>) syntax rather than break into TryGetValue/TryGetOrAdd pattern.

Let's go with the allocation-free patterns.

}

boundInstrument.Observe(value);
}
Expand Down
7 changes: 5 additions & 2 deletions src/OpenTelemetry/Metrics/Int64ObserverMetricSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@ public Int64ObserverMetricSdk(string name, Action<Int64ObserverMetric> callback)
public override void Observe(long value, LabelSet labelset)
{
// TODO cleanup of handle/aggregator. Issue #530
var boundInstrument =
this.observerHandles.GetOrAdd(labelset, new Int64ObserverMetricHandleSdk());
Int64ObserverMetricHandleSdk boundInstrument;
if (!this.observerHandles.TryGetValue(labelset, out boundInstrument))
{
boundInstrument = this.observerHandles.GetOrAdd(labelset, new Int64ObserverMetricHandleSdk());
}

boundInstrument.Observe(value);
}
Expand Down
26 changes: 4 additions & 22 deletions src/OpenTelemetry/Metrics/LabelSetSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,31 +62,13 @@ public override bool Equals(object obj)

private static IEnumerable<KeyValuePair<string, string>> SortAndDedup(IEnumerable<KeyValuePair<string, string>> labels)
{
// TODO - could be optimized to avoid creating List twice.
var orderedList = labels.OrderBy(x => x.Key).ToList();
if (orderedList.Count == 1)
{
return orderedList;
}

var dedupedList = new List<KeyValuePair<string, string>>();

int dedupedListIndex = 0;
dedupedList.Add(orderedList[dedupedListIndex]);
for (int i = 1; i < orderedList.Count; i++)
var dedupedList = new SortedDictionary<string, KeyValuePair<string, string>>(StringComparer.Ordinal);
foreach (var label in labels)
{
if (orderedList[i].Key.Equals(orderedList[i - 1].Key, StringComparison.Ordinal))
{
dedupedList[dedupedListIndex] = orderedList[i];
}
else
{
dedupedList.Add(orderedList[i]);
dedupedListIndex++;
}
dedupedList[label.Key] = label;
}

return dedupedList;
return dedupedList.Values;
}

private static string GetLabelSetEncoded(IEnumerable<KeyValuePair<string, string>> labels)
Expand Down
8 changes: 7 additions & 1 deletion src/OpenTelemetry/Metrics/MeasureMetricSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,13 @@ public MeasureMetricSdk(string name)

public override BoundMeasureMetric<T> Bind(LabelSet labelset)
{
return this.measureBoundInstruments.GetOrAdd(labelset, this.CreateMetric());
BoundMeasureMetricSdkBase<T> boundMeasure;
if (!this.measureBoundInstruments.TryGetValue(labelset, out boundMeasure))
{
boundMeasure = this.measureBoundInstruments.GetOrAdd(labelset, this.CreateMetric());
}

return boundMeasure;
}

public override BoundMeasureMetric<T> Bind(IEnumerable<KeyValuePair<string, string>> labels)
Expand Down
48 changes: 42 additions & 6 deletions src/OpenTelemetry/Metrics/MeterSdk.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,34 +244,70 @@ public virtual void Collect()

public override CounterMetric<long> CreateInt64Counter(string name, bool monotonic = true)
{
return this.longCounters.GetOrAdd(name, new Int64CounterMetricSdk(name));
Int64CounterMetricSdk metric;
if (!this.longCounters.TryGetValue(name, out metric))
{
metric = this.longCounters.GetOrAdd(name, new Int64CounterMetricSdk(name));
}

return metric;
}

public override CounterMetric<double> CreateDoubleCounter(string name, bool monotonic = true)
{
return this.doubleCounters.GetOrAdd(name, new DoubleCounterMetricSdk(name));
DoubleCounterMetricSdk metric;
if (!this.doubleCounters.TryGetValue(name, out metric))
{
metric = this.doubleCounters.GetOrAdd(name, new DoubleCounterMetricSdk(name));
}

return metric;
}

public override MeasureMetric<double> CreateDoubleMeasure(string name, bool absolute = true)
{
return this.doubleMeasures.GetOrAdd(name, new DoubleMeasureMetricSdk(name));
DoubleMeasureMetricSdk metric;
if (!this.doubleMeasures.TryGetValue(name, out metric))
{
metric = this.doubleMeasures.GetOrAdd(name, new DoubleMeasureMetricSdk(name));
}

return metric;
}

public override MeasureMetric<long> CreateInt64Measure(string name, bool absolute = true)
{
return this.longMeasures.GetOrAdd(name, new Int64MeasureMetricSdk(name));
Int64MeasureMetricSdk metric;
if (!this.longMeasures.TryGetValue(name, out metric))
{
metric = this.longMeasures.GetOrAdd(name, new Int64MeasureMetricSdk(name));
}

return metric;
}

/// <inheritdoc/>
public override Int64ObserverMetric CreateInt64Observer(string name, Action<Int64ObserverMetric> callback, bool absolute = true)
{
return this.longObservers.GetOrAdd(name, new Int64ObserverMetricSdk(name, callback));
Int64ObserverMetricSdk metric;
if (!this.longObservers.TryGetValue(name, out metric))
{
metric = this.longObservers.GetOrAdd(name, new Int64ObserverMetricSdk(name, callback));
}

return metric;
}

/// <inheritdoc/>
public override DoubleObserverMetric CreateDoubleObserver(string name, Action<DoubleObserverMetric> callback, bool absolute = true)
{
return this.doubleObservers.GetOrAdd(name, new DoubleObserverMetricSdk(name, callback));
DoubleObserverMetricSdk metric;
if (!this.doubleObservers.TryGetValue(name, out metric))
{
metric = this.doubleObservers.GetOrAdd(name, new DoubleObserverMetricSdk(name, callback));
}

return metric;
}
}
}
110 changes: 108 additions & 2 deletions test/OpenTelemetry.Tests/Metrics/MetricsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace OpenTelemetry.Metrics.Tests
public class MetricsTest
{
[Fact]
public void CounterSendsAggregateToRegisteredProcessor()
public void LongCounterSendsAggregateToRegisteredProcessor()
{
var testProcessor = new TestMetricProcessor();
var meter = Sdk.CreateMeterProviderBuilder()
Expand Down Expand Up @@ -78,7 +78,63 @@ public void CounterSendsAggregateToRegisteredProcessor()
}

[Fact]
public void MeasureSendsAggregateToRegisteredProcessor()
public void DoubleCounterSendsAggregateToRegisteredProcessor()
{
var testProcessor = new TestMetricProcessor();
var meter = Sdk.CreateMeterProviderBuilder()
.SetProcessor(testProcessor)
.Build()
.GetMeter("library1") as MeterSdk;

var testCounter = meter.CreateDoubleCounter("testCounter");

var labels1 = new List<KeyValuePair<string, string>>();
labels1.Add(new KeyValuePair<string, string>("dim1", "value1"));

var labels2 = new List<KeyValuePair<string, string>>();
labels2.Add(new KeyValuePair<string, string>("dim1", "value2"));

var labels3 = new List<KeyValuePair<string, string>>();
labels3.Add(new KeyValuePair<string, string>("dim1", "value3"));

var context = default(SpanContext);
testCounter.Add(context, 100.2, meter.GetLabelSet(labels1));
testCounter.Add(context, 10.2, meter.GetLabelSet(labels1));

var boundCounterLabel2 = testCounter.Bind(labels2);
boundCounterLabel2.Add(context, 200.2);

testCounter.Add(context, 200.2, meter.GetLabelSet(labels3));
testCounter.Add(context, 10.2, meter.GetLabelSet(labels3));

meter.Collect();

Assert.Single(testProcessor.Metrics);
var metric = testProcessor.Metrics[0];

Assert.Equal("testCounter", metric.MetricName);
Assert.Equal("library1", metric.MetricNamespace);

// 3 time series, as 3 unique label sets.
Assert.Equal(3, metric.Data.Count);
var expectedSum = 100.2 + 10.2;
var metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value1"));
var metricDouble = metricSeries as DoubleSumData;
Assert.Equal(expectedSum, metricDouble.Sum);

expectedSum = 200.2;
metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value2"));
metricDouble = metricSeries as DoubleSumData;
Assert.Equal(expectedSum, metricDouble.Sum);

expectedSum = 200.2 + 10.2;
metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value3"));
metricDouble = metricSeries as DoubleSumData;
Assert.Equal(expectedSum, metricDouble.Sum);
}

[Fact]
public void LongMeasureSendsAggregateToRegisteredProcessor()
{
var testProcessor = new TestMetricProcessor();
var meter = Sdk.CreateMeterProviderBuilder()
Expand Down Expand Up @@ -125,6 +181,56 @@ public void MeasureSendsAggregateToRegisteredProcessor()
Assert.Equal(200, metricSummary.Max);
}

[Fact]
public void DoubleMeasureSendsAggregateToRegisteredProcessor()
{
var testProcessor = new TestMetricProcessor();
var meter = Sdk.CreateMeterProviderBuilder()
.SetProcessor(testProcessor)
.Build()
.GetMeter("library1") as MeterSdk;
var testMeasure = meter.CreateDoubleMeasure("testMeasure");

var labels1 = new List<KeyValuePair<string, string>>();
labels1.Add(new KeyValuePair<string, string>("dim1", "value1"));

var labels2 = new List<KeyValuePair<string, string>>();
labels2.Add(new KeyValuePair<string, string>("dim1", "value2"));

var context = default(SpanContext);
testMeasure.Record(context, 100.2, meter.GetLabelSet(labels1));
testMeasure.Record(context, 10.2, meter.GetLabelSet(labels1));
testMeasure.Record(context, 1.2, meter.GetLabelSet(labels1));
testMeasure.Record(context, 200.2, meter.GetLabelSet(labels2));
testMeasure.Record(context, 20.2, meter.GetLabelSet(labels2));

meter.Collect();

Assert.Single(testProcessor.Metrics);
var metric = testProcessor.Metrics[0];
Assert.Equal("testMeasure", metric.MetricName);
Assert.Equal("library1", metric.MetricNamespace);

// 2 time series, as 2 unique label sets.
Assert.Equal(2, metric.Data.Count);

var expectedSum = 100.2 + 10.2 + 1.2;
var metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value1"));
var metricSummary = metricSeries as DoubleSummaryData;
Assert.Equal(expectedSum, metricSummary.Sum);
Assert.Equal(3, metricSummary.Count);
Assert.Equal(1.2, metricSummary.Min);
Assert.Equal(100.2, metricSummary.Max);

expectedSum = 200.2 + 20.2;
metricSeries = metric.Data.Single(data => data.Labels.Any(l => l.Key == "dim1" && l.Value == "value2"));
metricSummary = metricSeries as DoubleSummaryData;
Assert.Equal(expectedSum, metricSummary.Sum);
Assert.Equal(2, metricSummary.Count);
Assert.Equal(20.2, metricSummary.Min);
Assert.Equal(200.2, metricSummary.Max);
}

[Fact]
public void LongObserverSendsAggregateToRegisteredProcessor()
{
Expand Down