Use ValueStringBuilder in HttpLoggingMiddleware

[Kahbazi]

No description provided.

[BrennanConroy]

Can you share perf numbers for this please

[davidfowl]

I would use ValueStringBuilder instead

[BrennanConroy]

I would use ValueStringBuilder instead

You keep mentioning that, but it's internal to Runtime 😆

[davidfowl]

Copy it and make it internal in ASP.NET Core.

[BrennanConroy]

ValueStringBuilder will be available in the repo once #32600 is merged

[davidfowl]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[BrennanConroy]

Can we come up with a rough guess of how big the string will be? Otherwise this will need to do a lot of internal copying and renting whenever it grows. For example, each _keyValues will have at minimum ~5 characters output + 8 for "Request:\n".

I'd love to see perf numbers from before and after, and with a guess at the size here so we can see that improvements are actually being made.

[Kahbazi]

What should I use to get the perf numbers? BenchmarkDotNet?

[BrennanConroy]

Yeah, take a look at https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/perf/Microbenchmarks/HandshakeBenchmark.cs for an example in the middleware area. You go to that folder and run dotnet run -c Release then select which benchmark to run if there are multiple to choose from.

[Kahbazi]

Here's the result with this list

list = new List<KeyValuePair<string, string>>();
list.Add(new KeyValuePair<string, string>("Protocol", "HTTP/1.1"));
list.Add(new KeyValuePair<string, string>("Method", "GET"));
list.Add(new KeyValuePair<string, string>("Scheme", "http"));
list.Add(new KeyValuePair<string, string>("PathBase", "/"));
list.Add(new KeyValuePair<string, string>("Path", "/"));
list.Add(new KeyValuePair<string, string>("QueryString", ""));
list.Add(new KeyValuePair<string, string>("Connection", "KeepAlive"));
list.Add(new KeyValuePair<string, string>("Content-Type", "text/plain"));

and the initial capacity is 8 + _keyValues.Count * 5.

Method	Mean	Error	StdDev	Median	Op/s	Gen 0	Gen 1	Gen 2	Allocated
StringBuilder	681.8 ns	13.62 ns	32.38 ns	669.2 ns	1,466,662.8	0.0448	-	-	1,208 B
ValueStringBuilder	674.5 ns	6.26 ns	5.22 ns	672.9 ns	1,482,670.6	0.0124	-	-	336 B
StringBuilderWithCapacity	530.5 ns	5.89 ns	4.60 ns	529.6 ns	1,884,897.5	0.0343	-	-	936 B
ValueStringBuilderWithCapacity	684.1 ns	4.40 ns	3.68 ns	683.1 ns	1,461,677.9	0.0124	-	-	336 B

[BrennanConroy]

That's a big difference between StringBuilderWithCapacity and ValueStringBuilderWithCapacity. Could you try initial capacity of 8 + _keyValues.Count * 100 for both? In the * 5 case, none of the names are going to be 1 byte, that was just the bare minimum, so a grow will happen which causes a copy and new rent.

[Kahbazi]

I don't think it is good enough. I had to make the HttpRequestLog public and also add a method which get the capacity as input. Also I had both StringBuilder and ValueStringBuilder in HttpRequestLog for the benchmark.

[BrennanConroy]

@Tratcher Thoughts on a good default capacity? We would prefer slightly over-estimating the total size of the headers.

[Tratcher]

All request headers on average? 4-16kb. Start at 2kb for the simple case though. Let the big requests pay the resize cost.

[Kahbazi]

@Tratcher What's the average on all response headers? Is 2kb good enough for response too?

[Tratcher]

That should be fine for now, there tend to be less response headers than request headers.

[BrennanConroy]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[BrennanConroy]

Thanks @Kahbazi !