Reduce the per message allocations #1630

davidfowl · 2018-03-17T02:58:43Z

Introduce Utf8BufferTextReader which is a simple TextReader implemented over a utf8 buffer. It encodes directly into the buffer allocated by JSON.NET.
Use JSON.NET's IArrayPool feature and back it with the shared array pool.

Baseline

             Method |          Input | HubProtocol |         Mean |       Error |       StdDev |       Median |        Op/s |   Gen 0 |  Gen 1 | Allocated |
------------------- |--------------- |------------ |-------------:|------------:|-------------:|-------------:|------------:|--------:|-------:|----------:|
  ReadSingleMessage |   FewArguments |        Json |   6,748.6 ns |   385.87 ns |  1,107.14 ns |   6,381.7 ns |   148,179.5 |  0.7172 | 0.0076 |    8728 B |
  ReadSingleMessage | LargeArguments |        Json | 137,546.7 ns | 4,607.99 ns | 13,514.45 ns | 136,188.9 ns |     7,270.3 | 18.0664 | 4.6387 |  217408 B | 
  ReadSingleMessage |  ManyArguments |        Json |   8,444.5 ns |   165.59 ns |    154.89 ns |   8,435.7 ns |   118,420.3 |  0.8698 | 0.0153 |   10600 B |
  ReadSingleMessage |    NoArguments |        Json |   4,381.1 ns |   211.40 ns |    613.31 ns |   4,278.5 ns |   228,254.2 |  0.6332 |      - |    7648 B |

This PR

             Method |          Input | HubProtocol |         Mean |        Error |       StdDev |       Median |        Op/s |  Gen 0 |  Gen 1 | Allocated |
------------------- |--------------- |------------ |-------------:|-------------:|-------------:|-------------:|------------:|-------:|-------:|----------:|
  ReadSingleMessage |   FewArguments |        Json |   5,576.3 ns |   127.120 ns |   197.911 ns |   5,510.1 ns |   179,331.1 | 0.2594 |      - |    3264 B |
  ReadSingleMessage | LargeArguments |        Json | 109,498.0 ns | 3,127.507 ns | 8,973.404 ns | 107,669.3 ns |     9,132.6 | 4.8828 | 0.4883 |   60960 B |
  ReadSingleMessage |  ManyArguments |        Json |   8,756.9 ns |   323.938 ns |   934.636 ns |   8,294.2 ns |   114,196.0 | 0.4120 |      - |    5096 B |
  ReadSingleMessage |    NoArguments |        Json |   3,712.5 ns |   162.313 ns |   470.900 ns |   3,570.0 ns |   269,359.2 | 0.1793 |      - |    2192 B |

Fixes #1618

- Introduce CharArrayTextReader which is a simple TextReader implemented over a utf8 buffer. It encodes directly into the buffer allocated by JSON.NET.

- Renamed CharArrayTextReader to Utf8BufferTextReader - Fixed length issue on netstandard version

JamesNK · 2018-03-17T23:37:26Z

src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs

-                }
+                var textReader = new Utf8BufferTextReader(payload);
+                messages.Add(ParseMessage(textReader, binder));
+


nit: remove line

JamesNK · 2018-03-17T23:38:09Z

src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/JsonHubProtocol.cs

        {
-            using (var reader = new JsonTextReader(new StreamReader(input)))
+            using (var reader = new JsonTextReader(input))


Add an internal helper method for creating a JsonTextReader with shared array pool?

JamesNK · 2018-03-17T23:51:47Z

src/Microsoft.AspNetCore.SignalR.Common/Internal/Protocol/Utf8BufferTextReader.cs

+    {
+        private ReadOnlyMemory<byte> _utf8Buffer;
+
+        public Utf8BufferTextReader(ReadOnlyMemory<byte> utf8Buffer)


Every time I read HubEndPoint and see the ToArray line 150 I don't like it. See below.

What about if HubProtocol took a sequence, which was then passed to this text reader. The logic in Utf8BufferTextReader would stay largely the same expect it could read over each memory block in the sequence instead of just one. We can avoid copying the sequence in its entirety into a new array.

SignalR/src/Microsoft.AspNetCore.SignalR.Core/HubEndPoint.cs

Lines 138 to 168 in 8b8c710

var result = await connection.Input.ReadAsync(connection.ConnectionAbortedToken);

var buffer = result.Buffer;

var consumed = buffer.End;

var examined = buffer.End;

try

{

if (!buffer.IsEmpty)

{

var hubMessages = new List<HubMessage>();

// TODO: Make this incremental

if (connection.Protocol.TryParseMessages(buffer.ToArray(), _dispatcher, hubMessages))

{

foreach (var hubMessage in hubMessages)

{

// Don't wait on the result of execution, continue processing other

// incoming messages on this connection.

_ = _dispatcher.DispatchMessageAsync(connection, hubMessage);

}

}

}

else if (result.IsCompleted)

{

break;

}

}

finally

{

connection.Input.AdvanceTo(consumed, examined);

}

This change will be made after @anurse 's refactoring. The problem is that right now the client and server are asymmetrical and they don't both parse ReadOnlySequence<byte>. Client uses a byte[] and the server uses ReadOnlySequence<byte>. This will be done as part of #1508.

The plan is to:

Move the reconnect logic to HubConnection instead of IConnection

Make IConnection single use, single reader and writer

Change the IConnection client to use pipelines

Change the parsers to be incremental

Change Utf8TextReader to take the ReadOnySequence<byte>directly from the pipe

I'm hoping after we do that, we're be blazingly fast on the client and server and will allocate very little.

Next after that will be object pooling of various non buffer things.

Could we make the server and Utf8BufferTextReader work with ReadOnlySequence<byte> now? On the client we can just wrap the byte[] in a ReadOnlySequence<byte> with little to no overhead before parsing, no? Memory and sequence are just structs.

I don't really see the point. I mean I actually want to do it, just not prematurely.

I also hate ToArray() with a passion but there is a master plan here and it's going to be glorious when it comes together. These are mostly incremental steps towards that shared goal. I expect to see dramatic performance improvements in the next couple of weeks.

davidfowl · 2018-03-18T07:05:50Z

Closing in favor of #1635

analogrelay

LGTM (post-merge)

analogrelay · 2018-03-19T16:28:27Z

client-ts/FunctionalTests/package-lock.json

@@ -7,13 +7,13 @@
    "@std/esm": {
      "version": "0.18.0",
      "resolved": "https://registry.npmjs.org/@std/esm/-/esm-0.18.0.tgz",
-      "integrity": "sha512-oeHSSVp/WxC08ngpKgyYR4LcI0+EBwZiJcB58jvIqyJnOGxudSkxTgAQKsVfpNsMXfOoILgu9PWhuzIZ8GQEjw==",
+      "integrity": "sha1-4hK1Zcdl+Tsp7FIqSToZLOeuK6Y=",


Upgrade your NPM bro

davidfowl requested review from analogrelay, BrennanConroy, JamesNK and mikaelm12 March 17, 2018 02:59

Reduce the per message allocations

128ce47

- Introduce CharArrayTextReader which is a simple TextReader implemented over a utf8 buffer. It encodes directly into the buffer allocated by JSON.NET.

davidfowl force-pushed the davidfowl/reduce-per-message branch from ac4e05d to 128ce47 Compare March 17, 2018 03:14

davidfowl added 2 commits March 17, 2018 00:35

Fixed bug and simplify the code

185e22b

- Renamed CharArrayTextReader to Utf8BufferTextReader - Fixed length issue on netstandard version

Use array pool to avoid allocating char[] buffers

6d54747

davidfowl mentioned this pull request Mar 17, 2018

[WIP]: Rewriting JsonHubProtocol to use JsonTextReader directly #1632

Closed

JamesNK reviewed Mar 17, 2018

View reviewed changes

This was referenced Mar 18, 2018

Look at using ObjectPool for various per message allocations #1634

Closed

Reduce the per message read allocations #1635

Merged

davidfowl closed this Mar 18, 2018

analogrelay reviewed Mar 19, 2018

View reviewed changes

davidfowl deleted the davidfowl/reduce-per-message branch March 28, 2018 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the per message allocations #1630

Reduce the per message allocations #1630

davidfowl commented Mar 17, 2018 •

edited

JamesNK Mar 17, 2018

JamesNK Mar 17, 2018

JamesNK Mar 17, 2018

davidfowl Mar 18, 2018 •

edited

JamesNK Mar 18, 2018 •

edited

davidfowl Mar 18, 2018 •

edited

davidfowl Mar 18, 2018 •

edited

davidfowl commented Mar 18, 2018

analogrelay left a comment

analogrelay Mar 19, 2018

	var result = await connection.Input.ReadAsync(connection.ConnectionAbortedToken);
	var buffer = result.Buffer;
	var consumed = buffer.End;
	var examined = buffer.End;

	try
	{
	if (!buffer.IsEmpty)
	{
	var hubMessages = new List<HubMessage>();

	// TODO: Make this incremental
	if (connection.Protocol.TryParseMessages(buffer.ToArray(), _dispatcher, hubMessages))
	{
	foreach (var hubMessage in hubMessages)
	{
	// Don't wait on the result of execution, continue processing other
	// incoming messages on this connection.
	_ = _dispatcher.DispatchMessageAsync(connection, hubMessage);
	}
	}
	}
	else if (result.IsCompleted)
	{
	break;
	}
	}
	finally
	{
	connection.Input.AdvanceTo(consumed, examined);
	}

Reduce the per message allocations #1630

Reduce the per message allocations #1630

Conversation

davidfowl commented Mar 17, 2018 • edited

Baseline

This PR

JamesNK Mar 17, 2018

Choose a reason for hiding this comment

JamesNK Mar 17, 2018

Choose a reason for hiding this comment

JamesNK Mar 17, 2018

Choose a reason for hiding this comment

davidfowl Mar 18, 2018 • edited

Choose a reason for hiding this comment

JamesNK Mar 18, 2018 • edited

Choose a reason for hiding this comment

davidfowl Mar 18, 2018 • edited

Choose a reason for hiding this comment

davidfowl Mar 18, 2018 • edited

Choose a reason for hiding this comment

davidfowl commented Mar 18, 2018

analogrelay left a comment

Choose a reason for hiding this comment

analogrelay Mar 19, 2018

Choose a reason for hiding this comment

davidfowl commented Mar 17, 2018 •

edited

davidfowl Mar 18, 2018 •

edited

JamesNK Mar 18, 2018 •

edited

davidfowl Mar 18, 2018 •

edited

davidfowl Mar 18, 2018 •

edited