Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Shrink ParseRequestLine inner loop #6594

Merged
merged 2 commits into from Jan 12, 2019
Merged

Shrink ParseRequestLine inner loop #6594

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
859d5b2
Shrink ParseRequestLine inner loop
benaadams Dec 13, 2018
c906723
enregister offset
benaadams Jan 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 29 additions & 40 deletions src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs
View file
Open in desktop
Expand Up @@ -73,62 +73,51 @@ public unsafe bool ParseRequestLine(TRequestHandler handler, in ReadOnlySequence
private unsafe void ParseRequestLine(TRequestHandler handler, byte* data, int length)
{
// Get Method and set the offset
var method = HttpUtilities.GetKnownMethod(data, length, out var offset);
var method = HttpUtilities.GetKnownMethod(data, length, out var pathStartOffset);

Span<byte> customMethod = method == HttpMethod.Custom ?
GetUnknownMethod(data, length, out offset) :
default;
Span<byte> customMethod = default;
Copy link
Member Author

Choose a reason for hiding this comment

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

-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

if (method == HttpMethod.Custom)
{
customMethod = GetUnknownMethod(data, length, out pathStartOffset);
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@benaadams benaadams Jan 11, 2019 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Great explanation 👍

// Skip space
offset++;
var offset = pathStartOffset + 1;
if (offset >= length)
{
// Start of path not found
RejectRequestLine(data, length);
}

byte ch = data[offset];
if (ch == ByteSpace || ch == ByteQuestionMark || ch == BytePercentage)
{
// Empty path is illegal, or path starting with percentage
RejectRequestLine(data, length);
}

byte ch = 0;
// Target = Path and Query
var pathEncoded = false;
var pathStart = -1;
var pathStart = offset;

// Skip first char (just checked)
offset++;

// Find end of path and if path is encoded
for (; offset < length; offset++)
{
ch = data[offset];
if (ch == ByteSpace)
if (ch == ByteSpace || ch == ByteQuestionMark)
{
if (pathStart == -1)
{
// Empty path is illegal
RejectRequestLine(data, length);
}

break;
}
else if (ch == ByteQuestionMark)
{
if (pathStart == -1)
{
// Empty path is illegal
RejectRequestLine(data, length);
}

// End of path
break;
}
else if (ch == BytePercentage)
{
if (pathStart == -1)
{
// Path starting with % is illegal
RejectRequestLine(data, length);
}

pathEncoded = true;
}
else if (pathStart == -1)
{
pathStart = offset;
}
}

if (pathStart == -1)
{
// Start of path not found
RejectRequestLine(data, length);
}

var pathBuffer = new Span<byte>(data + pathStart, offset - pathStart);
Expand Down