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.

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

Modify MetricPoint to avoid copy #2320

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 16 additions & 11 deletions src/OpenTelemetry/Metrics/BatchMetricPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace OpenTelemetry.Metrics
public readonly struct BatchMetricPoint : IDisposable
{
private readonly MetricPoint[] metricsPoints;
private readonly long targetCount;
private readonly int targetCount;
private readonly DateTimeOffset start;
private readonly DateTimeOffset end;

Expand Down Expand Up @@ -56,13 +56,13 @@ public Enumerator GetEnumerator()
/// </summary>
public struct Enumerator : IEnumerator
{
private readonly MetricPoint[] metricsPoints;
private readonly Memory<MetricPoint> metricsPoints;
private readonly DateTimeOffset start;
private readonly DateTimeOffset end;
private long targetCount;
private long index;
private int targetCount;
private int index;

internal Enumerator(MetricPoint[] metricsPoints, long targetCount, DateTimeOffset start, DateTimeOffset end)
internal Enumerator(MetricPoint[] metricsPoints, int targetCount, DateTimeOffset start, DateTimeOffset end)
{
this.Current = default;
this.metricsPoints = metricsPoints;
Expand All @@ -72,7 +72,7 @@ internal Enumerator(MetricPoint[] metricsPoints, long targetCount, DateTimeOffse
this.end = end;
}

public MetricPoint Current { get; private set; }
public MetricPointPointer Current { get; private set; }

/// <inheritdoc/>
object IEnumerator.Current => this.Current;
Expand All @@ -84,17 +84,22 @@ public void Dispose()
/// <inheritdoc/>
public bool MoveNext()
{
var metricPoints = this.metricsPoints;
if (metricPoints[this.index].StartTime == default)
if (this.metricsPoints.Span[this.index].StartTime == default)
Copy link
Member

Choose a reason for hiding this comment

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

In this method we access this.metricsPoints.Span[this.index] three times I think we could do something like...

ref MetricPoint current = ref this.metricsPoints.Span[this.index];

...to capture a local ref to the underlying struct in the local scope.

{
this.index++;
}

if (this.index < this.targetCount)
{
metricPoints[this.index].StartTime = this.start;
metricPoints[this.index].EndTime = this.end;
this.Current = metricPoints[this.index];
this.metricsPoints.Span[this.index].StartTime = this.start;
this.metricsPoints.Span[this.index].EndTime = this.end;

// The following is going to copy the MetricPoint
// struct.
// this.Current = metricPoints[this.index];
// the following does not copy the whole struct
// but just pointer to the struct.
this.Current = new MetricPointPointer(this.metricsPoints.Slice(this.index, 1));
this.index++;
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry/Metrics/MetricPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

namespace OpenTelemetry.Metrics
{
public struct MetricPoint
internal struct MetricPoint
{
private long longVal;
private long lastLongSum;
Expand Down
101 changes: 101 additions & 0 deletions src/OpenTelemetry/Metrics/MetricPointPointer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// <copyright file="MetricPointPointer.cs" company="OpenTelemetry Authors">
// 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.
// </copyright>

using System;
using System.Threading;

namespace OpenTelemetry.Metrics
{
public readonly struct MetricPointPointer
{
private readonly Memory<MetricPoint> point;

internal MetricPointPointer(
Memory<MetricPoint> point)
{
if (point.Length != 1)
{
// TODO: throw
}

this.point = point;
}

public long LongValue
Copy link
Member Author

@cijothomas cijothomas Sep 8, 2021

Choose a reason for hiding this comment

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

For a future follow up PR:

The ctor() can take the MetricType, and then we can offer more convenient methods like

public long LongSum
{
 if (metricType==SUM) return this.point.Span[0].LongValue;
else throw.
}

{
get
{
return this.point.Span[0].LongValue;
}
}

public double DoubleValue
{
get
{
return this.point.Span[0].DoubleValue;
}
}

public string[] Keys
{
get
{
return this.point.Span[0].Keys;
}
}

public object[] Values
{
get
{
return this.point.Span[0].Values;
}
}

public long[] BucketCounts
{
get
{
return this.point.Span[0].BucketCounts;
}
}

public double[] ExplicitBounds
{
get
{
return this.point.Span[0].ExplicitBounds;
}
}

public DateTimeOffset StartTime
{
get
{
return this.point.Span[0].StartTime;
}
}

public DateTimeOffset EndTime
{
get
{
return this.point.Span[0].EndTime;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ void ProcessExport(IEnumerable<Metric> batch)
var metric = requestMetrics[0];
Assert.NotNull(metric);
Assert.True(metric.MetricType == MetricType.Histogram);
var metricPoints = new List<MetricPoint>();
var metricPoints = new List<MetricPointPointer>();
foreach (var p in metric.GetMetricPoints())
{
metricPoints.Add(p);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ void ProcessExport(IEnumerable<Metric> batch)
Assert.NotNull(metric);
Assert.True(metric.MetricType == MetricType.Histogram);

var metricPoints = new List<MetricPoint>();
var metricPoints = new List<MetricPointPointer>();
foreach (var p in metric.GetMetricPoints())
{
metricPoints.Add(p);
Expand Down Expand Up @@ -188,7 +188,7 @@ void ProcessExport(IEnumerable<Metric> batch)
else
{
Assert.Single(requestMetrics);
var metricPoints = new List<MetricPoint>();
var metricPoints = new List<MetricPointPointer>();
foreach (var p in requestMetrics[0].GetMetricPoints())
{
metricPoints.Add(p);
Expand Down