Skip to content

Using SearchValues in ContentDispositionHeaderValue#55039

Merged
amcasey merged 12 commits intodotnet:mainfrom
ladeak:ladeak-46484-contentdisposition
Apr 19, 2024
Merged

Using SearchValues in ContentDispositionHeaderValue#55039
amcasey merged 12 commits intodotnet:mainfrom
ladeak:ladeak-46484-contentdisposition

Conversation

@ladeak
Copy link
Contributor

@ladeak ladeak commented Apr 9, 2024 •
edited
Loading

Using SearchValues in ContentDispositionHeaderValue

Task of #46484

Description

Updating the current Encode5987 method to use SearchValues and IndexOfAny method during the encode of the FileNameStar property. Adding unit tests and a benchmark to measure the before-after performance.

Compared to the .NET runtime implementation a second Span<char> is pooled because ValueStringBuilder used by the .NET runtime is internal, hence Encoding.ASCII.GetChars needs a way to reference the chars passed to the StringBuilder

Performance Comparison:

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.4.24201.1
[Host] : .NET 9.0.0 (9.0.24.20403), X64 RyuJIT
Job-SUANLL : .NET 9.0.0 (9.0.24.18101), X64 RyuJIT

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

BEFORE:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
FileNameStarEncoding 213.8 ns 1.79 ns 1.68 ns 4,676,383.4 0.0052 - - 496 B
FileNameStarNoEncoding 201.0 ns 1.32 ns 1.23 ns 4,975,577.6 0.0050 - - 464 B

AFTER UPDATED: 2024/04/13 corresponding to commit With custom utf8 and hex encoding combined:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
FileNameStarEncoding 213.61 ns 1.828 ns 1.710 ns 4,681,410.7 0.0052 - - 496 B
FileNameStarNoEncoding 87.12 ns 0.962 ns 0.853 ns 11,478,216.6 0.0039 - - 368 B

Fixes #46484

@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 9, 2024
@ladeak
Copy link
Contributor Author

ladeak commented Apr 9, 2024

Adding @amcasey as he has been reviewing the previous related PRs.

@amcasey
Copy link
Member

amcasey commented Apr 9, 2024

To confirm, FileNameStarEncoding is worse after the change?

@ladeak
Copy link
Contributor Author

ladeak commented Apr 9, 2024

To confirm, FileNameStarEncoding is worse after the change?

Encoding means it has characters to actually convert to HexString while NoEncoding does not have, but it still does make sure nothing to encode.
The actual results will also greatly depend on the number and frequency of chars to be converted to Hex.

@amcasey
Copy link
Member

amcasey commented Apr 9, 2024 •
edited
Loading

To confirm, FileNameStarEncoding is worse after the change?

Encoding means it has characters to actually convert to HexString while NoEncoding does not have, but it still does make sure nothing to encode. The actual results will also greatly depend on the number and frequency of chars to be converted to Hex.

Sorry, I think we got our lines crossed. I know encoding is expected to be slower than no-encoding (by a variable amount), but it looks like encoding-after is slower than encoding-before?

amcasey
amcasey reviewed Apr 9, 2024
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks generally correct, but I think the possible perf regression in the no-encoding case is worth revisiting.

ladeak
ladeak commented Apr 10, 2024
View reviewed changes
@ladeak
Copy link
Contributor Author

ladeak commented Apr 10, 2024

Here is my latest run:

Method Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
FileNameStarEncoding 211.84 ns 1.965 ns 1.838 ns 4,720,566.7 0.0052 - - 496 B
FileNameStarNoEncoding 97.99 ns 0.685 ns 0.608 ns 10,205,254.5 0.0039 - - 368 B

@ladeak ladeak requested a review from amcasey April 10, 2024 18:00
amcasey
amcasey reviewed Apr 10, 2024
View reviewed changes
BrennanConroy
BrennanConroy reviewed Apr 11, 2024
View reviewed changes
ladeak added 11 commits April 16, 2024 07:47
Using SearchValues in ContentDispositionHeaderValue
5b4e92d 
Updating current Encode5987 method to use SearchValues and IndexOfAny to encode the FileNameStar property.
Adding unit tests and a  benchmark to measure the before-after performance
Removing if-else case for the encoding parts with the rune.
43062a5
Correcting the size of the temp char buffer allocation
981ae53
Review feedback
4cf5521
Adding test for inputs that does not fit on a stacksize
210426c
Using a for loop instead of a while loop for processing toHexEscape
e35e565
Using ref struct string interpolation with string builder
d691376
Using a const for the stackallocted buffer sizes.
f93cadf
Remove unneeded span
f322930
Using Rune to encode/decode non ascii characters
713f552
With custom utf8 and hex encoding combined
7dc32d9
@ladeak ladeak force-pushed the ladeak-46484-contentdisposition branch from 2eadb69 to 7dc32d9 Compare April 16, 2024 05:48
@ladeak ladeak requested review from BrennanConroy and amcasey April 16, 2024 08:34
BrennanConroy
BrennanConroy reviewed Apr 16, 2024
View reviewed changes
src/Http/Headers/src/ContentDispositionHeaderValue.cs
builder.Append('%');
builder.Append(HexUpperChars[(c & 0xf0) >> 4]);
builder.Append(HexUpperChars[c & 0xf]);
// Inspired by https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs TryEncodeToUtf8
Copy link
Member

@BrennanConroy BrennanConroy Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of copying this code. How much would we lose if we just stackalloc byte[4] and called TryEncodeToUtf8 and looped over the result to write to the StringBuilder?

Copy link
Contributor Author

@ladeak ladeak Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#55039 (comment)

Copy link
Member

@amcasey amcasey Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I can live with the copy since it's an implementation of spec'd behavior. I don't think we'll get out of sync in a way that reduces correctness, but we could well fail to benefit from future perf wins. If there's a way around copying, that would be preferable, but I wouldn't block the PR for this. Having said that, I also wouldn't go ahead without @BrennanConroy's approval.

Copy link
Member

@BrennanConroy BrennanConroy Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair 👍

@ladeak ladeak requested a review from BrennanConroy April 17, 2024 04:52
gfoidl
gfoidl reviewed Apr 17, 2024
View reviewed changes
amcasey
amcasey approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, subject to @BrennanConroy's approval.

src/Http/Headers/src/ContentDispositionHeaderValue.cs
builder.Append('%');
builder.Append(HexUpperChars[(c & 0xf0) >> 4]);
builder.Append(HexUpperChars[c & 0xf]);
// Inspired by https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs TryEncodeToUtf8
Copy link
Member

@amcasey amcasey Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I can live with the copy since it's an implementation of spec'd behavior. I don't think we'll get out of sync in a way that reduces correctness, but we could well fail to benefit from future perf wins. If there's a way around copying, that would be preferable, but I wouldn't block the PR for this. Having said that, I also wouldn't go ahead without @BrennanConroy's approval.

@amcasey amcasey merged commit 84af599 into dotnet:main Apr 19, 2024
@amcasey
Copy link
Member

amcasey commented Apr 19, 2024

Thanks, @ladeak!

@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amcasey amcasey amcasey approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@captainsafia captainsafia Awaiting requested review from captainsafia

+1 more reviewer

@gfoidl gfoidl gfoidl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@adityamandaleeka adityamandaleeka

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member Perf

Projects

None yet

Milestone

9.0-preview4

Development

Successfully merging this pull request may close these issues.

Use SearchValues throughout dotnet/aspnetcore

5 participants

@ladeak @amcasey @gfoidl @BrennanConroy @adityamandaleeka