Using SearchValues in ContentDispositionHeaderValue

[ladeak]

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

[ladeak]

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

[amcasey]

To confirm, FileNameStarEncoding is worse after the change?

[ladeak]

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]

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]

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

[ladeak]

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

[BrennanConroy]

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?

[ladeak]

#55039 (comment)

[amcasey]

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.

[BrennanConroy]

That's fair 👍

[amcasey]

LGTM, subject to @BrennanConroy's approval.

[amcasey]

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]

Thanks, @ladeak!