Use one SequenceReader+Rewind rather than 2 SequenceReaders #8076

benaadams · 2019-03-01T15:51:06Z

SequenceReader is a very chunky struct; so taking a copy of it to rewind 2 chars in an uncommon case is expensive as it forces stack reservation and zeroing for the copy:

Also move the exception handling to the outer method, and clean up the loop

                Method |     Mean |    Error |   StdDev |        Op/s | Scaled |
 --------------------- |---------:|---------:|---------:|------------:|-------:|
- PlaintextTechEmpower | 310.4 ns | 1.301 ns | 1.153 ns | 3,221,411.1 |   1.00 |
+ PlaintextTechEmpower | 271.8 ns | 2.267 ns | 2.121 ns | 3,679,437.9 |   1.00 |
-           LiveAspNet | 526.9 ns | 2.322 ns | 1.939 ns | 1,897,874.1 |   1.70 |
+           LiveAspNet | 454.3 ns | 2.496 ns | 2.212 ns | 2,201,384.0 |   1.55 |
-              Unicode | 687.7 ns | 3.629 ns | 3.217 ns | 1,454,153.2 |   2.22 |
+              Unicode | 614.2 ns | 3.414 ns | 3.193 ns | 1,628,024.1 |   2.09 |

(Some PR changes highlighted by diff)

; Assembly listing for method HttpParser`1:ParseHeaders(struct,byref):bool:this
-; Lcl frame size = 1032
+; Lcl frame size = 120

-mov      ecx, 226
+mov      ecx, 14
 xor      rax, rax
 rep stosd

Zeroing in the common path to set it to default(SequenceReader<byte>)

-    call     SequenceReader`1:GetNextSpan():this
-
-G_M20071_IG23:
-    xor      rcx, rcx
-    lea      rax, bword ptr [rbp-108H]
-    vxorps   xmm0, xmm0
-    vmovdqu  qword ptr [rax], xmm0
-    vmovdqu  qword ptr [rax+16], xmm0
-    vmovdqu  qword ptr [rax+32], xmm0
-    vmovdqu  qword ptr [rax+48], xmm0
-    vmovdqu  qword ptr [rax+64], xmm0
-    vmovdqu  qword ptr [rax+80], xmm0
-    mov      qword ptr [rax+96], rcx
-    mov      dword ptr [rbp-10CH], ecx
-
-G_M20071_IG24:
-    cmp      byte  ptr [rbp-8CH], 0
-    je       G_M20071_IG190

+    call     SequenceReader`1:GetNextSpan():this
+
+G_M20042_IG23:
+    xor      ecx, ecx
+    mov      dword ptr [rbp-A4H], ecx
+
+G_M20042_IG24:
+    cmp      byte  ptr [rbp-8CH], 0
+    je       G_M20042_IG204

/cc @JeremyKuhne @davidfowl

benaadams · 2019-03-01T17:29:55Z

Rewind is expensive; trying something else

analogrelay · 2019-03-01T19:12:47Z

@benaadams Btw, rebase on master when you can. You've got some flaky tests failing in your PR and I think we've knocked them down in master now :).

Edit: I think re-running will get the changes, so I'm triggering helix again.

analogrelay · 2019-03-01T22:00:15Z

/azp run AspNetCore-helix-test

azure-pipelines · 2019-03-01T22:00:26Z

Azure Pipelines successfully started running 1 pipeline(s).

JeremyKuhne · 2019-03-01T22:28:47Z

@benaadams This change: aspnet/KestrelHttpServer#3068 has a revamp of this code which in micro benchmarks was much much faster. Do you have any interest in getting that back up / further refining it? It would be good for measuring the impact of other possible reader changes.

benaadams · 2019-03-01T23:05:26Z

@JeremyKuhne If I made the parser "safe"; what would @blowdart think of me 😱

Will take a look 😸

benaadams · 2019-03-02T02:29:50Z

made the parser "safe" ...

Bit of a rabbit hole, probably should be done as its own PR

JeremyKuhne · 2019-03-02T03:27:46Z

Bit of a rabbit hole, probably should be done as its own PR

Sorry, I wasn't suggesting that it be done in this PR. It does, however, cross over this same method and make this change moot (I think). I'll get that one going again if you don't want to, just thought I'd point it out and offer it to you as you're much more focused on this right now than I am. :) Just let me know.

benaadams · 2019-03-02T23:27:29Z

                Method |     Mean |    Error |   StdDev |        Op/s | Scaled |
 --------------------- |---------:|---------:|---------:|------------:|-------:|
- PlaintextTechEmpower | 310.4 ns | 1.301 ns | 1.153 ns | 3,221,411.1 |   1.00 |
+ PlaintextTechEmpower | 271.8 ns | 2.267 ns | 2.121 ns | 3,679,437.9 |   1.00 |
-           LiveAspNet | 526.9 ns | 2.322 ns | 1.939 ns | 1,897,874.1 |   1.70 |
+           LiveAspNet | 454.3 ns | 2.496 ns | 2.212 ns | 2,201,384.0 |   1.55 |
-              Unicode | 687.7 ns | 3.629 ns | 3.217 ns | 1,454,153.2 |   2.22 |
+              Unicode | 614.2 ns | 3.414 ns | 3.193 ns | 1,628,024.1 |   2.09 |

src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

benaadams · 2019-03-03T00:29:36Z

Helix Win10 error

System.IO.IOException : The process cannot access the file '...' because it is being used by another process.

Failed   Microsoft.AspNetCore.Authentication.DataHandler.SecureDataFormatTests.UnprotectWithDifferentPurposeFails
Error Message:
 System.Security.Cryptography.CryptographicException : An error occurred while trying to encrypt the provided data. Refer to the inner exception for more information.
---- System.IO.IOException : The process cannot access the file 'C:\Users\runner\AppData\Local\ASP.NET\DataProtection-Keys\key-7ec42fe5-23f5-4bc1-ad2c-85ba5ff21a6e.xml' because it is being used by another process.
Stack Trace:
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.Protect(Byte[] plaintext) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedDataProtector.cs:line 141
   at Microsoft.AspNetCore.Authentication.SecureDataFormat`1.Protect(TData data, String purpose) in /_/src/Security/Authentication/Core/src/SecureDataFormat.cs:line 34
   at Microsoft.AspNetCore.Authentication.DataHandler.SecureDataFormatTests.UnprotectWithDifferentPurposeFails() in /_/src/Security/Authentication/test/SecureDataFormatTests.cs:line 59
----- Inner Stack Trace -----
   at System.IO.FileStream.ValidateFileHandle(SafeFileHandle fileHandle)
   at System.IO.FileStream.CreateFileOpenHandle(FileMode mode, FileShare share, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share, Int32 bufferSize, FileOptions options)
   at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share)
   at System.IO.File.OpenRead(String path)
   at Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.ReadElementFromFile(String fullPath) in /_/src/DataProtection/DataProtection/src/Repositories/FileSystemXmlRepository.cs:line 102
   at Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.GetAllElementsCore()+MoveNext() in /_/src/DataProtection/DataProtection/src/Repositories/FileSystemXmlRepository.cs:line 83
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.GetAllElements() in /_/src/DataProtection/DataProtection/src/Repositories/FileSystemXmlRepository.cs:line 68
   at Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.GetAllKeys() in /_/src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs:line 152
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.CreateCacheableKeyRingCore(DateTimeOffset now, IKey keyJustAdded) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 57
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.CreateCacheableKeyRingCore(DateTimeOffset now, IKey keyJustAdded) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 103
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.ICacheableKeyRingProvider.GetCacheableKeyRing(DateTimeOffset now) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 254
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.GetCurrentKeyRingCore(DateTime utcNow) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 186
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.GetCurrentKeyRing() in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs:line 142
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.Protect(Byte[] plaintext) in /_/src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedDataProtector.cs:line 102

analogrelay · 2019-03-03T01:07:03Z

Yep, that's https://github.com/aspnet/AspNetCore/issues/7366. Applying ye olde skip attribute ;P

analogrelay · 2019-03-03T01:26:37Z

#8129 should unblock this

benaadams · 2019-04-06T02:27:52Z

Rebased

benaadams · 2019-04-09T11:48:52Z

Rebased

analogrelay · 2019-04-18T17:41:51Z

Moving this to preview 6. FYI, this should not be merged until we've got confirmation that the branch is open for preview 6 changes. We've got a tight timeline for preview 5 and are starting to lock down now :).

benaadams · 2019-05-09T19:00:56Z

Rebased

benaadams · 2019-05-27T15:06:14Z

Rebased

analogrelay · 2019-05-28T17:14:02Z

@davidfowl @Tratcher @jkotalik @halter73 Where do we stand (on our side) as far as merging this? Preview 6 is locking soon and we already punted this from preview 5 :).

benaadams · 2019-05-29T02:18:37Z

Rebased

benaadams · 2019-05-29T23:02:24Z

Rebased

benaadams · 2019-06-06T10:29:01Z

Rebased

analogrelay · 2019-06-06T16:57:02Z

Ping @Tratcher @jkotalik @halter73 - we should get this reviewed instead of just making @benaadams rebase it down the road ;P

halter73

This looks solid to me. I think we're ready to merge. @davidfowl do you agree?

halter73 · 2019-06-06T23:29:11Z

src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs

            }

-            return result;
+            bool TrimAndTakeMessageHeaders(in ReadOnlySequence<byte> buffer, bool trailers, out SequencePosition consumed, out SequencePosition examined)


The redundancy of TakeMessageHeaders and TrimAndTakeMessageHeaders is one thing I don't love, but I understand doing this if necessary to optimize the fast path.

jkotalik · 2019-06-07T20:15:36Z

Thanks @benaadams

benaadams requested review from jkotalik and Tratcher as code owners March 1, 2019 15:51

benaadams mentioned this pull request Mar 1, 2019

[No Merge][WIP] SequenceReader pass ReadOnlySequence via in dotnet/corefx#35678

Closed

Eilon added the area-servers label Mar 1, 2019

benaadams changed the title ~~Use one SequenceReader+Rewind rather than 2 SequenceReaders~~ [wip] Use one SequenceReader+Rewind rather than 2 SequenceReaders Mar 1, 2019

analogrelay added the rebase-needed label Mar 1, 2019

benaadams force-pushed the Parser branch from 5b59c6f to b043fea Compare March 2, 2019 04:27

benaadams changed the title ~~[wip] Use one SequenceReader+Rewind rather than 2 SequenceReaders~~ Use one SequenceReader+Rewind rather than 2 SequenceReaders Mar 2, 2019

benaadams force-pushed the Parser branch from 778560c to 0d160d2 Compare March 2, 2019 22:32

davidfowl reviewed Mar 3, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs Outdated Show resolved Hide resolved

analogrelay removed the rebase-needed label Mar 3, 2019

benaadams force-pushed the Parser branch from 0d160d2 to 47f36ca Compare March 4, 2019 01:35

benaadams mentioned this pull request Mar 14, 2019

Avoid boxing allocations for async in Tier0 dotnet/coreclr#22984

Merged

benaadams force-pushed the Parser branch from 47f36ca to a79a3ea Compare April 6, 2019 00:18

benaadams requested a review from analogrelay as a code owner April 6, 2019 00:18

benaadams force-pushed the Parser branch from a79a3ea to 2e31598 Compare April 6, 2019 00:26

analogrelay added this to the 3.0.0-preview5 milestone Apr 8, 2019

benaadams force-pushed the Parser branch from 2e31598 to ad1dd37 Compare April 9, 2019 11:48

analogrelay modified the milestones: 3.0.0-preview5, 3.0.0-preview6 Apr 18, 2019

benaadams force-pushed the Parser branch from ad1dd37 to 707ed97 Compare May 9, 2019 19:00

benaadams force-pushed the Parser branch from 707ed97 to 60c6f32 Compare May 27, 2019 15:05

benaadams force-pushed the Parser branch from 60c6f32 to 9811413 Compare May 29, 2019 02:18

benaadams force-pushed the Parser branch from 9811413 to 83d2730 Compare May 29, 2019 23:00

analogrelay modified the milestones: 3.0.0-preview6, 3.0.0-preview7 Jun 3, 2019

Use one SequenceReader+Rewind rather than 2

b0b4dd9

benaadams force-pushed the Parser branch from 83d2730 to b0b4dd9 Compare June 6, 2019 10:28

halter73 approved these changes Jun 6, 2019

View reviewed changes

halter73 reviewed Jun 6, 2019

View reviewed changes

jkotalik approved these changes Jun 7, 2019

View reviewed changes

jkotalik merged commit f10680a into dotnet:master Jun 7, 2019

benaadams deleted the Parser branch June 7, 2019 20:25

halter73 mentioned this pull request Apr 27, 2020

Don't count start line toward header size limit #21272

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use one SequenceReader+Rewind rather than 2 SequenceReaders #8076

Use one SequenceReader+Rewind rather than 2 SequenceReaders #8076

benaadams commented Mar 1, 2019 •

edited

Loading

benaadams commented Mar 1, 2019

analogrelay commented Mar 1, 2019 •

edited

Loading

analogrelay commented Mar 1, 2019

azure-pipelines bot commented Mar 1, 2019

JeremyKuhne commented Mar 1, 2019

benaadams commented Mar 1, 2019

benaadams commented Mar 2, 2019

JeremyKuhne commented Mar 2, 2019

benaadams commented Mar 2, 2019 •

edited

Loading

benaadams commented Mar 3, 2019

analogrelay commented Mar 3, 2019 •

edited

Loading

analogrelay commented Mar 3, 2019

benaadams commented Apr 6, 2019

benaadams commented Apr 9, 2019

analogrelay commented Apr 18, 2019

benaadams commented May 9, 2019

benaadams commented May 27, 2019

analogrelay commented May 28, 2019

benaadams commented May 29, 2019

benaadams commented May 29, 2019

benaadams commented Jun 6, 2019

analogrelay commented Jun 6, 2019

halter73 left a comment

halter73 Jun 6, 2019

jkotalik commented Jun 7, 2019

Use one SequenceReader+Rewind rather than 2 SequenceReaders #8076

Use one SequenceReader+Rewind rather than 2 SequenceReaders #8076

Conversation

benaadams commented Mar 1, 2019 • edited Loading

benaadams commented Mar 1, 2019

analogrelay commented Mar 1, 2019 • edited Loading

analogrelay commented Mar 1, 2019

azure-pipelines bot commented Mar 1, 2019

JeremyKuhne commented Mar 1, 2019

benaadams commented Mar 1, 2019

benaadams commented Mar 2, 2019

JeremyKuhne commented Mar 2, 2019

benaadams commented Mar 2, 2019 • edited Loading

benaadams commented Mar 3, 2019

analogrelay commented Mar 3, 2019 • edited Loading

analogrelay commented Mar 3, 2019

benaadams commented Apr 6, 2019

benaadams commented Apr 9, 2019

analogrelay commented Apr 18, 2019

benaadams commented May 9, 2019

benaadams commented May 27, 2019

analogrelay commented May 28, 2019

benaadams commented May 29, 2019

benaadams commented May 29, 2019

benaadams commented Jun 6, 2019

analogrelay commented Jun 6, 2019

halter73 left a comment

Choose a reason for hiding this comment

halter73 Jun 6, 2019

Choose a reason for hiding this comment

jkotalik commented Jun 7, 2019

benaadams commented Mar 1, 2019 •

edited

Loading

analogrelay commented Mar 1, 2019 •

edited

Loading

benaadams commented Mar 2, 2019 •

edited

Loading

analogrelay commented Mar 3, 2019 •

edited

Loading