Removing unsafe parts of Decode method in QueryStringEnumerable #56630

ladeak · 2024-07-04T20:59:41Z

Removing unsafe parts of Decode method in QueryStringEnumerable

Description

Removing unsafe part and using string.Create passing in a ROS
Fixing IterationCount related data consistency issues in QueryCollectionBenchmarks
Executing QueryCollectionBenchmarks
Experimented avoiding the string.Create by stack allocating a buffer when the input is smaller than 256 chars and using the ROS overload of Uri.UnescapeDataString. While this reduced some allocations overall, it cost a performance penalty (ie. the Single case below is executing in the ~74 ns range instead of ~67ns) on my machine.

Performance

Just to confirm there is no perf degradation.

BEFORE:

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.22631
12th Gen Intel Core i7-1255U, 1 CPU, 12 logical and 10 physical cores
.NET SDK=9.0.100-preview.6.24316.4
[Host] : .NET 9.0.0 (9.0.24.32008), X64 RyuJIT
Job-PTTTFA : .NET 9.0.0 (9.0.24.30702), X64 RyuJIT

Server=True Toolchain=.NET Core 9.0 RunStrategy=Throughput

Measurement 1:

|   Method |     Categories |        Mean |     Error |    StdDev |                Op/s |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |--------------- |------------:|----------:|----------:|--------------------:|-------:|------:|------:|----------:|
| ParseNew |    QueryString | 267.7844 ns | 3.5603 ns | 3.3303 ns |         3,734,347.7 | 0.0052 |     - |     - |     512 B |
|          |                |             |           |           |                     |        |       |       |
| ParseNew |         Single |  65.8172 ns | 0.5523 ns | 0.4612 ns |        15,193,595.8 | 0.0032 |     - |     - |     304 B |
|          |                |             |           |           |                     |        |       |       |
| ParseNew | SingleWithPlus |  82.7875 ns | 0.4517 ns | 0.4004 ns |        12,079,119.8 | 0.0042 |     - |     - |     392 B |
|          |                |             |           |           |                     |        |       |       |
| ParseNew |        Encoded |  98.9658 ns | 0.3215 ns | 0.2850 ns |        10,104,503.6 | 0.0042 |     - |     - |     384 B |

Measurement 2:

|   Method |     Categories |        Mean |     Error |    StdDev |                Op/s |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |--------------- |------------:|----------:|----------:|--------------------:|-------:|------:|------:|----------:|
| ParseNew |    QueryString | 275.0711 ns | 3.5305 ns | 3.3025 ns |         3,635,423.8 | 0.0052 |     - |     - |     512 B |
|          |                |             |           |           |                     |        |       |       |           |
| ParseNew |         Single |  67.2911 ns | 0.6149 ns | 0.5752 ns |        14,860,804.6 | 0.0032 |     - |     - |     304 B |
|          |                |             |           |           |                     |        |       |       |           |
| ParseNew | SingleWithPlus |  83.6517 ns | 0.6121 ns | 0.5726 ns |        11,954,328.4 | 0.0042 |     - |     - |     392 B |
|          |                |             |           |           |                     |        |       |       |           |
| ParseNew |        Encoded |  99.3918 ns | 0.4289 ns | 0.3802 ns |        10,061,194.3 | 0.0042 |     - |     - |     384 B |

AFTER:

Measurement 1:

|   Method |     Categories |      Mean |    Error |   StdDev |         Op/s |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |--------------- |----------:|---------:|---------:|-------------:|-------:|------:|------:|----------:|
| ParseNew |    QueryString | 274.17 ns | 4.893 ns | 4.577 ns |  3,647,342.1 | 0.0052 |     - |     - |     512 B |
|          |                |           |          |          |              |        |       |       |           |
| ParseNew |         Single |  68.62 ns | 0.595 ns | 0.527 ns | 14,571,966.8 | 0.0032 |     - |     - |     304 B |
|          |                |           |          |          |              |        |       |       |           |
| ParseNew | SingleWithPlus |  89.93 ns | 0.541 ns | 0.480 ns | 11,119,491.8 | 0.0042 |     - |     - |     392 B |
|          |                |           |          |          |              |        |       |       |           |
| ParseNew |        Encoded |  93.91 ns | 0.704 ns | 0.624 ns | 10,648,088.7 | 0.0038 |     - |     - |     352 B |

Measurement 2:

|   Method |     Categories |      Mean |    Error |   StdDev |         Op/s |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |--------------- |----------:|---------:|---------:|-------------:|-------:|------:|------:|----------:|
| ParseNew |    QueryString | 275.22 ns | 2.572 ns | 2.406 ns |  3,633,462.4 | 0.0052 |     - |     - |     512 B |
|          |                |           |          |          |              |        |       |       |           |
| ParseNew |         Single |  69.96 ns | 0.784 ns | 0.695 ns | 14,293,085.2 | 0.0032 |     - |     - |     304 B |
|          |                |           |          |          |              |        |       |       |           |
| ParseNew | SingleWithPlus |  92.03 ns | 0.841 ns | 0.787 ns | 10,865,661.8 | 0.0042 |     - |     - |     392 B |
|          |                |           |          |          |              |        |       |       |           |
| ParseNew |        Encoded |  93.94 ns | 0.716 ns | 0.670 ns | 10,645,632.4 | 0.0038 |     - |     - |     352 B |

Part of: #56534

Fixing IterationCount related data consistency issues in QueryCollectionBenchmarks

src/Shared/QueryStringEnumerable.cs

ladeak · 2024-07-06T11:23:32Z

src/Shared/QueryStringEnumerable.cs

+            source.Replace(buffer, '+', ' ');
+            var success = Uri.TryUnescapeDataString(buffer, buffer, out var unescapedLength);
+            Debug.Assert(success);
+            return buffer.AsMemory(0, unescapedLength);


The difference here to commit Removing unsafe parts of Decode method in QueryStringEnumerable is that this implementation keeps a reference to the 'buffer' array that might be larger to the return value with the AsMemory() call. On the other hand, this implementation avoids an extra string allocation as shown on the perf measurement's Encoded line - and as well faster in that scenario.

I think Uri.TryUnescapeDataString should not return false, despite this comment. The unescape level used by TryUnescapeDataString method seems to avoid the 'rare' case. As an alternative the code could branch based on the result of TryUnescapeDataString, and in case of a false value invoke the Uri.UnescapeDataString method as a fallback.
Or as another alternative, go with the implementation from the previous commit.

I agree, TryUnescapeDataString shouldn't be able to return false in our case.
https://source.dot.net/#System.Private.Uri/System/UriExt.cs,648

src/Shared/QueryStringEnumerable.cs

BrennanConroy · 2024-07-08T20:55:25Z

src/Shared/QueryStringEnumerable.cs

+            source.Replace(buffer, '+', ' ');
+            var success = Uri.TryUnescapeDataString(buffer, buffer, out var unescapedLength);
+            Debug.Assert(success);
+            return buffer.AsMemory(0, unescapedLength);


I agree, TryUnescapeDataString shouldn't be able to return false in our case.
https://source.dot.net/#System.Private.Uri/System/UriExt.cs,648

…contains call

BrennanConroy · 2024-07-09T21:10:28Z

Thanks @ladeak! 1/Unknown unsafe usages gone 😄

Removing unsafe parts of Decode method in QueryStringEnumerable

988c868

Fixing IterationCount related data consistency issues in QueryCollectionBenchmarks

ladeak requested review from BrennanConroy, halter73 and captainsafia as code owners July 4, 2024 20:59

ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jul 4, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 4, 2024

ladeak commented Jul 4, 2024

View reviewed changes

src/Shared/QueryStringEnumerable.cs Outdated Show resolved Hide resolved

ladeak marked this pull request as draft July 4, 2024 21:27

mkArtakMSFT assigned adityamandaleeka Jul 5, 2024

Using TryUnescapeDataString to avoid extra string allocation

6e20619

ladeak commented Jul 6, 2024

View reviewed changes

ladeak marked this pull request as ready for review July 6, 2024 11:24

gfoidl reviewed Jul 8, 2024

View reviewed changes

src/Shared/QueryStringEnumerable.cs Outdated Show resolved Hide resolved

Review feedback

ce917f4

MihaZupan reviewed Jul 8, 2024

View reviewed changes

src/Shared/QueryStringEnumerable.cs Outdated Show resolved Hide resolved

BrennanConroy approved these changes Jul 8, 2024

View reviewed changes

Using contains and removing the length check for 16 chars before the …

68f6b5b

…contains call

ladeak force-pushed the ladeak-56534 branch from 0a7e701 to 68f6b5b Compare July 9, 2024 18:45

ladeak requested review from BrennanConroy and MihaZupan July 9, 2024 18:47

BrennanConroy added the Perf label Jul 9, 2024

BrennanConroy merged commit c52c284 into dotnet:main Jul 9, 2024
26 checks passed

dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Removing unsafe parts of Decode method in QueryStringEnumerable #56630

Removing unsafe parts of Decode method in QueryStringEnumerable #56630

Uh oh!

ladeak commented Jul 4, 2024 •

edited

Loading

Uh oh!

Uh oh!

ladeak Jul 6, 2024

Uh oh!

BrennanConroy Jul 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy Jul 8, 2024

Uh oh!

Uh oh!

BrennanConroy commented Jul 9, 2024

Uh oh!

Uh oh!

Removing unsafe parts of Decode method in QueryStringEnumerable #56630

Removing unsafe parts of Decode method in QueryStringEnumerable #56630

Uh oh!

Conversation

ladeak commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Performance

BEFORE:

AFTER:

Uh oh!

Uh oh!

ladeak Jul 6, 2024

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrennanConroy commented Jul 9, 2024

Uh oh!

Uh oh!

ladeak commented Jul 4, 2024 •

edited

Loading