NameValueCollectionExtensions Performance Optimisation

[stevejgordon]

Would you be open to taking PR(s) for performance optimisations, specifically for cases where the NEST library is consumed within NetStandard 2.1 and NetCoreApp 2.1 apps?

I was exploring NameValueCollectionExtensions and one simple example is the ToQueryString method. By applying Span<T> optimisation, it's possible to reduce the allocations fairly significantly. Given this is called every time a RequestData instance is constructed it may have worthwhile affect for consumers.

I've created a benchmark and an initial optimised implementation with the following results:

|   Method |     Mean |     Error |    StdDev | Ratio | RatioSD |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |---------:|----------:|----------:|------:|--------:|-------:|------:|------:|----------:|
| Original | 4.593 us | 0.1413 us | 0.4099 us |  1.00 |    0.00 | 0.2060 |     - |     - |     896 B |
|      New | 2.685 us | 0.1360 us | 0.3859 us |  0.93 |    0.12 | 0.1221 |     - |     - |     544 B |

This reduced allocated bytes by 39.3% and operates 42% quicker with my test data of 5 random keys and values.

A note regarding the execution time. My prototype takes a shortcut and uses a rented char[] from the ArrayPool with a minimum length of 512 chars. If there is a realistic maximum length expected for the qs, then hard-coding it rather than calculating the length each time is potentially worthwhile. With the length calculation the execution time is 4.227 us, so only slightly faster than current performance.

With my test data (where one key has a char that requires escaping), by applying custom Uri escaping, it may be possible to reduce a further 48 bytes. If it is common for keys/values to require escaping then the allocation reduction could be more significant.

If you feel such allocation reductions would have value to consumers and you'd be open to such a PR, I'd happily submit this?

Example of prototype code...

internal static string ToQueryStringNew(this NameValueCollection nv)
{
    if (nv == null || nv.AllKeys.Length == 0) return string.Empty;

    var length = 1 + nv.AllKeys.Length - 1; // acct. for '?', and '&' chars
    foreach (var key in nv.AllKeys)
    {
        length += (key.Length * 3) + (nv[key].Length * 3) + 1; // worst case all escaped chars + '=' 
    }

    // possible to reduce execution time ~50% by hard-coding to 512 chars
    var buffer = ArrayPool<char>.Shared.Rent(length); 
    var bufferSpan = buffer.AsSpan();

    try
    {
        var position = 0;
        bufferSpan[position++] = '?';

        foreach (var key in nv.AllKeys)
        {
            if (position != 1)
                bufferSpan[position++] = '&';

            var escapedKey = Uri.EscapeDataString(key);
            escapedKey.AsSpan().CopyTo(bufferSpan.Slice(position));
            position += escapedKey.Length;

            var value = nv[key];
            if (!value.IsNullOrEmpty())
            {
                bufferSpan[position++] = '=';
                var escaped = Uri.EscapeDataString(value);                
                escaped.AsSpan().CopyTo(bufferSpan.Slice(position));
                position += escaped.Length;
            }
        }

        return new string(bufferSpan.Slice(0, position));
    }
    finally
    {
        ArrayPool<char>.Shared.Return(buffer);
    }
}

[russcam]

Thanks for opening @stevejgordon.

In general, performance optimizations are most definitely PRs that we welcome and encourage! If I understand correctly, to be able to benefit from this optimization for those folks running on a platform that supports .NET Standard 2.1, we would need to use a NETSTANDARD2_1 preprocessor directive and publish a .NET Standard 2.1 specific package. That's a small hurdle as we only publish .NET Standard 2.0 and .NET Framework 4.6.1 packages currently, though we have discussed internally about targeting newer TFMs to take advantage of better performing approaches.

I am open to accepting such a PR, and publishing a .NET Standard 2.1 package in addition. @Mpdreamz, what are your thoughts?

[stevejgordon]

You're welcome, @russcam.

Forgive me, I'd assumed there was a 2.1 package as I saw it listed in the TargetFrameworks for the project and some use of the NETSTANDARD2_1 preprocessor directive in the code base.

FYI, my original benchmark was a little skewed as I'd accidentally targeted .NET 5 so was getting some perf improvements from there too. I've updated the original issue with the app targeting 2.1 so that the benchmarks are more accurate.

NS2.1 what the first to include Span<T> and ArrayPool<T>. They can be referenced from NS2.0 compatible packages System.Memory and System.Buffers though. So it's possible to support the performance changes in 2.0 by conditionally adding those package references. System.Buffers already seems to be referenced in fact. The perf gains will be a little less. Technically, I think it's possible to consume those libraries in .NET Framework 4.6.1, however it's no longer recommended.

I'd advocate for a 2.1 package which includes the latest 2.1 optimisations for consumers on .NET Core 3.x and higher.

[Mpdreamz]

If you feel such allocation reductions would have value to consumers and you'd be open to such a PR, I'd happily submit this?

A ~40% speed and allocation improvement is always welcomed with open arms 👍

I am open to accepting such a PR, and publishing a .NET Standard 2.1 package in addition. @Mpdreamz, what are your thoughts?

Agreed, I would even open it up to netcoreappN.N if we feel we'd benefit from a new API. It's pretty clear now that there won't be any more netstandardN.N with the push for one .NET with .NET 5.

So it's possible to support the performance changes in 2.0 by conditionally adding those package references. System.Buffers already seems to be referenced in fact. The perf gains will be a little less.

@adamsitnik squashed any qualms I had about slow span vs fast span in the past: https://adamsitnik.com/Span/#slow-vs-fast-span. Our plan was always to start introducing the System.Memory nuget package when we first pulled in our fork of the Utf8Json serializer (see: #3493 (comment)). That got put on hold though since we agreed to swap out Utf8Json for Sytem.Text.Json.

Technically, I think it's possible to consume those libraries in .NET Framework 4.6.1, however it's no longer recommended.

Both of these libraries have explicit net45 targets which still takes precedence as fallback for full framework libs.

The System.Buffer package we depend on already technically and unintentionally pulls in a lower version on full framework due to this resolution see: dotnet/runtime#1830 which should be "fine" but is a support matrix nightmare 😹

Long story short, nothing prevents us from adding this reference AFAICT.