Optimize QueryStringEnumerable.Decode()
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 290
Description
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Is your feature request related to a problem? Please describe the problem.
QueryStringEnumerable.Decode() contains a test to avoid allocating if decoding is not required:
https://github.com/dotnet/aspnetcore/blob/474d2944dba601440987b328d7044155023c3c14/src/Shared/QueryStringEnumerable.cs#L95
It only runs this shortcut if the input string is short, c.f. the comment
```
// If the value is short, it's cheap to check up front if it really needs decoding. If it doesn't,
// then we can save some allocations.
```
However, with net8.0 and `SearchValues` the penalty for doing the comparison is ~15% for the worst case of a 2k-chars component with an encoded character at the end, negligible for small character length, and it significantly benefits unencoded strings - more so as the string length increases, because you avoid allocations.
```
| Method | SourceString | Mean | Error | StdDev | Ratio | RatioSD | Gen0 | Allocated | Alloc Ratio |
|------------------ |--------------------- |-----------:|----------:|----------:|------:|--------:|-------:|----------:|------------:|
| BenchmarkExisting | abc | 8.694 ns | 0.0694 ns | 0.0649 ns | 1.00 | 0.00 | - | - | NA |
| BenchmarkNew | abc | 8.969 ns | 0.0641 ns | 0.0569 ns | 1.03 | 0.01 | - | - | NA |
| | | | | | | | | | |
| BenchmarkExisting | abc%21 | 24.384 ns | 0.1432 ns | 0.1339 ns | 1.00 | 0.00 | 0.0057 | 72 B | 1.00 |
| BenchmarkNew | abc%21 | 24.682 ns | 0.1255 ns | 0.1112 ns | 1.01 | 0.01 | 0.0057 | 72 B | 1.00 |
| | | | | | | | | | |
| BenchmarkExisting | abcd(...)wxyz [1997] | 209.647 ns | 3.9865 ns | 3.5339 ns | 1.00 | 0.00 | 0.3200 | 4016 B | 1.00 |
| BenchmarkNew | abcd(...)wxyz [1997] | 48.714 ns | 0.1828 ns | 0.1710 ns | 0.23 | 0.00 | - | - | 0.00 |
| | | | | | | | | | |
| BenchmarkExisting | abcd(...)z%21 [2000] | 395.953 ns | 6.2142 ns | 5.8128 ns | 1.00 | 0.00 | 0.6413 | 8048 B | 1.00 |
| BenchmarkNew | abcd(...)z%21 [2000] | 449.338 ns | 8.6114 ns | 9.2141 ns | 1.14 | 0.03 | 0.6413 | 8048 B | 1.00 |
| | | | | | | | | | |
| BenchmarkExisting | abcd(...)wxyz [130] | 28.758 ns | 0.3092 ns | 0.2741 ns | 1.00 | 0.00 | 0.0229 | 288 B | 1.00 |
| BenchmarkNew | abcd(...)wxyz [130] | 12.959 ns | 0.0384 ns | 0.0340 ns | 0.45 | 0.00 | - | - | 0.00 |
| | | | | | | | | | |
| BenchmarkExisting | abcd(...)z%21 [133] | 51.948 ns | 0.1715 ns | 0.1604 ns | 1.00 | 0.00 | 0.0459 | 576 B | 1.00 |
| BenchmarkNew | abcd(...)z%21 [133] | 55.890 ns | 0.3071 ns | 0.2397 ns | 1.08 | 0.01 | 0.0459 | 576 B | 1.00 |
```
### Describe the solution you'd like
The "new" code under test is:
```csharp
private static readonly SearchValues EncodedValues = SearchValues.Create("%+");
private static unsafe ReadOnlyMemory DecodeNew(ReadOnlyMemory chars)
{
// With searchvalues, it is quick to check on almost any length of string.
if (!chars.Span.ContainsAny(EncodedValues))
{
return chars;
}
#pragma warning disable CS8500 // This takes the address of, gets the size of, or declares a pointer to a managed type
ReadOnlySpan span = chars.Span;
return Uri.UnescapeDataString(
string.Create(span.Length,
(IntPtr)(&span), static (dest, ptr) => ((ReadOnlySpan*)ptr)->Replace(dest, '+', ' '))).AsMemory();
#pragma warning restore CS8500
}
```
While a "length" check could still be made if the trade off at 14% is too much, I would prefer to avoid the allocations.
### Additional context
_No response_
Contributor guide
Assessment
This issue has not been assessed yet.