Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

More Vector performance #1000

Closed
benaadams wants to merge 4 commits into from
Closed

More Vector performance #1000

benaadams wants to merge 4 commits into from

Conversation

@benaadams
Copy link
Contributor

@benaadams benaadams commented Jul 20, 2016

Now Unsafe is in nuget, re-opening of (minus CopyBlk, as not sure about perf on that)

#811
#804
#812

@benaadams benaadams changed the title Vector stuffs More Vector performance Jul 20, 2016
@halter73
Copy link
Member

halter73 commented Jul 21, 2016

Cool. Maybe we can get the System.Runtime.CompilerServices.Unsafe NuGet package a little above 125 downloads!

Based on the micro benchmarks I did a few months back, this should be about 39% faster than what we have today with some large (~60KB) headers mixed in, and 32% faster for more normal sized (~2.5KB) headers. I'll have to to rerun with this PR to make sure the numbers haven' changed before merging. We'll also run the plaintext benchmark just to make sure nothing is worse.

halter73
halter73 reviewed Jul 21, 2016
View reviewed changes
src/Microsoft.AspNetCore.Server.Kestrel/Internal/Infrastructure/MemoryPoolIterator.cs
var remaining = count;
while (true)
{
var following = block.End - index;
Copy link
Member

@halter73 halter73 Jul 21, 2016

Choose a reason for hiding this comment

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

While you might not need to use the wasLastBlock pattern since we want to skip it anyway, I don't feel to good about using a different pattern for traversing blocks in this one place.

It certainly risks returning an invalid actual out param. While this might never technically be used today by callers to CopyTo with a null array, I don't think it's OK for it to ever be wrong in case it is used.

Copy link
Contributor Author

@benaadams benaadams Jul 21, 2016

Choose a reason for hiding this comment

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

Changed to same pattern

@benaadams
Copy link
Contributor Author

benaadams commented Jul 21, 2016 •
edited
Loading

Rebased the feedback into commit 2 so it stays with the correct commit tag, my git fu is improving

@benaadams
Copy link
Contributor Author

benaadams commented Jul 22, 2016

There are some crazy cookie sizes in the wild 👍

@halter73
Copy link
Member

halter73 commented Jul 25, 2016

I really want the overall benchmarks for this PR to be better since the microbenchmarks for the header validation showed a nice improvement. At least with the plaintext benchmarks, there doesn't seem to be an improvement. Maybe we'll see an improvement with larger request and response headers.

BTW. Did you microbenchmark Unsafe.Read vs the normal Vector ctor. Obviously Unsafe.Read can have an array to begin with (like with the header strings), but I haven't measured the difference between using that vs the normal ctor with our blocks where we have both the pointer and the array.

benchmarks\src\BenchmarksDriver> dotnet run -- -n plaintext -s http://adx-3u-perfsvr:5001 -c http://adx-3u-svr02:5002 -o  "KestrelHttpServer@benaadams/vector-stuffs-merged"
Project Repository (.NETStandard,Version=v1.0) was previously compiled. Skipping compilation.
Project Benchmarks.ClientJob (.NETStandard,Version=v1.0) was previously compiled. Skipping compilation.
Project Benchmarks.ServerJob (.NETStandard,Version=v1.0) was previously compiled. Skipping compilation.
Project BenchmarksDriver (.NETCoreApp,Version=v1.0) was previously compiled. Skipping compilation.
[01:14:27.419] Starting scenario Plaintext on benchmark server...
[01:14:48.257] Warmup
[01:14:48.266] Starting scenario Plaintext on benchmark client...
[01:14:59.420] Scenario Plaintext completed on benchmark client
[01:14:59.420] RPS: 1005705.16
[01:14:59.421] Stopping scenario Plaintext on benchmark client...
[01:14:59.423] Benchmark
[01:14:59.423] Starting scenario Plaintext on benchmark client...
[01:15:10.467] Scenario Plaintext completed on benchmark client
[01:15:10.468] RPS: 1093077.62
[01:15:10.468] Stopping scenario Plaintext on benchmark client...
[01:15:10.471] Stopping scenario Plaintext on benchmark server...
benchmarks\src\BenchmarksDriver> dotnet run -- -n plaintext -s http://adx-3u-perfsvr:5001 -c http://adx-3u-svr02:5002 -o  "KestrelHttpServer@dev"
Project Repository (.NETStandard,Version=v1.0) was previously compiled. Skipping compilation.
Project Benchmarks.ClientJob (.NETStandard,Version=v1.0) was previously compiled. Skipping compilation.
Project Benchmarks.ServerJob (.NETStandard,Version=v1.0) was previously compiled. Skipping compilation.
Project BenchmarksDriver (.NETCoreApp,Version=v1.0) was previously compiled. Skipping compilation.
[01:15:24.546] Starting scenario Plaintext on benchmark server...
[01:15:45.182] Warmup
[01:15:45.195] Starting scenario Plaintext on benchmark client...
[01:15:56.320] Scenario Plaintext completed on benchmark client
[01:15:56.321] RPS: 1057693.46
[01:15:56.321] Stopping scenario Plaintext on benchmark client...
[01:15:56.323] Benchmark
[01:15:56.323] Starting scenario Plaintext on benchmark client...
[01:16:07.380] Scenario Plaintext completed on benchmark client
[01:16:07.380] RPS: 1129921.75
[01:16:07.381] Stopping scenario Plaintext on benchmark client...
[01:16:07.383] Stopping scenario Plaintext on benchmark server...

@benaadams
Copy link
Contributor Author

benaadams commented Jul 26, 2016

I'll do some standalone Unsafe vs new

@muratg
Copy link
Contributor

muratg commented Aug 17, 2016

@benaadams did you end up doing more benchmarking?

@benaadams
Copy link
Contributor Author

benaadams commented Nov 16, 2016

@halter73 @muratg some outliers which might be windows doing something or a quirk :( But mostly good

Pre

Method Mean StdDev Scaled RPS
ParsePlaintext 927.9056 ns 13.4866 ns 1.00 1,077,695.89
ParsePipelinedPlaintext 753.9081 ns 9.5989 ns 0.81 1,326,421.56
ParseLiveAspNet 5,015.1654 ns 71.4417 ns 5.41 199,395.22
ParsePipelinedLiveAspNet 4,684.1782 ns 30.5706 ns 5.05 213,484.62
ParseUnicode 8,448.2189 ns 145.9482 ns 9.11 118,368.15
ParseUnicodePipelined 8,247.4369 ns 95.3297 ns 8.89 121,249.79

Post

Method Mean StdDev Scaled RPS Change
ParsePlaintext 896.9537 ns 16.9184 ns 1.00 1,114,884.80 + 3.45%
ParsePipelinedPlaintext 755.1664 ns 12.9676 ns 0.84 1,324,211.48 - 0.16%
ParseLiveAspNet 4,868.1637 ns 32.2829 ns 5.43 205,416.26 + 3.02%
ParsePipelinedLiveAspNet 4,541.4938 ns 46.2298 ns 5.06 220,191.87 + 3.14%
ParseUnicode 8,435.4590 ns 102.8463 ns 9.41 118,547.19 + 0.15%
ParseUnicodePipelined 8,032.8591 ns 12.0597 ns 8.96 124,488.68 + 2.67%

@benaadams
Copy link
Contributor Author

benaadams commented Nov 16, 2016

Though it probably needs some more TLC; my understanding of how Vectors and the jit interact has changed considerably since I first did this; and it will likely clash horribly with the MemoryPoolIterator changes #1138

@halter73 halter73 mentioned this pull request Nov 18, 2016
Merged
@benaadams benaadams mentioned this pull request Jan 10, 2017
Closed
@benaadams
Copy link
Contributor Author

benaadams commented Jan 10, 2017

Not sure what MemoryPoolIterator will be like post Pipelines so moved the Header Validate to #1301

@davidfowl
Copy link
Member

davidfowl commented Feb 18, 2017

Closing because pipelines change makes this unmergable

@davidfowl davidfowl closed this Feb 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@benaadams @halter73 @muratg @davidfowl @dnfclas