dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

Faster creating query string in 'Microsoft.AspNetCore.Http.Extensions.QueryBuilder' #42810

Closed antomys closed 2 years ago

antomys commented 2 years ago

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

This is not a problem, just a suggestion for improving QueryBuilder by using stack, arrayPool and span manipulations

Describe the solution you'd like

Suggesting to use stackalloc or ArrayPool while building query for improving performance. Also storing count of overall values.

QueryBuilder as is:

public override string ToString()
{
    var builder = new StringBuilder();
    bool first = true;
    for (int i = 0; i < _params.Count; i++)
    {
        var pair = _params[i];
        builder.Append(first ? "?" : "&");
        first = false;
        builder.Append(UrlEncoder.Default.Encode(pair.Key));
        builder.Append("=");
        builder.Append(UrlEncoder.Default.Encode(pair.Value));
    }

    return builder.ToString();
}

QueryBuilde could be:

public override string ToString()
{
    if (Count is 0)
    {
        return string.Empty;
    }

    var queryLength = Count * 2;

    var isStackAlloc = queryLength <= 64;
    var currentPosition = 0;

    var array = isStackAlloc ? null : ArrayPool<char>.Shared.Rent(queryLength);
    var resultSpan = isStackAlloc ? stackalloc char[queryLength] : array;

    try
    {
        var first = true;
        for (var i = 0; i < _valuePairs.Count; i++)
        {
            var pair = _valuePairs[i];
            resultSpan[currentPosition] = first ? '?' : '&';
            first = false;
            currentPosition++;

            var escapeKey = System.Uri.EscapeDataString(pair.Key);
            escapeKey.CopyTo(resultSpan[currentPosition..]);
            currentPosition += escapeKey.Length;

            resultSpan[currentPosition++] = '=';

            var escapedValue = System.Uri.EscapeDataString(pair.Value);
            escapedValue.CopyTo(resultSpan[currentPosition..]);
            currentPosition += escapedValue.Length;
        }

        var endIndex = resultSpan.IndexOf('\0');

        return endIndex is -1
            ? resultSpan[..currentPosition].ToString()
            : resultSpan[..(endIndex > currentPosition ? currentPosition : endIndex)].ToString();
    }
    finally
    {
        if (array is not null)
        {
            ArrayPool<char>.Shared.Return(array);
        }
    }
}

Additional context

Why so? Benefits:

  1. Improving performace >10% on any query building and eliminating creating new StringBuilder every time
  2. Supporting escaping, unlike Current solution (Controversial, because this could break something, can be removed)

Drawbacks:

  1. Adding new property Count that holds amount of entities inside of a List<KeyValuePair>.
  2. I think using Count += parameters.Sum(pair => pair.Key.Length + pair.Value.Length); in a constructor with a parameter of List<KeyValuePair> can be somewhat irrational.
  3. Everytime stackallocating or renting array, which length is 2 time larger that current (this is done because of Escaping some characters and additional values such as = and & or ?

As for benchmark results:

NOTE: QueryAspNetCore - current AspNet solution. QueryCustomBuilder - implementation with suggested solution

Method QueryCount Mean Error StdDev StdErr Min Q1 Median Q3 Max Op/s Gen 0 Allocated
QueryCustomBuilder 1 133.5 ns 2.44 ns 2.71 ns 0.62 ns 130.4 ns 131.5 ns 132.6 ns 134.6 ns 139.8 ns 7,492,384.5 0.0305 128 B
QueryAspNetCore 1 151.8 ns 2.13 ns 1.89 ns 0.50 ns 148.4 ns 150.7 ns 151.7 ns 152.4 ns 154.8 ns 6,589,520.4 0.0553 232 B
QueryCustomBuilder 2 187.1 ns 3.33 ns 2.95 ns 0.79 ns 182.4 ns 185.5 ns 187.3 ns 189.0 ns 192.0 ns 5,345,047.0 0.0420 176 B
QueryAspNetCore 2 257.4 ns 4.05 ns 3.59 ns 0.96 ns 252.6 ns 254.4 ns 257.6 ns 259.8 ns 263.8 ns 3,884,810.1 0.0918 384 B
QueryCustomBuilder 5 363.0 ns 5.83 ns 5.99 ns 1.45 ns 357.5 ns 359.3 ns 360.7 ns 364.7 ns 378.9 ns 2,754,534.8 0.0763 320 B
QueryAspNetCore 5 482.3 ns 9.05 ns 8.47 ns 2.19 ns 468.0 ns 476.3 ns 481.1 ns 486.3 ns 499.4 ns 2,073,507.6 0.1583 664 B
QueryCustomBuilder 10 682.7 ns 9.09 ns 8.50 ns 2.20 ns 668.8 ns 677.3 ns 681.7 ns 687.0 ns 701.2 ns 1,464,686.5 0.1335 560 B
QueryAspNetCore 10 852.4 ns 16.67 ns 14.78 ns 3.95 ns 837.1 ns 840.1 ns 847.4 ns 860.3 ns 883.4 ns 1,173,131.6 0.2632 1,104 B

This approach gives is faster by approx 13% for 1 query and for average 2 approx 18%. Speaking about memory, for 1 query we have 45% less used memory and for 2 - 55%

See plots: Query Benchmarks Benchmarks QueryBuilderBenchmarks-barplot Query Benchmarks Benchmarks QueryBuilderBenchmarks-boxplot

gfoidl commented 2 years ago

Instead of the StringBuilder it could be based on ValueStringBuilder. This does essentially the same as your proposal, but encapsulated within in a type, so the usage is more streamlined and virtually the same as now.

antomys commented 2 years ago

Yes, you are right, @gfoidl. It would be much better to use ValueStringBuilder. But as i was investigating new possibilities of improving QueryBuilder, I was unable to use ValueStringBuilder because of internal modifier. As they will be in same assembly, it would be better to use it and not add additional proposed by me overhead

halter73 commented 2 years ago

ValueStringBuilder can be used by adding it manually to the csproj from the shared source folder like we do here:

https://github.com/dotnet/aspnetcore/blob/81cb87e31c66785b64dbc78b01d4f31c80d7ac17/src/Middleware/HttpLogging/src/Microsoft.AspNetCore.HttpLogging.csproj#L22

antomys commented 2 years ago

Thanks @halter73 for hint! I will do a PR with a suggestion and link this issue to it