New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shrink ParseRequestLine inner loop #6594

Merged
merged 2 commits into from Jan 12, 2019

Conversation

Projects
None yet
3 participants
@benaadams
Copy link
Contributor

benaadams commented Jan 11, 2019

pathStart == -1 and RejectRequestLine calls are unnecessary as they can be checked and called outside loop.

Removes 4 branches and 3 method calls from inside the path for loop, and puts the offset variable in a register

                Method |     Mean |        Op/s |
 --------------------- |---------:|------------:|
- PlaintextTechEmpower | 338.1 ns | 2,957,510.7 |
+ PlaintextTechEmpower | 328.4 ns | 3,045,455.2 |
-           LiveAspNet | 578.0 ns | 1,729,970.8 |
+           LiveAspNet | 572.8 ns | 1,745,677.2 |
-              Unicode | 792.0 ns | 1,262,606.8 |
+              Unicode | 735.9 ns | 1,358,882.0 |
; Assembly listing for method HttpParser`1:ParseRequestLine(struct,long,int):this

-; Total bytes of code 1003, prolog size 62 for method HttpParser`1:ParseRequestLine(struct,long,int):this
+; Total bytes of code 881, prolog size 65 for method HttpParser`1:ParseRequestLine(struct,long,int):this

/cc @davidfowl @pakrym @halter73 @JamesNK

@benaadams

This comment has been minimized.

Copy link
Contributor

benaadams commented Jan 11, 2019

IISIntegration test, wrong exception

[xUnit.net 00:02:12.71]     Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.ClientDisconnectTests.ReaderThrowsCancelledException [FAIL]
Failed   Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.ClientDisconnectTests.ReaderThrowsCancelledException
Error Message:
 Assert.IsType() Failure
Expected: System.OperationCanceledException
Actual:   Microsoft.AspNetCore.Connections.ConnectionResetException
Stack Trace:
   at Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.ClientDisconnectTests.ReaderThrowsCancelledException() in /_/src/Servers/IIS/IIS/test/IIS.Tests/ClientDisconnectTests.cs:line 206
--- End of stack trace from previous location where exception was thrown ---
Standard Output Messages:
 | [0.001s] TestLifetime Information: Starting test ReaderThrowsCancelledException at 2019-01-11T03:52:22
 | [4.007s] Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.TestServer Debug: Method: GET, RequestUri: 'http://localhost:2096/start', Version: 2.0, Content: <null>, Headers:
 |                                                                                       {
 |                                                                                       }
 | [4.013s] Microsoft.AspNetCore.Hosting.Internal.WebHost Debug: Hosting starting
 | [4.014s] Microsoft.AspNetCore.Hosting.Internal.WebHost Debug: Hosting started
 | [4.014s] Microsoft.AspNetCore.Hosting.Internal.WebHost Debug: Loaded hosting startup assembly IIS.Tests, Version=3.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60
 | [4.015s] Microsoft.AspNetCore.Hosting.Internal.WebHost Information: Request starting HTTP/1.1 GET http://localhost:2096/start  
 | [4.015s] Microsoft.AspNetCore.Hosting.Internal.WebHost Information: Request finished in 0.71ms 200 
 | [4.016s] Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.TestServer Debug: StatusCode: 200, ReasonPhrase: 'OK', Version: 1.1, Content: System.Net.Http.HttpConnectionResponseContent, Headers:
 |                                                                                       {
 |                                                                                         Server: Microsoft-IIS/10.0
 |                                                                                         Date: Fri, 11 Jan 2019 03:52:26 GMT
 |                                                                                         Content-Length: 4
 |                                                                                       }
 | [4.016s] Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests.TestServer Debug: Done
 | [4.019s] Microsoft.AspNetCore.Hosting.Internal.WebHost Information: Request starting HTTP/1.1 POST http://localhost/  1
 | [125.578s] Microsoft.AspNetCore.Server.IIS.Core.IISHttpServer Debug: Connection ID "18230571293206380545" disconnecting.
 | [125.579s] Microsoft.AspNetCore.Hosting.Internal.WebHost Information: Request finished in 121560.5506ms 0 
 | [125.682s] Microsoft.AspNetCore.Hosting.Internal.WebHost Debug: Hosting shutdown
 | [126.888s] TestLifetime Information: Finished test ReaderThrowsCancelledException in 126.8868381s
@benaadams

This comment has been minimized.

Copy link
Contributor

benaadams commented Jan 11, 2019

; Assembly listing for method HttpParser`1:ParseRequestLine(struct,long,int):this

-G_M24530_IG10:                                                   ; for body start
-       8B45C0               mov      eax, dword ptr [rbp-40H]
-       4863C8               movsxd   rcx, eax
-       0FB6040E             movzx    rax, byte  ptr [rsi+rcx]
-       83F820               cmp      eax, 32
-       7439                 je       SHORT G_M24530_IG16
-
-G_M24530_IG11:
-       83F83F               cmp      eax, 63
-       0F8490010000         je       G_M24531_IG30
-
-G_M24531_IG12:
-       83F825               cmp      eax, 37
-       7512                 jne      SHORT G_M24531_IG14
-       4183FBFF             cmp      r11d, -1
-       0F8446020000         je       G_M24532_IG40
-
-G_M24532_IG13:
-       41BA01000000         mov      r10d, 1
-       EB0A                 jmp      SHORT G_M24532_IG15
-
-G_M24532_IG14:
-       4183FBFF             cmp      r11d, -1
-       7504                 jne      SHORT G_M24532_IG15
-       448B5DC0             mov      r11d, dword ptr [rbp-40H]
-
-G_M24532_IG15:
-       8B4DC0               mov      ecx, dword ptr [rbp-40H]
-       FFC1                 inc      ecx
-       894DC0               mov      dword ptr [rbp-40H], ecx
-       397DC0               cmp      dword ptr [rbp-40H], edi
-       7CBA                 jl       SHORT G_M24537_IG10         ; for body end

+G_M24517_IG13:                                                   ; for body start
+       4963C8               movsxd   rcx, r8d
+       0FB60C0E             movzx    rcx, byte  ptr [rsi+rcx]
+       83F920               cmp      ecx, 32
+       7417                 je       SHORT G_M24517_IG15
+       83F93F               cmp      ecx, 63
+       7412                 je       SHORT G_M24517_IG15
+       83F925               cmp      ecx, 37
+       7505                 jne      SHORT G_M24517_IG14
+       BA01000000           mov      edx, 1
+
+G_M24517_IG14:
+       41FFC0               inc      r8d
+       443BC7               cmp      r8d, edi
+       7CDD                 jl       SHORT G_M24517_IG13         ; for body end

-; Total bytes of code 1003, prolog size 62 for method HttpParser`1:ParseRequestLine(struct,long,int):this
+; Total bytes of code 881, prolog size 65 for method HttpParser`1:ParseRequestLine(struct,long,int):this
Span<byte> customMethod = method == HttpMethod.Custom ?
GetUnknownMethod(data, length, out offset) :
default;
Span<byte> customMethod = default;

This comment has been minimized.

@benaadams

benaadams Jan 11, 2019

Contributor
-G_M24527_IG07:
-       4183FF09             cmp      r15d, 9
-       740D                 je       SHORT G_M24529_IG08
-       33D2                 xor      rdx, rdx
-       33C9                 xor      ecx, ecx
-       488955B0             mov      bword ptr [rbp-50H], rdx
-       894DB8               mov      dword ptr [rbp-48H], ecx
-       EB1B                 jmp      SHORT G_M24529_IG09

+G_M24517_IG07:
+       488D55B0             lea      rdx, bword ptr [rbp-50H]
+       C5F857C0             vxorps   xmm0, xmm0
+       C5FA7F02             vmovdqu  qword ptr [rdx], xmm0

G_M24529_IG08:
+      4183FF09             cmp      r15d, 9
@benaadams

This comment has been minimized.

Copy link
Contributor

benaadams commented Jan 11, 2019

Updated numbers

                Method |     Mean |    Error |   StdDev |        Op/s |
 --------------------- |---------:|---------:|---------:|------------:|
- PlaintextTechEmpower | 338.1 ns | 1.350 ns | 1.197 ns | 2,957,510.7 |
+ PlaintextTechEmpower | 328.4 ns | 2.863 ns | 2.538 ns | 3,045,455.2 |
-           LiveAspNet | 578.0 ns | 3.571 ns | 3.165 ns | 1,729,970.8 |
+           LiveAspNet | 572.8 ns | 2.481 ns | 2.320 ns | 1,745,677.2 |
-              Unicode | 792.0 ns | 7.900 ns | 6.597 ns | 1,262,606.8 |
+              Unicode | 735.9 ns | 4.949 ns | 4.387 ns | 1,358,882.0 |

@davidfowl davidfowl requested a review from halter73 Jan 11, 2019


// Use a new offset var as pathStartOffset needs to be on stack
// as its passed by reference above so can't be in register.

This comment has been minimized.

@halter73

halter73 Jan 11, 2019

Member

Can you explain this a little more? Why is it important to use pathStartOffset for the out var instead of reusing offset like before? Is there better performance if you don't mutate the out var or something?

This comment has been minimized.

@benaadams

benaadams Jan 11, 2019

Contributor

Its pass byref so it needs a memory location; registers are only used for parameters following strict ABI rules.

Basically the location will get passed via register; but it needs a location on stack to be that location. Which is fine.

Or in Jit terms:

  • do-not-enreg[XS] must-init addr-exposed ld-addr-op
  • Do not put in register, as memory location address must be used for this variable.

The problem is the Jit will only allocate the local var once. So it will get a stack location rather than a register, because it needs to be passed byref.

The side effect is every other use of that local in the function will also be that stack location; so you get this:

mov      ecx, dword ptr [rbp-40H]   ; move value in memory to register ecx
inc      ecx                        ; increment register ecx
mov      dword ptr [rbp-40H], ecx   ; move register ecx to memory value
cmp      dword ptr [rbp-40H], edi   ; compare memory value to register edi
jl       SHORT G_M24537_IG10

With the memory location dword ptr [rbp-40H] being the local variable (and it has to move it back a forth from register as some ops only work on registers; then update the memory location with the register result).

Assigning it to another variable an then using that one instead breaks the dependency on that memory location, so instead you get this:

inc      r8d                        ; increment register ecx
cmp      r8d, edi                   ; compare register ecx to register edi
jl       SHORT G_M24517_IG13 

With the register r8d being the local variable; and registers are faster than memory locations, as well as having less shuffling back and forth.

This comment has been minimized.

@halter73

halter73 Jan 11, 2019

Member

Great explanation 👍

@davidfowl davidfowl merged commit b27739a into aspnet:master Jan 12, 2019

1 check passed

license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment