Remove unsafe use ASCII.GetBytes for WriteAscii

[benaadams]

ResponseHeadersWritingBenchmark

                       Type | branch |      Mean |     Error |         Op/s | Precent |
--------------------------- |------- |----------:|----------:|-------------:|--------:|
                 LiveAspNet | master | 347.71 ns | 0.7750 ns |  2,875,994.1 |         |
                 LiveAspNet |     pr | 357.18 ns | 1.3362 ns |  2,799,723.0 |     97% |
           PlaintextChunked | master |  56.30 ns | 0.0803 ns | 17,760,761.0 |         |
           PlaintextChunked |     pr |  56.88 ns | 0.1912 ns | 17,581,284.2 |     98% |
 PlaintextChunkedWithCookie | master | 126.62 ns | 0.3121 ns |  7,897,881.3 |         |
 PlaintextChunkedWithCookie |     pr | 102.36 ns | 0.2788 ns |  9,768,985.8 |    123% |
        PlaintextWithCookie | master | 120.31 ns | 0.4068 ns |  8,312,171.8 |         |
        PlaintextWithCookie |     pr |  91.83 ns | 0.2701 ns | 10,890,175.9 |    131% |
       TechEmpowerPlaintext | master |  50.50 ns | 0.2020 ns | 19,801,547.7 |         |
       TechEmpowerPlaintext |     pr |  50.87 ns | 0.2549 ns | 19,657,723.3 |     99% |

/cc @halter73 @GrabYourPitchforks @gfoidl

[gfoidl]

Ah, and it's ASP.NET Core, so var instead of concrete types or did this change with the move into the dotnet-org?

[halter73]

Ah, and it's ASP.NET Core, so var instead of concrete types or did this change with the move into the dotnet-org?

We still tend to use var in aspnetcore repo. That hasn't changed. At least not yet. That said, it's not like we run a linter for var vs type name.

[GrabYourPitchforks]

Why not use the framework-provided Encoding.ASCII.GetBytes? The in-box implementation is heavily optimized.

[benaadams]

Drops some on LiveAspNet and TechEmpowerPlaintext; but is a win for Plaintext+Cookie

                       Type |   Branch |      Mean |     Error |         Op/s | Precent |
--------------------------- |--------- |----------:|----------:|-------------:|--------:|
                 LiveAspNet |   master | 347.71 ns | 0.7750 ns |  2,875,994.1 |         |
                 LiveAspNet |    local | 320.89 ns | 1.1672 ns |  3,116,371.4 |    108% |
                 LiveAspNet | Encoding | 352.85 ns | 0.8868 ns |  2,834,071.4 |     98% |

           PlaintextChunked |   master |  56.30 ns | 0.0803 ns | 17,760,761.0 |         |
           PlaintextChunked |    local |  53.40 ns | 0.1866 ns | 18,725,755.9 |    105% |
           PlaintextChunked | Encoding |  55.96 ns | 0.1309 ns | 17,871,047.3 |    100% |

 PlaintextChunkedWithCookie |   master | 126.62 ns | 0.3121 ns |  7,897,881.3 |         |
 PlaintextChunkedWithCookie |    local | 120.37 ns | 0.4784 ns |  8,307,803.1 |    105% |
 PlaintextChunkedWithCookie | Encoding |  95.00 ns | 0.2749 ns | 10,525,934.9 |    133% |

        PlaintextWithCookie |   master | 120.31 ns | 0.4068 ns |  8,312,171.8 |         |
        PlaintextWithCookie |    local | 101.64 ns | 0.0933 ns |  9,839,094.2 |    118% |
        PlaintextWithCookie | Encoding |  93.90 ns | 0.3246 ns | 10,649,646.7 |    127% |

       TechEmpowerPlaintext |   master |  50.50 ns | 0.2020 ns | 19,801,547.7 |         |
       TechEmpowerPlaintext |    local |  47.72 ns | 0.2294 ns | 20,954,670.0 |    106% |
       TechEmpowerPlaintext | Encoding |  51.73 ns | 0.3582 ns | 19,329,642.0 |     97% |

Probably worth it as it removes a chunk of more unusual code and pushes the perf requirements back to the runtime where everyone benefits from improvements?

[GrabYourPitchforks]

We can always make further improvements at the runtime layer if needed. Additionally, if you're able, calling Encoding.ASCII.GetBytes(spans) instead of Encoding.ASCII.GetBytes(pointers) should shave off a tiny bit of overhead when dealing with small payloads.

[benaadams]

Regresses by comparison, but will push to this branch to see if you see anything wrong with the implementation

                       Type |   Branch |      Mean |     Error |         Op/s | Precent |
--------------------------- |----------|----------:|----------:|-------------:|--------:|
                 LiveAspNet |    spans | 368.19 ns | 1.1918 ns |  2,715,987.4 |     94% |
           PlaintextChunked |    spans |  54.76 ns | 0.2999 ns | 18,261,105.6 |    103% |
 PlaintextChunkedWithCookie |    spans | 101.44 ns | 0.2343 ns |  9,857,676.6 |    125% |
        PlaintextWithCookie |    spans |  95.57 ns | 0.2581 ns | 10,463,517.9 |    126% |
       TechEmpowerPlaintext |    spans |  51.80 ns | 0.1325 ns | 19,305,402.9 |     97% |

[benaadams]

Noticed am missing out a Macro-Op Fusion opportunity

[benaadams]

Will go with that

                       Type | branch |      Mean |     Error |         Op/s | Precent |
--------------------------- |------- |----------:|----------:|-------------:|--------:|
                 LiveAspNet | master | 347.71 ns | 0.7750 ns |  2,875,994.1 |         |
                 LiveAspNet |     pr | 357.18 ns | 1.3362 ns |  2,799,723.0 |     97% |
           PlaintextChunked | master |  56.30 ns | 0.0803 ns | 17,760,761.0 |         |
           PlaintextChunked |     pr |  56.88 ns | 0.1912 ns | 17,581,284.2 |     98% |
 PlaintextChunkedWithCookie | master | 126.62 ns | 0.3121 ns |  7,897,881.3 |         |
 PlaintextChunkedWithCookie |     pr | 102.36 ns | 0.2788 ns |  9,768,985.8 |    123% |
        PlaintextWithCookie | master | 120.31 ns | 0.4068 ns |  8,312,171.8 |         |
        PlaintextWithCookie |     pr |  91.83 ns | 0.2701 ns | 10,890,175.9 |    131% |
       TechEmpowerPlaintext | master |  50.50 ns | 0.2020 ns | 19,801,547.7 |         |
       TechEmpowerPlaintext |     pr |  50.87 ns | 0.2549 ns | 19,657,723.3 |     99% |

[davidfowl]

Contributes to #4720

[GrabYourPitchforks]

FWIW, I'd also personally like to see less unsafe code within the implementations of APIs like Encoding.ASCII.GetBytes. Those are opportunities for improvement in the runtime as we make the JIT smarter about proving correctness and as we add more "safe" low-level primitives into the VM. Applications which use these APIs will get these improvements for free as they come online.

[davidfowl]

I’m all for these changes (I filed that issue) but we don’t want to suddenly drop on techpower As well. We should make sure we can measure and fix whatever regressions come as a result of this(or decide they are minimal).

[GrabYourPitchforks]

The Encoding.ASCII.GetBytes API also has some logic for checking "what if this data isn't actually ASCII?" and going down a fallback code path. This check obviously add overhead compared with an implementation which assumes ASCII and performs straight-up widening or narrowing.

If there's a performance need for such an API there's a WIP proposal for this and a bunch of related APIs posted at https://github.com/dotnet/corefx/issues/34144#issuecomment-564893304. Just let us know what you'd like to see prioritized. :)

[ghost]

Hello @halter73!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.