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

Jump to bottom

Improve GetUri #2947

Merged
merged 5 commits into from
Mar 9, 2022
Merged

Improve GetUri #2947

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa1f3f0
Improve GetUri
Tornhoof Feb 25, 2022
d045602
Push namespace usage down to get around IDE0005
Tornhoof Feb 25, 2022
bfabcd1
Merge branch 'main' into ImproveGetUri
reyang Feb 28, 2022
f0e8c1b
Merge branch 'main' into ImproveGetUri
cijothomas Mar 9, 2022
d54a24b
Add line to changelog
Tornhoof Mar 9, 2022
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
70 changes: 39 additions & 31 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
#if !NETSTANDARD2_0
using System.Runtime.CompilerServices;
#endif
using System.Text;
using Microsoft.AspNetCore.Http;
using OpenTelemetry.Context.Propagation;
using OpenTelemetry.Internal;
Expand Down Expand Up @@ -300,37 +299,46 @@ public override void OnException(Activity activity, object payload)

private static string GetUri(HttpRequest request)
{
var builder = new StringBuilder();

builder.Append(request.Scheme).Append("://");

if (request.Host.HasValue)
{
builder.Append(request.Host.Value);
}
else
// this follows the suggestions from https://github.com/dotnet/aspnetcore/issues/28906
var scheme = request.Scheme ?? string.Empty;

// HTTP 1.0 request with NO host header would result in empty Host.
// Use placeholder to avoid incorrect URL like "http:///"
var host = request.Host.Value ?? UnknownHostName;
var pathBase = request.PathBase.Value ?? string.Empty;
var path = request.Path.Value ?? string.Empty;
var queryString = request.QueryString.Value ?? string.Empty;
var length = scheme.Length + Uri.SchemeDelimiter.Length + host.Length + pathBase.Length
+ path.Length + queryString.Length;
#if NETSTANDARD2_1
return string.Create(length, (scheme, host, pathBase, path, queryString), (span, parts) =>
Copy link
Member

@CodeBlanch CodeBlanch Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would hoist the delegate to a static field. Eg:

#if NETSTANDARD2_1
		private readonly static SpanAction<char, (string scheme, string host, string pathBase, string path, string queryString)> s_CreateStringFunc = (span, state) =>
		{
                CopyTo(ref span, parts.scheme);
                CopyTo(ref span, Uri.SchemeDelimiter);
                CopyTo(ref span, parts.host);
                CopyTo(ref span, parts.pathBase);
                CopyTo(ref span, parts.path);
                CopyTo(ref span, parts.queryString);

                static void CopyTo(ref Span<char> buffer, ReadOnlySpan<char> text)
                {
                    if (!text.IsEmpty)
                    {
                        text.CopyTo(buffer);
                        buffer = buffer.Slice(text.Length);
                    }
                }
		};
#endif

Invoke like this:

return string.Create(length, (scheme, host, pathBase, path, queryString), s_CreateStringFunc);

Why do that?

As it is written we are relying on the compiler to do the right thing and not allocate a delegate for each invocation. Sometimes it does the right thing, sometimes it does not. It is a bit mysterious to me as to the reasons for that. But even if it does decide to do the right thing, the emitted IL will be to check if it has created a static, and if not, create the static, and then do the invocation. So it is a bit better perf to just make the static initially and avoid the check on each call.

Copy link
Contributor Author

@Tornhoof Tornhoof Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Mysterious?, it depends on capturing variables, that's why you can now have static expressions and static local methods etc. You're correct that the current code will do a null check against the static method and initiate if required.

I still think it's a bad idea, because the static field would be located (if I follow the other source codes with static fields, including this type) up above the constructor, between other fields:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Lines 44 to 46 in aa1f3f0

private const string UnknownHostName = "UNKNOWN-HOST";
private static readonly Func<HttpRequest, string, IEnumerable<string>> HttpRequestHeaderValuesGetter = (request, name) => request.Headers[name];
private readonly PropertyFetcher<HttpContext> startContextFetcher = new("HttpContext");

And that reduces the readability of the class way too much.

As for performance:
The null check could be elided by Tier2 compilation with static/dynamic pgo, I don't know if that happens here though. The performance difference is not high, the noise in my benchmark runs is higher (I've seen everything between 1ns to 3ns difference):

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 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
Method Mean Error StdDev Gen 0 Allocated
StaticLocalMethod 26.71 ns 0.123 ns 0.115 ns 0.0043 72 B
StringBuilder 53.75 ns 0.425 ns 0.355 ns 0.0167 280 B
StringBuilderFixedLength 35.88 ns 0.311 ns 0.290 ns 0.0114 192 B
StaticFunctor 25.97 ns 0.071 ns 0.066 ns 0.0043 72 B 
// See https://aka.ms/new-console-template for more information

using System.Text;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<StringCreate>();

[MemoryDiagnoser()]
public class StringCreate
{

	[Benchmark]
	public string StaticLocalMethod()
	{
		var scheme = "http";
		var host = "localhost";
		var pathBase = string.Empty;
		var path = "/api/foo";
		var queryString = string.Empty;
		var length = scheme.Length + Uri.SchemeDelimiter.Length + host.Length + pathBase.Length
		             + path.Length + queryString.Length;
		return string.Create(length, (scheme, host, pathBase, path, queryString), (span, parts) =>
		{
			CopyTo(ref span, parts.scheme);
			CopyTo(ref span, Uri.SchemeDelimiter);
			CopyTo(ref span, parts.host);
			CopyTo(ref span, parts.pathBase);
			CopyTo(ref span, parts.path);
			CopyTo(ref span, parts.queryString);

			static void CopyTo(ref Span<char> buffer, ReadOnlySpan<char> text)
			{
				if (!text.IsEmpty)
				{
					text.CopyTo(buffer);
					buffer = buffer.Slice(text.Length);
				}
			}
		});
    }

	private static readonly System.Buffers.SpanAction<char, (string scheme, string host, string pathBase, string path, string queryString)> s_CreateStringFunc = (span, parts) =>
	{
		CopyTo(ref span, parts.scheme);
		CopyTo(ref span, Uri.SchemeDelimiter);
		CopyTo(ref span, parts.host);
		CopyTo(ref span, parts.pathBase);
		CopyTo(ref span, parts.path);
		CopyTo(ref span, parts.queryString);

		static void CopyTo(ref Span<char> buffer, ReadOnlySpan<char> text)
		{
			if (!text.IsEmpty)
			{
				text.CopyTo(buffer);
				buffer = buffer.Slice(text.Length);
			}
		}
	};

	[Benchmark]
	public string StringBuilder()
	{
		var scheme = "http";
		var host = "localhost";
		var pathBase = string.Empty;
		var path = "/api/foo";
		var queryString = string.Empty;
		return new StringBuilder()
			.Append(scheme)
			.Append(Uri.SchemeDelimiter)
			.Append(host)
			.Append(pathBase)
			.Append(path)
			.Append(queryString)
			.ToString();
	}

	[Benchmark]
	public string StringBuilderFixedLength()
	{
		var scheme = "http";
		var host = "localhost";
		var pathBase = string.Empty;
		var path = "/api/foo";
		var queryString = string.Empty;
		var length = scheme.Length + Uri.SchemeDelimiter.Length + host.Length + pathBase.Length
		             + path.Length + queryString.Length;
		return new StringBuilder(length)
			.Append(scheme)
			.Append(Uri.SchemeDelimiter)
			.Append(host)
			.Append(pathBase)
			.Append(path)
			.Append(queryString)
			.ToString();
	}

	[Benchmark]
	public string StaticFunctor()
	{
		var scheme = "http";
		var host = "localhost";
		var pathBase = string.Empty;
		var path = "/api/foo";
		var queryString = string.Empty;
		var length = scheme.Length + Uri.SchemeDelimiter.Length + host.Length + pathBase.Length
		             + path.Length + queryString.Length;
		return string.Create(length, (scheme, host, pathBase, path, queryString), s_CreateStringFunc);
	}
}

Copy link
Member

Choose a reason for hiding this comment

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

Mysterious?, it depends on capturing variables,

A local function with captures is always going to allocate. The mysterious part I was referring to is sometimes it will allocate a delegate even for static local functions.

Don't believe me? Check this out...

        private static readonly Action<(string, int)> FieldCallback = ((string Name, int Id) state) => { };

        public static void CallDoSomethingNoAllocation()
        {
            DoSomething(FieldCallback, ("mystate", 0));
        }

        public static void CallDoSomethingAllocation()
        {
            DoSomething(LocalCallback, ("mystate", 0));

            static void LocalCallback((string Name, int Id) state)
            {
            }
        }

        public static void DoSomething<T>(Action<T> callback, T state)
        {
            callback(state);
        }

CallDoSomethingNoAllocation compiles to...

    IL_0000: ldsfld       class [System.Runtime]System.Action`1<valuetype [System.Runtime]System.ValueTuple`2<string, int32>> OpenTelemetry.Benchmarks.Program::FieldCallback
    IL_0005: ldstr        "mystate"
    IL_000a: ldc.i4.0
    IL_000b: newobj       instance void valuetype [System.Runtime]System.ValueTuple`2<string, int32>::.ctor(!0/*string*/, !1/*int32*/)
    IL_0010: call         void OpenTelemetry.Benchmarks.Program::DoSomething<valuetype [System.Runtime]System.ValueTuple`2<string, int32>>(class [System.Runtime]System.Action`1<!!0/*valuetype [System.Runtime]System.ValueTuple`2<string, int32>*/>, !!0/*valuetype [System.Runtime]System.ValueTuple`2<string, int32>*/)

CallDoSomethingAllocation compiles to...

    IL_0000: ldnull
    IL_0001: ldftn        void OpenTelemetry.Benchmarks.Program::'<CallDoSomethingAllocation>g__LocalCallback|2_0'(valuetype [System.Runtime]System.ValueTuple`2<string, int32>)
    IL_0007: newobj       instance void class [System.Runtime]System.Action`1<valuetype [System.Runtime]System.ValueTuple`2<string, int32>>::.ctor(object, native int)
    IL_000c: ldstr        "mystate"
    IL_0011: ldc.i4.0
    IL_0012: newobj       instance void valuetype [System.Runtime]System.ValueTuple`2<string, int32>::.ctor(!0/*string*/, !1/*int32*/)
    IL_0017: call         void OpenTelemetry.Benchmarks.Program::DoSomething<valuetype [System.Runtime]System.ValueTuple`2<string, int32>>(class [System.Runtime]System.Action`1<!!0/*valuetype [System.Runtime]System.ValueTuple`2<string, int32>*/>, !!0/*valuetype [System.Runtime]System.ValueTuple`2<string, int32>*/)

Why did CallDoSomethingAllocation decide to allocate a delegate? Who knows! That's the mystery I'm referring to. Something to do with the generic, I think.

Anyway from what I have seen the pattern I'm recommending always works to bypass allocations and has a speed benefit so for me the tradeoff in readability is worth it 😄

Copy link
Contributor Author

@Tornhoof Tornhoof Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If I enter your example in https://sharplab.io/#v2:EYLgtghglgdgNAFxFANgHwAICYCMBYAKAwGYACbUgYVIG9DSHT7GWWAHAJygDcIEBTcjgBspDvwgATAPYwUATyFYAPAAoMOAAxxSsBAEoAfKQBiUfikmUIKFMAgBjANakAvKVXqtpAHIQw/Dp6pACSkvqkAM4IfPwRrsY0pAC+ANzMrORkGqIYACxUNigAItIAytIBCAAWsADmPtIAgrbSDnxQsqr6Gax0BJmZpRVVtTB1qmYWVkX2zjqqAERg8tGxizqa+vrpA4PJvSwkQrkF1rbDlfw19S0obR1dPXuZ/YOsl6P1qgAyDyjnOyOJwLZarGICDakLY7Q6DHLkAp/doA2bAzwaTS+fyBXQwBChcJRCFxOGvMmsA4vSkU44I/KkT7XMZ1ZQAFUMXhUHNIKKB81IbOJsWe7ze70YfLmTlUawEsOpLCpVMIQA==

Both do ldsfld. So depending on where you've seen that code, it might be some bug or unoptimized code.
Apparently the old netfx does it your way.

Copy link
Member

Choose a reason for hiding this comment

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

What I sent is compiled/optimized with .NET 6.

Not familiar with sharplab. Looks like your link is using a newer roslyn? Cool to see they are making improvements to this! Here is a link that shows the same:

https://sharplab.io/#v2:C4LglgNgPgAgTARgLACgYGYAE9MGFMDeqmJmxpFFADgE5gBuAhsAKbYIBsmNLjAJgHsAdhACe7OAB4AFDAQAGADSYwQ4AEoAfJgBiYFhD65GECACNGAYwDWmALyZpshZgByjALYtlq4JgCSfOqYAM7AzCzBdtoEmAC+ANzklNhYclwwACx4JhAAIgIAygJewAAWqgDmrgIAgqYClsxgwtLqyZREKCkpBcWlFUKV0noGRrkWNsrSAEQeomERM8ry6upJ3T1xHRQY7BnZxqZ9JSzlVfUQjc2t7ZspXT2UJwNV0gAy1xBH5lbW03MFuFWMtMKt1jseulsNlPk1vhM/k45PI3J5vCo1AEgqFgZFIQ8CZRtvdiUS9tCspgXmdBpVJAAVTTOKRMzDw35TTAM3ERO5PR5PUgcybWaSLVgQ0kUEkk1BAA===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The newer Version includes dotnet/roslyn#58288 which is what you're missing in the other type.

Copy link
Member

Choose a reason for hiding this comment

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

@Tornhoof Thanks for the link. You are kind of making my point for me 😄 It is a magical thing that requires (currently) experimental compiler features to work sometimes (there are limitations on that PR). I prefer to use the fail-safe pattern (which also has the nice benefit of being faster).

Copy link
Contributor Author

@Tornhoof Tornhoof Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In this case your problem does not apply, my link just shows that the behavior you're seeing is fixed for C#11 for the work on caching delegates for method group conversions, because that's what your code does.
If you do it without method group call conversion, e.g. DoSomething(q => LocalCallback(q), ("mystate", 0)); it will generate the ldsfld correctly even in old compiler versions (including netfx).
My link was just to tell you that you can skip doing the static functor dance in C#11 and use method groups without them to your heart's content.

In case of the span code, it never did that as it is not a method group conversion call, but a proper functor call with arguments.

As I've shown that the performance difference is negligable, the only remaining point is readability. And I highly value readability of a class over a minor perf enhancement.

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

To unblock, it'll propose to merge this now, and create a new issue tracking further improvements/discussions on this.

This is an internal implementation detail, and involves no public API change, so we can change it anytime.

{
// HTTP 1.0 request with NO host header would result in empty Host.
// Use placeholder to avoid incorrect URL like "http:///"
builder.Append(UnknownHostName);
}

if (request.PathBase.HasValue)
{
builder.Append(request.PathBase.Value);
}

if (request.Path.HasValue)
{
builder.Append(request.Path.Value);
}

if (request.QueryString.HasValue)
{
builder.Append(request.QueryString);
}

return builder.ToString();
CopyTo(ref span, parts.scheme);
CopyTo(ref span, Uri.SchemeDelimiter);
CopyTo(ref span, parts.host);
CopyTo(ref span, parts.pathBase);
CopyTo(ref span, parts.path);
CopyTo(ref span, parts.queryString);

static void CopyTo(ref Span<char> buffer, ReadOnlySpan<char> text)
{
if (!text.IsEmpty)
{
text.CopyTo(buffer);
buffer = buffer.Slice(text.Length);
}
}
});
#else
return new System.Text.StringBuilder(length)
.Append(scheme)
.Append(Uri.SchemeDelimiter)
.Append(host)
.Append(pathBase)
.Append(path)
.Append(queryString)
.ToString();
#endif
}

#if !NETSTANDARD2_0
Expand Down
19 changes: 13 additions & 6 deletions ....Instrumentation.AspNetCore.Tests/IncomingRequestsCollectionsIsAccordingToTheSpecTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor
}

[Theory]
[InlineData("/api/values", "user-agent", 503, "503")]
[InlineData("/api/values", null, 503, null)]
[InlineData("/api/exception", null, 503, null)]
[InlineData("/api/exception", null, 503, null, true)]
[InlineData("/api/values", null, "user-agent", 503, "503")]
[InlineData("/api/values", "?query=1", null, 503, null)]
[InlineData("/api/exception", null, null, 503, null)]
[InlineData("/api/exception", null, null, 503, null, true)]
public async Task SuccessfulTemplateControllerCallGeneratesASpan(
string urlPath,
string query,
string userAgent,
int statusCode,
string reasonPhrase,
Expand All @@ -81,7 +82,13 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor
}

// Act
var response = await client.GetAsync(urlPath);
var path = urlPath;
if (query != null)
{
path += query;
}

var response = await client.GetAsync(path);
}
catch (Exception)
{
Expand Down Expand Up @@ -109,7 +116,7 @@ public IncomingRequestsCollectionsIsAccordingToTheSpecTests(WebApplicationFactor
Assert.Equal("localhost", activity.GetTagValue(SemanticConventions.AttributeHttpHost));
Assert.Equal("GET", activity.GetTagValue(SemanticConventions.AttributeHttpMethod));
Assert.Equal(urlPath, activity.GetTagValue(SemanticConventions.AttributeHttpTarget));
Assert.Equal($"http://localhost{urlPath}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl));
Assert.Equal($"http://localhost{urlPath}{query}", activity.GetTagValue(SemanticConventions.AttributeHttpUrl));
Assert.Equal(statusCode, activity.GetTagValue(SemanticConventions.AttributeHttpStatusCode));

if (statusCode == 503)
Expand Down