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.

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

Don't allocate in BeginChunkBytes #5688

Merged

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Dec 18, 2018

Use the BufferWriter's Span instead

Think allocations are up by factor of 1024 in the benchmark (i.e. x IterationCount for the pre)

                Method | DataLength |      Mean |          Op/s |  Gen 0 | Allocated |
 --------------------- |----------- |----------:|--------------:|-------:|----------:|
- WriteBeginChunkBytes |          0 |  33.30 ns |  30,030,302.5 | 0.7324 |     40 KB |
+ WriteBeginChunkBytes |          0 |   8.09 ns | 123,639,689.1 | 0.7324 |       0 B |
- WriteBeginChunkBytes |          1 |  34.48 ns |  28,998,176.5 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |          1 |   8.40 ns | 119,093,216.2 | 0.6714 |       0 B |
- WriteBeginChunkBytes |         16 |  34.07 ns |  29,348,584.0 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |         16 |   9.01 ns | 110,966,446.7 | 0.6714 |       0 B |
- WriteBeginChunkBytes |        256 |  45.12 ns |  22,164,899.8 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |        256 |  10.81 ns |  92,489,663.4 | 0.6714 |       0 B |
- WriteBeginChunkBytes |       4096 |  33.05 ns |  30,257,577.0 | 0.7324 |     40 KB |
+ WriteBeginChunkBytes |       4096 |  11.54 ns |  86,654,067.4 | 0.7324 |       0 B |
- WriteBeginChunkBytes |      65536 |  45.85 ns |  21,809,105.6 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |      65536 |  12.69 ns |  78,783,888.6 | 0.6714 |       0 B |
- WriteBeginChunkBytes |    1048576 |  33.94 ns |  29,459,649.1 | 0.7324 |     40 KB |
+ WriteBeginChunkBytes |    1048576 |  13.92 ns |  71,856,593.8 | 0.7324 |       0 B |
- WriteBeginChunkBytes |   16777216 |  33.30 ns |  30,028,526.6 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |   16777216 |  15.35 ns |  65,169,537.2 | 0.6714 |       0 B |

/cc @pakrym @davidfowl

@davidfowl
Copy link
Member

@benaadams perf tests?

@benaadams
Copy link
Member Author

benaadams commented Dec 18, 2018

perf tests?

Don't think BDN is dividing allocations by iterations, howevers:

                Method | DataLength |      Mean |          Op/s |  Gen 0 | Allocated |
 --------------------- |----------- |----------:|--------------:|-------:|----------:|
- WriteBeginChunkBytes |          0 |  33.30 ns |  30,030,302.5 | 0.7324 |     40 KB |
+ WriteBeginChunkBytes |          0 |   8.09 ns | 123,639,689.1 | 0.7324 |       0 B |
- WriteBeginChunkBytes |          1 |  34.48 ns |  28,998,176.5 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |          1 |   8.40 ns | 119,093,216.2 | 0.6714 |       0 B |
- WriteBeginChunkBytes |         16 |  34.07 ns |  29,348,584.0 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |         16 |   9.01 ns | 110,966,446.7 | 0.6714 |       0 B |
- WriteBeginChunkBytes |        256 |  45.12 ns |  22,164,899.8 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |        256 |  10.81 ns |  92,489,663.4 | 0.6714 |       0 B |
- WriteBeginChunkBytes |       4096 |  33.05 ns |  30,257,577.0 | 0.7324 |     40 KB |
+ WriteBeginChunkBytes |       4096 |  11.54 ns |  86,654,067.4 | 0.7324 |       0 B |
- WriteBeginChunkBytes |      65536 |  45.85 ns |  21,809,105.6 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |      65536 |  12.69 ns |  78,783,888.6 | 0.6714 |       0 B |
- WriteBeginChunkBytes |    1048576 |  33.94 ns |  29,459,649.1 | 0.7324 |     40 KB |
+ WriteBeginChunkBytes |    1048576 |  13.92 ns |  71,856,593.8 | 0.7324 |       0 B |
- WriteBeginChunkBytes |   16777216 |  33.30 ns |  30,028,526.6 | 0.6714 |     40 KB |
+ WriteBeginChunkBytes |   16777216 |  15.35 ns |  65,169,537.2 | 0.6714 |       0 B |


namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
{
internal static class ChunkWriter
{
private static readonly ArraySegment<byte> _endChunkBytes = CreateAsciiByteArraySegment("\r\n");
private static readonly byte[] _hex = Encoding.ASCII.GetBytes("0123456789abcdef");
Copy link
Contributor

@pentp pentp Dec 20, 2018

Choose a reason for hiding this comment

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

This could be changed to a ReadOnlySpan<byte> returning property (or inlined in the method) - it would be compiled as just static data and startHex will be a constant then. This would get rid of the static .cctor also.

var hex = new ReadOnlySpan<byte>(new byte[] {(byte)'0', (byte)'1', (byte)'2',/*...*/ (byte)'e', (byte)'f' });
ref var startHex = ref Unsafe.AsRef(in hex[0]);

Sharplab example

@pakrym pakrym closed this Dec 26, 2018
@pakrym pakrym reopened this Dec 26, 2018
@benaadams benaadams changed the title Don't allocate in BeginChunkBytes [wip] Don't allocate in BeginChunkBytes Dec 26, 2018
@benaadams benaadams changed the title [wip] Don't allocate in BeginChunkBytes Don't allocate in BeginChunkBytes Dec 26, 2018
@benaadams
Copy link
Member Author

benaadams commented Dec 26, 2018

2 unrelated errors
#6135

Microsoft.AspNetCore.FunctionalTests.Microsoft.AspNetCore.Tests.WebHostFunctionalTests.RunsInIISExpressInProcess
Assert.Equal() Failure
          ↓ (pos 0)
Expected: CreateDefaultBuilderApp
Actual:   <!DOCTYPE html>\r\n<html lang="en-US" ns···
          ↑ (pos 0)
   at Microsoft.AspNetCore.Tests.WebHostFunctionalTests.RunsInIISExpressInProcess()
   in /_/src/DefaultBuilder/test/Microsoft.AspNetCore.FunctionalTests/WebHostFunctionalTests.cs:line 179
--- End of stack trace from previous location where exception was thrown ---

#6136

ServerComparison.FunctionalTests.ResponseTests.ResponseFormats_Chunked(variant: 
 Server: IISExpress, TFM: netcoreapp3.0, Type: Portable, Arch: x64, ANCM: V2, Host: InProcess)
/chunked, chunked?
Expected: True
Actual:   (null)
   at ServerComparison.FunctionalTests.ResponseTests.CheckChunkedAsync(HttpClient client, ILogger logger)
    in /_/src/Servers/test/FunctionalTests/ResponseTests.cs:line 211
   at ServerComparison.FunctionalTests.ResponseTests.ResponseFormats(TestVariant variant, Func`3 scenario, String testName)
    in /_/src/Servers/test/FunctionalTests/ResponseTests.cs:line 125
--- End of stack trace from previous location where exception was thrown ---

@benaadams
Copy link
Member Author

Opened issues for the errors #6135 and #6136

@pakrym
Copy link
Contributor

pakrym commented Dec 26, 2018

While RunsInIISExpressInProcess constantly fails, I've never seen ResponseFormats_Chunked failing in other RP. Doesn't seem to be connected at all but I'll restart it again.

@pakrym pakrym closed this Dec 26, 2018
@pakrym pakrym reopened this Dec 26, 2018
@pakrym
Copy link
Contributor

pakrym commented Dec 26, 2018

@pakrym pakrym merged commit cb1917a into dotnet:master Dec 27, 2018
@benaadams benaadams deleted the don't-allocate-in-WriteBeginChunkBytes branch December 27, 2018 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants