dotnet / dotnet/aspnetcore

Optimize QueryStringEnumerable.Decode()

Open
#52,487 10 comments 0 reactions 1 assignee Claimed by @BrennanConroy View on GitHub
area-networking
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.