Reduce GetString allocs and conversions #312

benaadams · 2015-11-01T07:18:25Z

Resolves #291

davidfowl · 2015-11-01T07:32:44Z

src/Microsoft.AspNet.Server.Kestrel/Infrastructure/MemoryPoolIterator2.cs

+        {
+            // avoid declaring other local vars, or doing work with stackalloc
+            // to prevent the .locals init cil flag , see: https://github.com/dotnet/coreclr/issues/1279
+            char* output = stackalloc char[length];


How big can length be?

Added 16k max

Is just a limit on the max it will attempt with stackalloc before using new and heap allocating instead; isn't actually a parsing limit.

halter73 · 2015-11-04T17:52:41Z

src/Microsoft.AspNet.Server.Kestrel/Infrastructure/MemoryPoolIterator2.cs

@@ -10,6 +10,7 @@ namespace Microsoft.AspNet.Server.Kestrel.Infrastructure
 {
    public struct MemoryPoolIterator2
    {
+        private const int _maxHeaderStackLength = 16384;


Is there a specific reason we don't want to stackalloc a char array larger than this?

Total headers are generally constrained < 8k/16k So individual headers shouldn't be larger than 16k; if you are going larger than that you probably have other issues to resolve than performance - Added some related comments to: https://github.com/aspnet/KestrelHttpServer/pull/313/files#diff-6b7953b837b6ad7c921259c907520639R14

Can go larger; but that was my reasoning.

For upper levels; total stack size aspnet4 x86 was 256k, x64 was 512k; .net 1MB - not sure what the current defaults are.

For ref:
nginx default 8k
Apache default 8190 bytes
IIS default 16384 bytes

To add even more reasoning here: HTTP/2 compression will further reduce the average header size. I agree 16k is a good limit given what we see out in the world.

This value is in the wrong layer MemoryPoolIterator2 doesn't understand what it's looking at. That's the caller's job.

Its to prevent the stack blowing up; so if length is larger than this it heap allocates instead. Maybe a rename like _maxStackAllocBytes?

halter73 · 2015-11-13T00:57:50Z

This looks good to me, but since this is a pretty big change I would like someone else to also sign off on it.

@lodejard @davidfowl @troydai thoughts?

davidfowl · 2015-11-13T01:08:52Z

Is there anyway we can take advantage of CopyTo here? We already have the memory pool interator

benaadams · 2015-11-13T12:58:26Z

@davidfowl you mean move into MemoryPoolIterator2 rather than have it in Frame? GetNextString kinda thing?

troydai · 2015-11-13T17:56:00Z

It looks good to me. The change is straightforward. One thing we may or may not to do is to extract the logics of taking two iterators to generate a string to a separate class. The logic here expand the iterator class which in my opinion should be kept concise. The logic of creating string with two iterators is very much self-contained.

benaadams · 2015-11-14T23:12:31Z

Renamed constant

benaadams · 2015-11-15T11:20:52Z

I think this can be cleaned up

benaadams · 2015-11-15T11:44:08Z

Simpler?

benaadams · 2015-11-15T12:10:30Z

Is there anyway we can take advantage of CopyTo here?

Its the byte -> char conversion that needs to be explicit to get it in right format for string; CopyTo is byte-> byte

One thing we may or may not to do is to extract the logics of taking two iterators to generate a string to a separate class.

@troydai Changed the string from MemoryPoolIterator to Extenstion methods; better?

benaadams · 2015-11-15T15:10:06Z

For the Utf8->char*(string) it can be done faster with intrinsics but I don't think Vector exposes all the needed methods (see http://woboq.com/blog/utf-8-processing-using-simd.html) and probably would want to push that in the coreclr/corefx Encoding.UTF8, so this code would remain the same anyway.

davidfowl · 2015-11-16T02:08:46Z

src/Microsoft.AspNet.Server.Kestrel/Infrastructure/MemoryPoolIterator2Extenstions.cs

+            }
+        }
+
+        private static string GetAsciiStringHeap(MemoryPoolBlock2 start, MemoryPoolIterator2 end, int inputOffset, int length)


In the name of code reuse:

private static unsafe string GetAsciiStringHeap(MemoryPoolBlock2 start, MemoryPoolIterator2 end, int inputOffset, int length) { var buffer = new char[length]; fixed (char* output = buffer) { return MultiBlockAsciiIter(output, start, end, inputOffset, length); } } private static unsafe string MultiBlockAsciiIter(char* output, MemoryPoolBlock2 start, MemoryPoolIterator2 end, int inputOffset, int length) { ... }

Much better, done

troydai · 2015-11-16T19:31:23Z

davidfowl · 2015-11-16T23:37:43Z

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

+                }
+                else
+                {
+                    requestUrlPath = pathBegin.GetAsciiString(pathEnd);


Always use Utf8 for the path.

Changed and reverted as discussed in outdated diff

benaadams · 2015-11-16T23:57:49Z

src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs

@@ -675,7 +675,7 @@ private bool TakeStartLine(SocketInput input)
                    pathEnd = UrlPathDecoder.Unescape(pathBegin, pathEnd);
                }

-                var requestUrlPath = pathBegin.GetString(pathEnd);
+                var requestUrlPath = pathBegin.GetUtf8String(pathEnd);


@davidfowl just told me to always Utf8 the path; was last commit change

Lol @benaadams wires crossed

It is utf8 because line 675 is already doing the first level hex un-escape.

Wasn't in previous commit: benaadams@6769e1e

Oooh, benaadams@6769e1e was better. Sorry about that.

@davidfowl revert?

Question is are paths always url encoded?

https://tools.ietf.org/html/rfc3986#page-11 seems to suggest so; but looking for a more recent one...

Internationalized Resource Identifiers (IRIs) https://www.ietf.org/rfc/rfc3987.txt "Mapping of IRIs to URIs"; says yes? Convert to utf8 then url-encode to ascii - so reverting would be a safe transformation?

benaadams · 2015-11-17T01:55:55Z

Adding some tests for this

Reduce GetString allocs and conversions

dnfclas added the cla-already-signed label Nov 1, 2015

davidfowl reviewed Nov 1, 2015
View reviewed changes

This was referenced Nov 1, 2015

[For Testing] Go faster stripes MkII - Collated changes #309

Closed

[Perf] High Allocator MemoryPoolIterator2.GetString #291

Closed

benaadams force-pushed the MemoryPoolIterator2-GetString branch from 702e560 to 6282d0b Compare November 1, 2015 09:43

benaadams closed this Nov 3, 2015

benaadams reopened this Nov 3, 2015

dnfclas added the cla-already-signed label Nov 3, 2015

benaadams force-pushed the MemoryPoolIterator2-GetString branch 2 times, most recently from 6b5e761 to 4161301 Compare November 3, 2015 10:20

benaadams closed this Nov 3, 2015

benaadams reopened this Nov 4, 2015

dnfclas added the cla-already-signed label Nov 4, 2015

benaadams force-pushed the MemoryPoolIterator2-GetString branch from 056bcd1 to 5de4169 Compare November 4, 2015 11:51

halter73 reviewed Nov 4, 2015
View reviewed changes

benaadams force-pushed the MemoryPoolIterator2-GetString branch 13 times, most recently from fee6712 to d88d4c3 Compare November 10, 2015 10:50

benaadams force-pushed the MemoryPoolIterator2-GetString branch from c3ca476 to 68d63ed Compare November 10, 2015 11:21

benaadams mentioned this pull request Nov 14, 2015

[For Testing] Bundled Memory Changes #386

Closed

benaadams force-pushed the MemoryPoolIterator2-GetString branch from 68d63ed to 7f50736 Compare November 14, 2015 23:06

benaadams force-pushed the MemoryPoolIterator2-GetString branch 3 times, most recently from 9d3760e to 53582f2 Compare November 15, 2015 11:42

benaadams force-pushed the MemoryPoolIterator2-GetString branch 2 times, most recently from 145e5d1 to 9bdc06a Compare November 15, 2015 12:21

davidfowl reviewed Nov 16, 2015
View reviewed changes

benaadams force-pushed the MemoryPoolIterator2-GetString branch from 05dea5d to 6769e1e Compare November 16, 2015 23:42

benaadams reviewed Nov 16, 2015
View reviewed changes

benaadams force-pushed the MemoryPoolIterator2-GetString branch from c18dd0f to 30fdee1 Compare November 17, 2015 01:33

Reduce GetString allocs and conversions

4dc4346

benaadams force-pushed the MemoryPoolIterator2-GetString branch from fc8e1fa to 4dc4346 Compare November 17, 2015 03:46

davidfowl added a commit that referenced this pull request Nov 17, 2015

Merge pull request #312 from benaadams/MemoryPoolIterator2-GetString

9c47796

Reduce GetString allocs and conversions

davidfowl merged commit 9c47796 into aspnet:dev Nov 17, 2015

benaadams deleted the MemoryPoolIterator2-GetString branch November 17, 2015 04:16

Reduce GetString allocs and conversions #312

Reduce GetString allocs and conversions #312

Uh oh!

Conversation

benaadams commented Nov 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 commented Nov 13, 2015

Uh oh!

davidfowl commented Nov 13, 2015

Uh oh!

benaadams commented Nov 13, 2015

Uh oh!

troydai commented Nov 13, 2015

Uh oh!

benaadams commented Nov 14, 2015

Uh oh!

benaadams commented Nov 15, 2015

Uh oh!

benaadams commented Nov 15, 2015

Uh oh!

benaadams commented Nov 15, 2015

Uh oh!

benaadams commented Nov 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troydai commented Nov 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Nov 17, 2015

Uh oh!

Uh oh!