-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Reduce allocations in QueryCollection #31594
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.
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
Changes from all commits
793b523
ec818d0
18d4aff
11c3afa
8012a72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| // Copyright (c) .NET Foundation. All rights reserved. | ||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using BenchmarkDotNet.Attributes; | ||
| using Microsoft.AspNetCore.Http.Features; | ||
| using Microsoft.AspNetCore.WebUtilities; | ||
| using Microsoft.Extensions.Primitives; | ||
| using static Microsoft.AspNetCore.Http.Features.QueryFeature; | ||
|
|
||
| namespace Microsoft.AspNetCore.Http | ||
| { | ||
| public class QueryCollectionBenchmarks | ||
| { | ||
| private string _queryString; | ||
| private string _singleValue; | ||
|
|
||
| [IterationSetup] | ||
| public void Setup() | ||
| { | ||
| _queryString = "?key1=value1&key2=value2&key3=value3&key4=&key5="; | ||
| _singleValue = "?key1=value1"; | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public void ParseNew() | ||
| { | ||
| var dict = QueryFeature.ParseNullableQueryInternal(_queryString); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Return the value (to prevent any possible optimization that discards the result)? |
||
| } | ||
|
|
||
| [Benchmark] | ||
| public void ParseNewSingle() | ||
| { | ||
| var dict = QueryFeature.ParseNullableQueryInternal(_singleValue); | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public void Parse() | ||
| { | ||
| var dict = QueryHelpers.ParseNullableQuery(_queryString); | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public void Constructor() | ||
| { | ||
| var dict = new KvpAccumulator(); | ||
| if (dict.HasValues) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,10 @@ | |||||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||||||
|
|
||||||
| using System; | ||||||
| using System.Collections.Generic; | ||||||
| using Microsoft.AspNetCore.Internal; | ||||||
| using Microsoft.AspNetCore.WebUtilities; | ||||||
| using Microsoft.Extensions.Primitives; | ||||||
|
|
||||||
| namespace Microsoft.AspNetCore.Http.Features | ||||||
| { | ||||||
|
|
@@ -69,15 +72,15 @@ public IQueryCollection Query | |||||
| { | ||||||
| _original = current; | ||||||
|
|
||||||
| var result = QueryHelpers.ParseNullableQuery(current); | ||||||
| var result = ParseNullableQueryInternal(current); | ||||||
|
|
||||||
| if (result == null) | ||||||
| { | ||||||
| _parsedValues = QueryCollection.Empty; | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| _parsedValues = new QueryCollection(result); | ||||||
| _parsedValues = new QueryCollectionInternal(result); | ||||||
| } | ||||||
| } | ||||||
| return _parsedValues; | ||||||
|
|
@@ -100,5 +103,170 @@ public IQueryCollection Query | |||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Parse a query string into its component key and value parts. | ||||||
| /// </summary> | ||||||
| /// <param name="queryString">The raw query string value, with or without the leading '?'.</param> | ||||||
| /// <returns>A collection of parsed keys and values, null if there are no entries.</returns> | ||||||
| internal static AdaptiveCapacityDictionary<string, StringValues>? ParseNullableQueryInternal(string? queryString) | ||||||
| { | ||||||
| var accumulator = new KvpAccumulator(); | ||||||
|
|
||||||
| if (string.IsNullOrEmpty(queryString) || queryString == "?") | ||||||
| { | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| int scanIndex = 0; | ||||||
| if (queryString[0] == '?') | ||||||
| { | ||||||
| scanIndex = 1; | ||||||
| } | ||||||
|
|
||||||
| int textLength = queryString.Length; | ||||||
| int equalIndex = queryString.IndexOf('='); | ||||||
| if (equalIndex == -1) | ||||||
| { | ||||||
| equalIndex = textLength; | ||||||
| } | ||||||
| while (scanIndex < textLength) | ||||||
| { | ||||||
| int delimiterIndex = queryString.IndexOf('&', scanIndex); | ||||||
| if (delimiterIndex == -1) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is really a micro-optimization, as the this produces But you don't need to take it here, more or less FYI. |
||||||
| { | ||||||
| delimiterIndex = textLength; | ||||||
| } | ||||||
| if (equalIndex < delimiterIndex) | ||||||
| { | ||||||
| while (scanIndex != equalIndex && char.IsWhiteSpace(queryString[scanIndex])) | ||||||
| { | ||||||
| ++scanIndex; | ||||||
| } | ||||||
| string name = queryString.Substring(scanIndex, equalIndex - scanIndex); | ||||||
| string value = queryString.Substring(equalIndex + 1, delimiterIndex - equalIndex - 1); | ||||||
| accumulator.Append( | ||||||
| Uri.UnescapeDataString(name.Replace('+', ' ')), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can these allocations for the intermediate string be avoided? Ideally Uri.UnescapeDataString should have an overload for {RO}Span too, so some intermeidate allocations can be avoided. So should the queryString be copied to a Span-buffer on which is operated? Helpers codeTo replace internal static class MySpanExtensions
{
public static void ReplaceInPlace(this Span<char> span, char oldChar, char newChar)
{
foreach (ref char c in span)
{
if (c == oldChar)
{
c = newChar;
}
}
}
}Or with vectorization (x86 only): Use methode internal static class MySpanExtensions
{
public static void ReplacePlusWithSpaceInPlace(this Span<char> span)
=> ReplaceInPlace(span, '+', ' ');
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static unsafe void ReplaceInPlace(this Span<char> span, char oldChar, char newChar)
{
nint i = 0;
nint n = (nint)(uint)span.Length;
fixed (char* ptr = span)
{
ushort* pVec = (ushort*)ptr;
if (Sse41.IsSupported && n >= Vector128<ushort>.Count)
{
Vector128<ushort> vecOldChar = Vector128.Create((ushort)oldChar);
Vector128<ushort> vecNewChar = Vector128.Create((ushort)newChar);
do
{
Vector128<ushort> vec = Sse2.LoadVector128(pVec + i);
Vector128<ushort> mask = Sse2.CompareEqual(vec, vecOldChar);
Vector128<ushort> res = Sse41.BlendVariable(vec, vecNewChar, mask);
Sse2.Store(pVec + i, res);
i += Vector128<ushort>.Count;
} while (i <= n - Vector128<ushort>.Count);
}
for (; i < n; ++i)
{
if (ptr[i] == oldChar)
{
ptr[i] = newChar;
}
}
}
}
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I gave this appraoch a try. So it's a bit faster, but most important when there are any benchmark codeusing BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using Microsoft.AspNetCore.Http.Features;
namespace Microsoft.AspNetCore.Http
{
[MemoryDiagnoser]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]
[CategoriesColumn]
public class QueryCollectionBenchmarks
{
private string _queryString = "?key1=value1&key2=value2&key3=value3&key4=&key5=";
private string _singleValue = "?key1=value1";
private string _queryStringWithPlus = "?key1=value1+value2&key2=a+b+c+d&message=hello+asp+world";
[Benchmark(Baseline = true, Description = "Default")]
[BenchmarkCategory("QueryString")]
public object? ParseNew_Default()
{
return QueryFeature_Default.ParseNullableQueryInternal(_queryString);
}
[Benchmark(Baseline = true, Description = "Default")]
[BenchmarkCategory("SingleValue")]
public object? ParseNewSingle_Default()
{
return QueryFeature_Default.ParseNullableQueryInternal(_singleValue);
}
[Benchmark(Baseline = true, Description = "Default")]
[BenchmarkCategory("WithPlus")]
public object? ParseNewPlus_Default()
{
return QueryFeature_Default.ParseNullableQueryInternal(_queryStringWithPlus);
}
[Benchmark(Description = "Span-based")]
[BenchmarkCategory("QueryString")]
public object? ParseNew_1()
{
return QueryFeature_1.ParseNullableQueryInternal(_queryString);
}
[Benchmark(Description = "Span-based")]
[BenchmarkCategory("SingleValue")]
public object? ParseNewSingle_1()
{
return QueryFeature_1.ParseNullableQueryInternal(_singleValue);
}
[Benchmark(Description = "Span-based")]
[BenchmarkCategory("WithPlus")]
public object? ParseNewPlus_1()
{
return QueryFeature_1.ParseNullableQueryInternal(_queryStringWithPlus);
}
}
}In the span-based version gfoidl@e1d3c95 I've use the vectorized approach in MySpanExtensions (a bad name I know). Vectorization kicks in if a segment (name or value) is >= 8 chars. You can merge that comment and adjust some things to fit to the repo here (naming, placement of the extension method, etc.). |
||||||
| Uri.UnescapeDataString(value.Replace('+', ' '))); | ||||||
| equalIndex = queryString.IndexOf('=', delimiterIndex); | ||||||
| if (equalIndex == -1) | ||||||
| { | ||||||
| equalIndex = textLength; | ||||||
| } | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| if (delimiterIndex > scanIndex) | ||||||
| { | ||||||
| accumulator.Append(queryString.Substring(scanIndex, delimiterIndex - scanIndex), string.Empty); | ||||||
| } | ||||||
| } | ||||||
| scanIndex = delimiterIndex + 1; | ||||||
| } | ||||||
|
|
||||||
| if (!accumulator.HasValues) | ||||||
| { | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| return accumulator.GetResults(); | ||||||
| } | ||||||
|
|
||||||
| internal struct KvpAccumulator | ||||||
| { | ||||||
| /// <summary> | ||||||
| /// This API supports infrastructure and is not intended to be used | ||||||
| /// directly from your code. This API may change or be removed in future releases. | ||||||
| /// </summary> | ||||||
| private AdaptiveCapacityDictionary<string, StringValues> _accumulator; | ||||||
| private AdaptiveCapacityDictionary<string, List<string>> _expandingAccumulator; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// This API supports infrastructure and is not intended to be used | ||||||
| /// directly from your code. This API may change or be removed in future releases. | ||||||
| /// </summary> | ||||||
| public void Append(string key, string value) | ||||||
| { | ||||||
| if (_accumulator == null) | ||||||
| { | ||||||
| _accumulator = new AdaptiveCapacityDictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase); | ||||||
| } | ||||||
|
|
||||||
| StringValues values; | ||||||
| if (_accumulator.TryGetValue(key, out values)) | ||||||
| { | ||||||
| if (values.Count == 0) | ||||||
| { | ||||||
| // Marker entry for this key to indicate entry already in expanding list dictionary | ||||||
| _expandingAccumulator[key].Add(value); | ||||||
| } | ||||||
| else if (values.Count == 1) | ||||||
| { | ||||||
| _accumulator[key] = StringValues.Concat(values, value); | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| // Add zero count entry and move to data to expanding list dictionary | ||||||
| _accumulator[key] = default(StringValues); | ||||||
|
|
||||||
| if (_expandingAccumulator == null) | ||||||
| { | ||||||
| _expandingAccumulator = new AdaptiveCapacityDictionary<string, List<string>>(5, StringComparer.OrdinalIgnoreCase); | ||||||
| } | ||||||
|
|
||||||
| // Already 5 entries so use starting allocated as 10; then use List's expansion mechanism for more | ||||||
| var list = new List<string>(10); | ||||||
|
|
||||||
| list.AddRange(values); | ||||||
| list.Add(value); | ||||||
|
|
||||||
| _expandingAccumulator[key] = list; | ||||||
| } | ||||||
| } | ||||||
| else | ||||||
| { | ||||||
| // First value for this key | ||||||
| _accumulator[key] = new StringValues(value); | ||||||
| } | ||||||
|
|
||||||
| ValueCount++; | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// This API supports infrastructure and is not intended to be used | ||||||
| /// directly from your code. This API may change or be removed in future releases. | ||||||
| /// </summary> | ||||||
| public bool HasValues => ValueCount > 0; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// This API supports infrastructure and is not intended to be used | ||||||
| /// directly from your code. This API may change or be removed in future releases. | ||||||
| /// </summary> | ||||||
| public int KeyCount => _accumulator?.Count ?? 0; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// This API supports infrastructure and is not intended to be used | ||||||
| /// directly from your code. This API may change or be removed in future releases. | ||||||
| /// </summary> | ||||||
| public int ValueCount { get; private set; } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// This API supports infrastructure and is not intended to be used | ||||||
| /// directly from your code. This API may change or be removed in future releases. | ||||||
| /// </summary> | ||||||
| public AdaptiveCapacityDictionary<string, StringValues> GetResults() | ||||||
| { | ||||||
| if (_expandingAccumulator != null) | ||||||
| { | ||||||
| // Coalesce count 3+ multi-value entries into _accumulator dictionary | ||||||
| foreach (var entry in _expandingAccumulator) | ||||||
| { | ||||||
| _accumulator[entry.Key] = new StringValues(entry.Value.ToArray()); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return _accumulator ?? new AdaptiveCapacityDictionary<string, StringValues>(0, StringComparer.OrdinalIgnoreCase); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be assigned to the fields directly. Just don't make the fields const or static readonly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add a case where some
+are contained, to reflect the allocations for the string.Replace?