Use System.Text.Json for Negotiate and Handshake #6977

BrennanConroy · 2019-01-24T05:52:05Z

Before:

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
WriteResponse_MemoryBufferWriter	1,365.1 ns	28.49 ns	41.76 ns	732,543.9	0.0038	456 B
ParsingNegotiateResponseMessageSuccessForValid1	1,055.5 ns	27.91 ns	82.29 ns	947,388.5	0.0458	3928 B
ParsingNegotiateResponseMessageSuccessForValid2	853.0 ns	20.59 ns	60.72 ns	1,172,387.5	0.0429	3696 B
ParsingNegotiateResponseMessageSuccessForValid3	1,019.7 ns	20.33 ns	52.12 ns	980,699.7	0.0439	3776 B
ParsingNegotiateResponseMessageSuccessForValid4	1,491.3 ns	29.80 ns	64.78 ns	670,546.6	0.0477	4176 B
ParsingNegotiateResponseMessageSuccessForValid5	1,826.6 ns	39.14 ns	95.26 ns	547,463.8	0.0496	4384 B

After:

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
WriteResponse_MemoryBufferWriter	487.2 ns	8.223 ns	7.692 ns	2,052,423.5	0.0010	144 B
ParsingNegotiateResponseMessageSuccessForValid1	356.4 ns	7.551 ns	20.284 ns	2,806,082.7	0.0010	120 B
ParsingNegotiateResponseMessageSuccessForValid2	252.7 ns	5.078 ns	7.905 ns	3,957,506.2	0.0014	120 B
ParsingNegotiateResponseMessageSuccessForValid3	411.9 ns	8.259 ns	19.468 ns	2,427,993.3	0.0014	152 B
ParsingNegotiateResponseMessageSuccessForValid4	667.2 ns	14.815 ns	40.556 ns	1,498,818.5	0.0029	272 B
ParsingNegotiateResponseMessageSuccessForValid5	1,038.2 ns	51.740 ns	152.557 ns	963,224.3	0.0048	480 B

cc @ahsonkhan

...gnalR/common/Http.Connections.Common/src/Microsoft.AspNetCore.Http.Connections.Common.csproj

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

...gnalR/common/Http.Connections.Common/src/Microsoft.AspNetCore.Http.Connections.Common.csproj

ahsonkhan · 2019-01-24T06:29:23Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

@@ -13,77 +15,166 @@ namespace Microsoft.AspNetCore.Http.Connections
    public static class NegotiateProtocol
    {
        private const string ConnectionIdPropertyName = "connectionId";
+        private static readonly byte[] ConnectionIdPropertyNameBytes = Encoding.UTF8.GetBytes("connectionId");


Consider re-using the const strings to avoid having to change them in multiple places.

private static readonly byte[] ConnectionIdPropertyNameBytes = Encoding.UTF8.GetBytes(ConnectionIdPropertyName);

OR, if performance is really critical, pre-compute the UTF-8 bytes and store them as byte arrays whose values are known at compile-time.

For example, for "url":

private static ReadOnlySpan<byte> ConnectionIdPropertyNameBytes => new byte [3] {117, 114, 108}; // OR private static ReadOnlySpan<byte> ConnectionIdPropertyNameBytes => new byte[3] { (byte)'u', (byte)'r', (byte)'l' };

Though I wouldn't go that far. It hurts readability too much.

If that's something we decide is worthwhile, we would want to change a bunch of type in Kestrel like HttpProtocol to pre-compute bytes as well.

ahsonkhan · 2019-01-24T06:39:27Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs


        public static void WriteResponse(NegotiationResponse response, IBufferWriter<byte> output)
        {
-            var textWriter = Utf8BufferTextWriter.Get(output);
-            try
+            var writer = new Utf8JsonWriter(output, new JsonWriterState(new JsonWriterOptions() { SkipValidation = true }));


nit:

var writer = new Utf8JsonWriter(output, new JsonWriterState(options: new JsonWriterOptions { SkipValidation = true }));

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

ahsonkhan · 2019-01-24T06:43:28Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

+            writer.Flush(isFinalBlock: true);
+        }
+
+        public static NegotiationResponse ParseResponse(byte[] content)


I would be curious to see the performance of using the JsonDocument in this case compared to the Utf8JsonReader.

I gave JsonDocument a quick try, although I'm definitely no expert on it's usage:

Method Mean Error StdDev Op/s Gen 0 Allocated

ParsingNegotiateResponseMessageSuccessForValid1 1,252.6 ns 24.96 ns 24.510 ns 798,312.5 0.0057 528 B

ParsingNegotiateResponseMessageSuccessForValid2 877.6 ns 14.30 ns 9.462 ns 1,139,434.6 0.0038 368 B

ParsingNegotiateResponseMessageSuccessForValid3 1,096.4 ns 21.74 ns 26.694 ns 912,048.6 0.0057 536 B

ParsingNegotiateResponseMessageSuccessForValid4 2,365.0 ns 46.47 ns 63.604 ns 422,835.1 0.0076 944 B

ParsingNegotiateResponseMessageSuccessForValid5 2,688.2 ns 52.91 ns 49.489 ns 372,001.7 0.0114 1208 B

Thanks for giving it a try. Do you happen to have a commit with the code change to use JsonDocument that I can look at?

I didn't save it, but it was fairly quick so I can make one for you.

Take a look at https://github.com/aspnet/AspNetCore/compare/brecon/jsonNeg...brecon/jsonDoc?expand=1

This time I noticed the TryGetProperty method, so it was a little bit better.
~~The ToArray in JsonDocument.Parse should be avoidable if an overload was added to support ReadOnlySpan<byte>.~~

Side by side numbers:
With JsonDocument

Method Mean Error StdDev Op/s Gen 0 Allocated

ParsingNegotiateResponseMessageSuccessForValid1 1,495.7 ns 31.17 ns 65.75 ns 668,580.0 0.0019 264 B

ParsingNegotiateResponseMessageSuccessForValid2 1,070.6 ns 22.78 ns 43.34 ns 934,042.2 0.0019 248 B

ParsingNegotiateResponseMessageSuccessForValid3 1,314.7 ns 26.21 ns 55.86 ns 760,646.2 0.0019 304 B

ParsingNegotiateResponseMessageSuccessForValid4 2,482.5 ns 49.28 ns 88.86 ns 402,823.8 0.0038 456 B

ParsingNegotiateResponseMessageSuccessForValid5 2,820.7 ns 55.10 ns 99.36 ns 354,521.8 0.0038 720 B

With Utf8JsonReader

Method Mean Error StdDev Op/s Gen 0 Allocated

ParsingNegotiateResponseMessageSuccessForValid1 404.7 ns 8.122 ns 13.79 ns 2,470,926.1 0.0010 120 B

ParsingNegotiateResponseMessageSuccessForValid2 305.5 ns 6.168 ns 15.13 ns 3,273,484.3 0.0010 120 B

ParsingNegotiateResponseMessageSuccessForValid3 461.3 ns 9.164 ns 18.09 ns 2,167,912.7 0.0014 152 B

ParsingNegotiateResponseMessageSuccessForValid4 752.4 ns 14.997 ns 29.95 ns 1,329,015.3 0.0029 272 B

ParsingNegotiateResponseMessageSuccessForValid5 1,101.3 ns 22.009 ns 60.25 ns 908,003.1 0.0038 480 B

The ToArray() isn't great :(

Can you try copying it to an array pool-rented array instead and share how much that helps?

// BEFORE: using (var doc = JsonDocument.Parse(content.ToArray(), readerOptions: default))

// AFTER: byte[] pooledBuffer = ArrayPool<byte>.Shared.Rent(content.Length); content.CopyTo(pooledBuffer); using (var doc = JsonDocument.Parse(pooledBuffer, readerOptions: default)) { ... } ArrayPool<byte>.Shared.Return(pooledBuffer);

I expected a ~2x regression between reader and document, not 3-4x.

Better yet, can we change the API signature to accept ReadOnlyMemory<byte> instead to avoid having to copy data at all? It looks like the only caller passes in a byte[] anyway:

public static NegotiationResponse ParseResponse(ReadOnlySpan<byte> content)

// From: HttpConnection.NegotiateAsync var negotiateResponse = NegotiateProtocol.ParseResponse(responseBuffer);

https://github.com/aspnet/AspNetCore/pull/6977/files#diff-041ab9d757596cece2ef39f92ede7e4dR443

Method Mean Error StdDev Op/s Gen 0 Allocated

ParsingNegotiateResponseMessageSuccessForValid1 1,436.9 ns 27.89 ns 42.59 ns 695,933.7 - 192 B

ParsingNegotiateResponseMessageSuccessForValid2 1,052.9 ns 33.93 ns 89.98 ns 949,797.0 0.0019 192 B

ParsingNegotiateResponseMessageSuccessForValid3 1,268.1 ns 27.48 ns 43.59 ns 788,606.6 0.0019 224 B

ParsingNegotiateResponseMessageSuccessForValid4 2,417.6 ns 46.92 ns 80.93 ns 413,630.6 0.0038 344 B

ParsingNegotiateResponseMessageSuccessForValid5 2,756.2 ns 54.63 ns 78.35 ns 362,818.5 0.0038 552 B

With no ToArray

Thanks for sharing the numbers. They provide useful insight.

ahsonkhan · 2019-01-24T06:45:47Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

+            if (reader.TokenType == JsonTokenType.StartObject || reader.TokenType == JsonTokenType.StartArray)
+            {
+                int depth = reader.CurrentDepth;
+                while (reader.Read() && depth < reader.CurrentDepth)


This might need to be depth <= reader.CurrentDepth

At least that's what I observed in https://github.com/dotnet/core-setup/pull/5009/files#diff-02337f8b4f14d01b128b474cc5a5cf2aR98

It depends on whether you have moved past the property name (and into start object/start array, and already incremented the depth) or not.

If you haven't observed a start object/array before calling Skip, then keeping it as is fine.

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

ahsonkhan

Otherwise, LGTM

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

Tornhoof · 2019-01-24T09:04:33Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

+            }
+        }
+
+        private static string GetTokenString(JsonTokenType tokenType)


Why is this method necessary at all?
The returned special case strings have the same value as the enum names?

Tornhoof · 2019-01-24T09:43:16Z

May I suggest considering special casing the project for netcoreapp3.0 and then using the codegen serializer currently under development and leaving the .netstandard 2.0 path as it is?
Reasons:

You still have JSON.NET code via the Stream method code path
The codegen serializer will most likely have comparable or even faster performance compared to the manually written code, specifically if the order of properties in the JSON in deserialization is not the same as the one in your code (as soon as your memberName.SequenceEqual(ErrorPropertyNameBytes) misses a branch, you'll be certainly slower than the ilasm integer tricks from Routing)

I understand that you all want to play with your new toys, but I think that code gets a bit out of hand.

davidfowl · 2019-01-24T15:31:00Z

This isn’t about playing with toys. We’re removing JSON.NET from the shared framework so everything that uses it by default needs to be decoupled. This is step 0.

Tornhoof · 2019-01-24T16:04:23Z

This isn’t about playing with toys. We’re removing JSON.NET from the shared framework so everything that uses it by default needs to be decoupled. This is step 0.

You want to get rid of it in the .net standard 2.0 codepaths too?, that's news to me, but that explains it.

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

BrennanConroy · 2019-01-24T23:01:06Z

Failing test is because of https://github.com/dotnet/corefx/issues/34632

…nection.cs Co-Authored-By: BrennanConroy <brecon@microsoft.com>

ahsonkhan · 2019-01-25T03:04:38Z

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs

-        private static AvailableTransport ParseAvailableTransport(JsonTextReader reader)
+        public static NegotiationResponse ParseResponse(Stream content)
+        {
+            var buffer = new byte[content.Length];


Is it guaranteed for the stream to be seekable here? If so, add Debug.Assert(content.CanSeek)? Also, do we need other arg validation like a null check?

I was thinking of marking this function as obsolete.

Even being marked as obsolete, this implementation seems worse than just throwing a NotImplementedException. Even if the Stream is seekable and has a Length, I don't think it's guaranteed the entire content is buffered and ready to go. Stream.Read() could block or return only partial content.

Do we include calls to this method in any of our samples, demos or docs?

I'm not sure why this class is public, I'm guessing it was a mistake.

It's not that big of a deal, hopefully it won't matter in the long run. Mark it obselete and we can carry on with our lives.

I'm suggesting doing anything complicated, just throwing a NotImplementedException. It seems better to do that than to introduce more subtle issues.

...gnalR/common/Http.Connections.Common/src/Microsoft.AspNetCore.Http.Connections.Common.csproj

src/SignalR/common/Shared/SystemTextJsonUtils.cs

src/SignalR/common/Shared/TextMessageParser.cs

BrennanConroy · 2019-01-27T07:33:31Z

@ahsonkhan What's with ValueSpan vs ValueSequence? A brief glance at the source looks like ValueSequence is never modified?

ahsonkhan · 2019-01-28T01:58:32Z

What's with ValueSpan vs ValueSequence? A brief glance at the source looks like ValueSequence is never modified?

Take a look at Utf8JsonReader.MultiSegment.cs. The only time ValueSequence is set/modified is when your input is within a ReadOnlySequence and your JSON token straddles more than 1 segment (for example a JSON string starts in one segment and ends in another). https://github.com/dotnet/corefx/blob/eec001d96a68376c0e504eb7635c8edec196f90f/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs#L576

In those cases, you shouldn't access the JSON element via ValueSpan directly (since it won't always contain the value). Instead, check HasValueSequence to see where the value lives.

ReadOnlySpan<byte> value = reader.HasValueSequence ? 
   reader.ValueSequence.ToArray() : 
   reader.ValueSpan;

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

ahsonkhan · 2019-01-28T02:04:25Z

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

+            }
+        }
+
+        public static string ReadAsString(this ref Utf8JsonReader reader, byte[] propertyName)


Similar to ReadAsInt32, if the string can be null, you need to check for TokenType.Null up front first to return null.

This will fail and throw an exception for null values. Poll the TokenType for the null JSON literal, not the ValueSpan.

public static string ReadAsString(this ref Utf8JsonReader reader, byte[] propertyName) { reader.Read(); if (reader.TokenType == JsonTokenType.Null) { return null; } if (reader.TokenType != JsonTokenType.String) { throw new InvalidDataException($"Expected '{Encoding.UTF8.GetString(propertyName)}' to be of type {JsonTokenType.String}."); } return reader.GetString(); }

I can't think of a situation where we want to accept a null string in the hub protocol.

We explicitly have tests that check for the parsing to fail if null is passed in.

Add a Debug.Assert(reader.TokenType != JsonTokenType.Null) to be explicit?

src/SignalR/common/SignalR.Common/src/Protocol/HandshakeProtocol.cs

BrennanConroy · 2019-01-28T19:58:47Z

Take a look at Utf8JsonReader.MultiSegment.cs.

Ah, so it is modified! I'll do the needful to handle ValueSequence then.

davidfowl · 2019-01-28T20:28:00Z

Ah, so it is modified! I'll do the needful to handle ValueSequence then.

Why do you need to handle it if you never pass in a ReadOnlySequence?

BrennanConroy · 2019-01-28T20:33:48Z

We do pass in a ReadOnlySequence for Handshakes

JamesNK · 2019-01-29T05:17:56Z

It is always working now because handshakes are so small that they fit in one segment. To fix and test you will need to make each segment tiny.

BrennanConroy · 2019-01-29T05:20:58Z

I know ;)

JamesNK · 2019-01-29T21:40:04Z

src/SignalR/common/SignalR.Common/src/Protocol/HandshakeProtocol.cs

-
-                    string protocol = null;
-                    int? protocolVersion = null;
+                    var memberName = reader.HasValueSequence ? reader.ValueSequence.ToArray() : reader.ValueSpan;


@ahsonkhan Have you considered having a helper method for comparing the current value that checks this logic for you? It would help avoid developers making the mistake of not using ValueSequence, and it could do something more efficient than allocating an array when doing the compare.

https://github.com/dotnet/corefx/issues/34938

I initially only had ValueSpan and would call ToArray on the ReadOnlySequence slice which circumvents this usability issue. However, it results in unnecessary allocations for JSON elements the user just wants to read past and not materialize/look into. So, we decided to leave the allocation to the caller since they have the context on when they actually care about the value or not.

it could do something more efficient than allocating an array when doing the compare.

We could consider providing such a helper for compares. That is interesting feedback :) I will investigate.

The concern I have is, if the compare returns true, the user would have to call ToArray on the ReadOnlySequence anyway, so now we walk the sequence twice (once for the helper to check/compare, and if true, the caller materializing the value to some string/byte[]). We could save on allocations (by potentially copying to an array pool or some custom EqualToSequence method), but I don't know if it helps performance. How often is the search true versus false?

Testing the property name, like is done here, is an example of where you only care about whether it matches or not.

Before:

var memberName = reader.HasValueSequence ? reader.ValueSequence.ToArray() : reader.ValueSpan; if (memberName.SequenceEqual(ProtocolPropertyNameBytes)) { protocol = reader.ReadAsString(ProtocolPropertyNameBytes); } else if (memberName.SequenceEqual(ProtocolVersionPropertyNameBytes)) { protocolVersion = reader.ReadAsInt32(ProtocolVersionPropertyNameBytes); } else { reader.Skip(); }

After:

if (reader.ValueEquals(ProtocolPropertyNameBytes)) { protocol = reader.ReadAsString(ProtocolPropertyNameBytes); } else if (reader.ValueEquals(ProtocolVersionPropertyNameBytes)) { protocolVersion = reader.ReadAsInt32(ProtocolVersionPropertyNameBytes); } else { reader.Skip(); }

So you've already written ReadOnlySequence.SequenceEqual and calling that only when HasValueSequence is true slows down the single-segment case significantly? If you were to do something like the following, I hope the single-segment case wouldn't take too much of a perf hit:

[MethodImpl(MethodImplOptions.AggressiveInlining)] private static bool ValueEquals(in Utf8JsonReader reader, Span<byte> value) { return reader.HasValueSequence ? reader.ValueSequence.SequenceEqual(value) : reader.ValueSpan.SequenceEqual(value); }

I'll give ValueEquals a try, maybe I was doing something dumb.

IMO don't worry about optimizing this in SignalR. These messages are always going to be in one segment. I think this is an improvement for Utf8JsonReader, and SignalR could consume them later.

I agree that it's not important for this change. I'm just curious why @BrennanConroy's helper function is slower. I'm thinking maybe it's the Utf8JsonReader parameter getting copied.

Just looking at the example provided, using a ValueEquals like method where you are doing multiple compares on the same value, I don't find it surprising that it is slower. You went from a single HasValueSequence check to moving that check for each compare.

Also, now you have to walk the sequence for each of the n compares. If instead we called ToArray() on the ROSequence on it, it results in a single walk (granted it allocates), but then we can call the vectorized, span SequenceEqual which should be faster (and we can avoid the n walks). The question is how many segments does the value straddle? Most likely it would be 1 (when it isn't 0), so maybe segment walking isn't too bad. We could CopyTo a pooled array rather than calling ToArray() to reduce the allocation, but I don't know if it would be more efficient.

BrennanConroy · 2019-01-30T21:50:55Z

Ping, is there anymore actionable feedback here? Or can I get approval(s)?

JamesNK

Maybe these should be CheckRead to handle JSON ending unexpectedly. I'll leave to you.

JamesNK · 2019-01-30T22:04:02Z

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

+        {
+            if (reader.TokenType == JsonTokenType.PropertyName)
+            {
+                reader.Read();


JamesNK · 2019-01-30T22:04:06Z

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

+            if (reader.TokenType == JsonTokenType.StartObject || reader.TokenType == JsonTokenType.StartArray)
+            {
+                int depth = reader.CurrentDepth;
+                while (reader.Read() && depth < reader.CurrentDepth)


JamesNK · 2019-01-30T22:04:12Z

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

+
+        public static string ReadAsString(this ref Utf8JsonReader reader, byte[] propertyName)
+        {
+            reader.Read();


JamesNK · 2019-01-30T22:04:16Z

src/SignalR/common/Shared/SystemTextJsonExtensions.cs

+
+        public static int? ReadAsInt32(this ref Utf8JsonReader reader, byte[] propertyName)
+        {
+            reader.Read();


davidfowl

LGTM

BrennanConroy added the area-signalr Includes: SignalR clients and servers label Jan 24, 2019

BrennanConroy requested review from halter73, davidfowl and mikaelm12 January 24, 2019 05:52

ahsonkhan reviewed Jan 24, 2019

View reviewed changes

...gnalR/common/Http.Connections.Common/src/Microsoft.AspNetCore.Http.Connections.Common.csproj Outdated Show resolved Hide resolved

davidfowl reviewed Jan 24, 2019

View reviewed changes

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 24, 2019

View reviewed changes

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 24, 2019

View reviewed changes

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 24, 2019

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 24, 2019

View reviewed changes

...gnalR/common/Http.Connections.Common/src/Microsoft.AspNetCore.Http.Connections.Common.csproj Outdated Show resolved Hide resolved

ahsonkhan reviewed Jan 24, 2019

View reviewed changes

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs Show resolved Hide resolved

ahsonkhan approved these changes Jan 24, 2019

View reviewed changes

JamesNK reviewed Jan 24, 2019

View reviewed changes

src/SignalR/common/Http.Connections.Common/src/NegotiateProtocol.cs Outdated Show resolved Hide resolved

Tornhoof reviewed Jan 24, 2019

View reviewed changes

TheBlueSky reviewed Jan 24, 2019

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Outdated Show resolved Hide resolved

TheBlueSky reviewed Jan 24, 2019

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Outdated Show resolved Hide resolved

BrennanConroy and others added 5 commits January 24, 2019 18:50

WIP use System.Text.Json for Negotiate

06dbae6

some fb

27e548a

fb, remove newtonsoft

286c439

Update src/SignalR/clients/csharp/Http.Connections.Client/src/HttpCon…

6479272

…nection.cs Co-Authored-By: BrennanConroy <brecon@microsoft.com>

fix

7fbb273

BrennanConroy force-pushed the brecon/jsonNeg branch from 9f9045a to 7fbb273 Compare January 25, 2019 02:56

ahsonkhan reviewed Jan 25, 2019

View reviewed changes

davidfowl reviewed Jan 25, 2019

View reviewed changes

...gnalR/common/Http.Connections.Common/src/Microsoft.AspNetCore.Http.Connections.Common.csproj Outdated Show resolved Hide resolved

utils

a0a9086

davidfowl reviewed Jan 27, 2019

View reviewed changes

src/SignalR/common/Shared/SystemTextJsonUtils.cs Outdated Show resolved Hide resolved

handshake and extension methods

1d1e220

BrennanConroy changed the title ~~Use System.Text.Json for Negotiate~~ Use System.Text.Json for Negotiate and Handshake Jan 27, 2019

rename

50ffe23

davidfowl reviewed Jan 27, 2019

View reviewed changes

src/SignalR/common/Shared/TextMessageParser.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 27, 2019

View reviewed changes

src/SignalR/common/Shared/TextMessageParser.cs Show resolved Hide resolved

split

51be998

ahsonkhan reviewed Jan 28, 2019

View reviewed changes

src/SignalR/common/Shared/SystemTextJsonExtensions.cs Outdated Show resolved Hide resolved

ahsonkhan reviewed Jan 28, 2019

View reviewed changes

src/SignalR/common/SignalR.Common/src/Protocol/HandshakeProtocol.cs Outdated Show resolved Hide resolved

ahsonkhan reviewed Jan 28, 2019

View reviewed changes

src/SignalR/common/SignalR.Common/src/Protocol/HandshakeProtocol.cs Outdated Show resolved Hide resolved

natemcmaster removed their request for review January 28, 2019 17:05

valuesequence

796f253

JamesNK reviewed Jan 29, 2019

View reviewed changes

ahsonkhan mentioned this pull request Jan 30, 2019

Seed the porting guide for going from Json.NET to System.Text.Json. dotnet/corefx#34944

Merged

not supported

2c6533e

JamesNK approved these changes Jan 30, 2019

View reviewed changes

ahsonkhan approved these changes Jan 31, 2019

View reviewed changes

davidfowl approved these changes Jan 31, 2019

View reviewed changes

BrennanConroy merged commit dbf82dc into master Jan 31, 2019

BrennanConroy deleted the brecon/jsonNeg branch January 31, 2019 19:38

natemcmaster mentioned this pull request Jan 31, 2019

Remove Newtonsoft.Json from the shared framework #4260

Closed

7 tasks

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
ParsingNegotiateResponseMessageSuccessForValid1	1,252.6 ns	24.96 ns	24.510 ns	798,312.5	0.0057	528 B
ParsingNegotiateResponseMessageSuccessForValid2	877.6 ns	14.30 ns	9.462 ns	1,139,434.6	0.0038	368 B
ParsingNegotiateResponseMessageSuccessForValid3	1,096.4 ns	21.74 ns	26.694 ns	912,048.6	0.0057	536 B
ParsingNegotiateResponseMessageSuccessForValid4	2,365.0 ns	46.47 ns	63.604 ns	422,835.1	0.0076	944 B
ParsingNegotiateResponseMessageSuccessForValid5	2,688.2 ns	52.91 ns	49.489 ns	372,001.7	0.0114	1208 B

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
ParsingNegotiateResponseMessageSuccessForValid1	1,495.7 ns	31.17 ns	65.75 ns	668,580.0	0.0019	264 B
ParsingNegotiateResponseMessageSuccessForValid2	1,070.6 ns	22.78 ns	43.34 ns	934,042.2	0.0019	248 B
ParsingNegotiateResponseMessageSuccessForValid3	1,314.7 ns	26.21 ns	55.86 ns	760,646.2	0.0019	304 B
ParsingNegotiateResponseMessageSuccessForValid4	2,482.5 ns	49.28 ns	88.86 ns	402,823.8	0.0038	456 B
ParsingNegotiateResponseMessageSuccessForValid5	2,820.7 ns	55.10 ns	99.36 ns	354,521.8	0.0038	720 B

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
ParsingNegotiateResponseMessageSuccessForValid1	404.7 ns	8.122 ns	13.79 ns	2,470,926.1	0.0010	120 B
ParsingNegotiateResponseMessageSuccessForValid2	305.5 ns	6.168 ns	15.13 ns	3,273,484.3	0.0010	120 B
ParsingNegotiateResponseMessageSuccessForValid3	461.3 ns	9.164 ns	18.09 ns	2,167,912.7	0.0014	152 B
ParsingNegotiateResponseMessageSuccessForValid4	752.4 ns	14.997 ns	29.95 ns	1,329,015.3	0.0029	272 B
ParsingNegotiateResponseMessageSuccessForValid5	1,101.3 ns	22.009 ns	60.25 ns	908,003.1	0.0038	480 B

Method	Mean	Error	StdDev	Op/s	Gen 0	Allocated
ParsingNegotiateResponseMessageSuccessForValid1	1,436.9 ns	27.89 ns	42.59 ns	695,933.7	-	192 B
ParsingNegotiateResponseMessageSuccessForValid2	1,052.9 ns	33.93 ns	89.98 ns	949,797.0	0.0019	192 B
ParsingNegotiateResponseMessageSuccessForValid3	1,268.1 ns	27.48 ns	43.59 ns	788,606.6	0.0019	224 B
ParsingNegotiateResponseMessageSuccessForValid4	2,417.6 ns	46.92 ns	80.93 ns	413,630.6	0.0038	344 B
ParsingNegotiateResponseMessageSuccessForValid5	2,756.2 ns	54.63 ns	78.35 ns	362,818.5	0.0038	552 B

Use System.Text.Json for Negotiate and Handshake #6977

Use System.Text.Json for Negotiate and Handshake #6977

Conversation

BrennanConroy commented Jan 24, 2019

Choose a reason for hiding this comment

ahsonkhan Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tornhoof commented Jan 24, 2019

davidfowl commented Jan 24, 2019

Tornhoof commented Jan 24, 2019

BrennanConroy commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented Jan 27, 2019

ahsonkhan commented Jan 28, 2019 • edited Loading

ahsonkhan Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented Jan 28, 2019

davidfowl commented Jan 28, 2019 • edited Loading

BrennanConroy commented Jan 28, 2019

JamesNK commented Jan 29, 2019

BrennanConroy commented Jan 29, 2019

JamesNK Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

BrennanConroy Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

JamesNK Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy commented Jan 30, 2019

JamesNK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

ahsonkhan Jan 24, 2019 •

edited

Loading

BrennanConroy Jan 30, 2019 •

edited

Loading

ahsonkhan Jan 31, 2019 •

edited

Loading

ahsonkhan Jan 24, 2019 •

edited

Loading

ahsonkhan commented Jan 28, 2019 •

edited

Loading

ahsonkhan Jan 28, 2019 •

edited

Loading

davidfowl commented Jan 28, 2019 •

edited

Loading

JamesNK Jan 29, 2019 •

edited

Loading

ahsonkhan Jan 29, 2019 •

edited

Loading

halter73 Jan 30, 2019 •

edited

Loading

BrennanConroy Jan 30, 2019 •

edited

Loading

JamesNK Jan 30, 2019 •

edited

Loading