From daa8eb25f5c3c5e2a3479b299bbc2e302e756a74 Mon Sep 17 00:00:00 2001 From: Steve Gordon Date: Mon, 10 Aug 2020 13:34:52 +0100 Subject: [PATCH 1/2] NameValueCollection.ToQueryString performance optimisation This commit optimises the internal `ToQueryString` extension method on the `NameValueCollection` type. It avoids some intermediate string allocations by using a zero-alloc buffer to format the final query string. - Adds a test for the extension method to validate existing behaviour is unaffected by the change of implementation. - Adds the `System.Memory` package to support `Span` usage across all target frameworks. This is registered conditionally for targets before NetStandard 2.1. I had some downgrade errors when using the latest package version, so I've aligned with the existing `System.Buffers` version for the time being. --- .../Elasticsearch.Net.csproj | 2 + .../NameValueCollectionExtensions.cs | 55 ++++++++++++++----- .../NameValueCollectionExtensionsTests.cs | 37 +++++++++++++ 3 files changed, 81 insertions(+), 13 deletions(-) create mode 100644 tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs diff --git a/src/Elasticsearch.Net/Elasticsearch.Net.csproj b/src/Elasticsearch.Net/Elasticsearch.Net.csproj index c32eb0ac7ab..9f8be8d970d 100644 --- a/src/Elasticsearch.Net/Elasticsearch.Net.csproj +++ b/src/Elasticsearch.Net/Elasticsearch.Net.csproj @@ -23,6 +23,8 @@ + + diff --git a/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs b/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs index 77f98dcb897..32a8b9cd3c8 100644 --- a/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs +++ b/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information using System; +using System.Buffers; using System.Collections.Generic; using System.Collections.Specialized; using System.Linq; @@ -12,28 +13,56 @@ namespace Elasticsearch.Net.Extensions { internal static class NameValueCollectionExtensions { + private const int MaxCharsOnStack = 256; // 512 bytes + internal static string ToQueryString(this NameValueCollection nv) { if (nv == null || nv.AllKeys.Length == 0) return string.Empty; - // initialize with capacity for number of key/values with length 5 each - var builder = new StringBuilder("?", nv.AllKeys.Length * 2 * 5); - for (var i = 0; i < nv.AllKeys.Length; i++) + var maxLength = 1 + nv.AllKeys.Length - 1; // account for '?', and any required '&' chars + foreach (var key in nv.AllKeys) { - if (i != 0) - builder.Append("&"); + maxLength += 1 + (key.Length + nv[key]?.Length ?? 0) * 3; // '=' char + worst case assume all key/value chars are escaped + } - var key = nv.AllKeys[i]; - builder.Append(Uri.EscapeDataString(key)); - var value = nv[key]; - if (!value.IsNullOrEmpty()) + // prefer stack allocated array for short lengths + // note: renting for larger lengths is slightly more efficient since no zeroing occurs + char[] rentedFromPool = null; + var buffer = maxLength > MaxCharsOnStack + ? rentedFromPool = ArrayPool.Shared.Rent(maxLength) + : stackalloc char[MaxCharsOnStack]; + + try + { + var position = 0; + buffer[position++] = '?'; + + foreach (var key in nv.AllKeys) { - builder.Append("="); - builder.Append(Uri.EscapeDataString(nv[key])); + if (position != 1) + buffer[position++] = '&'; + + var escapedKey = Uri.EscapeDataString(key); + escapedKey.AsSpan().CopyTo(buffer.Slice(position)); + position += escapedKey.Length; + + var value = nv[key]; + + if (value.IsNullOrEmpty()) continue; + + buffer[position++] = '='; + var escapedValue = Uri.EscapeDataString(value); + escapedValue.AsSpan().CopyTo(buffer.Slice(position)); + position += escapedValue.Length; } - } - return builder.ToString(); + return buffer.Slice(0, position).ToString(); + } + finally + { + if (rentedFromPool is object) + ArrayPool.Shared.Return(rentedFromPool, clearArray: false); + } } internal static void UpdateFromDictionary(this NameValueCollection queryString, Dictionary queryStringUpdates, diff --git a/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs b/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs new file mode 100644 index 00000000000..c69cc5d2649 --- /dev/null +++ b/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs @@ -0,0 +1,37 @@ +using System.Collections.Generic; +using System.Collections.Specialized; +using Elasticsearch.Net.Extensions; +using FluentAssertions; +using Xunit; + +namespace Tests.Extensions +{ + public class NameValueCollectionExtensionsTests + { + [Theory] + [MemberData(nameof(QueryStringTestData))] + public void ToQueryString_ReturnsExpectedString(NameValueCollection nvc, string expected) => nvc.ToQueryString().Should().Be(expected); + + public static IEnumerable QueryStringTestData => + new List + { + new object[] { new NameValueCollection + { + { "q", "title:\"The Right Way\" AND mod_date:[20020101 TO 20030101]" }, + { "from", "10000" }, + { "request_cache", bool.TrueString }, + { "size", "100" } + }, "?q=title%3A%22The%20Right%20Way%22%20AND%20mod_date%3A%5B20020101%20TO%2020030101%5D&from=10000&request_cache=True&size=100" }, + + new object[] { new NameValueCollection + { + { "q", "name:john~1 AND (age:[30 TO 40} OR surname:K*) AND -city" }, + }, "?q=name%3Ajohn~1%20AND%20%28age%3A%5B30%20TO%2040%7D%20OR%20surname%3AK%2A%29%20AND%20-city" }, + + new object[] { new NameValueCollection + { + { "q", null }, + }, "?q" } + }; + } +} From f95c007061166fab9ed031ac5fde1079ccaaec08 Mon Sep 17 00:00:00 2001 From: Steve Gordon Date: Thu, 13 Aug 2020 09:06:36 +0100 Subject: [PATCH 2/2] PR feedback and safer max length calculation. --- .../Extensions/NameValueCollectionExtensions.cs | 6 ++++-- .../Extensions/NameValueCollectionExtensionsTests.cs | 12 +++++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs b/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs index 32a8b9cd3c8..9900abe3efc 100644 --- a/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs +++ b/src/Elasticsearch.Net/Extensions/NameValueCollectionExtensions.cs @@ -22,7 +22,9 @@ internal static string ToQueryString(this NameValueCollection nv) var maxLength = 1 + nv.AllKeys.Length - 1; // account for '?', and any required '&' chars foreach (var key in nv.AllKeys) { - maxLength += 1 + (key.Length + nv[key]?.Length ?? 0) * 3; // '=' char + worst case assume all key/value chars are escaped + var bytes = Encoding.UTF8.GetByteCount(key) + Encoding.UTF8.GetByteCount(nv[key] ?? string.Empty); + var maxEncodedSize = bytes * 3; // worst case, assume all bytes are URL escaped to 3 chars + maxLength += 1 + maxEncodedSize; // '=' + encoded chars } // prefer stack allocated array for short lengths @@ -30,7 +32,7 @@ internal static string ToQueryString(this NameValueCollection nv) char[] rentedFromPool = null; var buffer = maxLength > MaxCharsOnStack ? rentedFromPool = ArrayPool.Shared.Rent(maxLength) - : stackalloc char[MaxCharsOnStack]; + : stackalloc char[maxLength]; try { diff --git a/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs b/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs index c69cc5d2649..b874382d07e 100644 --- a/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs +++ b/tests/Tests/Extensions/NameValueCollectionExtensionsTests.cs @@ -31,7 +31,17 @@ public class NameValueCollectionExtensionsTests new object[] { new NameValueCollection { { "q", null }, - }, "?q" } + }, "?q" }, + + new object[] { new NameValueCollection + { + { "emoji", "😅"} + }, "?emoji=%F0%9F%98%85" }, + + new object[] { new NameValueCollection + { + { "€", "€"} + }, "?%E2%82%AC=%E2%82%AC" } }; } }