-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Update MessagePack to v2.0 for SignalR #18133
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
Update MessagePack to v2.0 for SignalR #18133
Conversation
@pranavkm @AArnott
This seem to be caused by :
As it's a submodule, i have no idea how to leverage on that ? @AArnott, then to https://github.com/aspnet/MessagePack-CSharp |
Couple things: |
hello @BrennanConroy
You mean ? i should not edit the files inside the SubModule folder i guess ? yeah, i did not My guess is that the fork
To be honest i did it here first to know if it could be part of 3.1, i'm not sure if it's concidered as a Breaking change or not to be honest.
As you said i'll try tomorrow to rebase on master ASAP yes regarding the build error :
i just made the change locally to both |
also, in order to fix the potential breaking change due to consumer using
I have no idea of what it implies / the change / the performance impact etc ... To be fair I started the PR seeing that After load test with ~5000 call per second for few minutes we saw that |
It exists because Blazor uses MessagePack internally, but since Blazor is in the shared framework and we don't put third-party libraries in the shared framework, it needs to build MessagePack from source.
I don't see why you need to do anything with aspnet/MessagePack-CSharp, it's unrelated to SignalR's usage of MessagePack.
It is a breaking change to update non-patch versions of dependencies. |
because i saw this https://github.com/aspnet/AspNetCore/blob/release/3.1/.gitmodules#L7 Now i understand why i saw both the |
|
Not sure how should i proceed for all the Also i'm not happy with 2 of them in RedisProtocol to be honest |
I'm glad we're all on the same page that the submodule is irrelevant here. But just to respond to this question:
The fork relationship through aarnott first and then to neuecc is just a historical artifact from when the AArnott account had the 2.0 source code before neuecc had it. So it was logical to fork from the newer repo. Now that the fork is created, the relationship of being a direct child or grand-child of neuecc is irrelevant AFAIK because github doesn't do anything for you whether you're an immediate vs. distant fork 'relative'. You still have to pull from some upstream and push to your own repo to stay current. And ya, the aspnet fork of messagepack probably hasn't pushed the 2.0 RTM release of messagepack from neuecc yet as they haven't needed to but may choose to do that in the future. But again, irrelevant to what you're doing, so that's great. |
Given that (which makes total sense), perhaps the SignalR 3.1 change should be to add the upper-bound requirement on the messagepack reference. This way the expected behavior of someone who references both ASP.NET Core and MessagePack directly will get a nuget error to tell them that SignalR is incompatible with MessagePack 2.0. Then we could start a more long-running PR to upgrade SignalR to consume MessagePack 2.0 which would complete for some major upgrade of SignalR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to perf numbers once this is working
src/SignalR/server/StackExchangeRedis/src/Internal/MessagePackUtil.cs
Outdated
Show resolved
Hide resolved
src/SignalR/server/StackExchangeRedis/src/Internal/RedisProtocol.cs
Outdated
Show resolved
Hide resolved
src/SignalR/server/StackExchangeRedis/src/Internal/RedisProtocol.cs
Outdated
Show resolved
Hide resolved
src/SignalR/server/StackExchangeRedis/src/Internal/RedisProtocol.cs
Outdated
Show resolved
Hide resolved
src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocol.cs
Outdated
Show resolved
Hide resolved
src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocol.cs
Outdated
Show resolved
Hide resolved
src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocol.cs
Show resolved
Hide resolved
Any idea if the I'm not sure where to look at, it feels like something is missing somewhere, bad url or service being down |
well then "squash and merge", i try to give git history a sense for reviewer so people can navigate through it step by step, then review commits |
Don't use it. We have a dedicated analyzer that does a much more thorough job of keeping you on the happy path in MessagePack-CSharp/MessagePack-CSharp#769 that will be available very soon. |
Best way is to reach out to anyone on the team, asking them to do the |
sounds good to me i did not took shoot yet, i was not sure about all lifetime duration. I had doubt especially since the
@BrennanConroy dont worry i took a look at it elsewhere ( #18291)
@AArnott should i close #183291 then
@dougbu |
Early run =>
Soon, i'll reset on
HubProtocolBenchmark (
|
Method | Input | HubProtocol | Mean | Error | StdDev | Op/s | Gen 0 | Allocated |
---|---|---|---|---|---|---|---|---|
ReadSingleMessage | FewArguments | Json | 1,408.0 ns | 27.445 ns | 37.567 ns | 710,233.4 | - | 224 B |
WriteSingleMessage | FewArguments | Json | 677.0 ns | 12.220 ns | 11.431 ns | 1,477,014.5 | - | 80 B |
ReadSingleMessage | FewArguments | MsgPack | 508.7 ns | 8.661 ns | 8.101 ns | 1,965,800.8 | 0.0010 | 224 B |
WriteSingleMessage | FewArguments | MsgPack | 299.6 ns | 4.098 ns | 3.633 ns | 3,337,701.3 | - | 48 B |
ReadSingleMessage | FewArguments | NewtonsoftJson | 1,473.4 ns | 7.938 ns | 7.037 ns | 678,724.0 | 0.0057 | 960 B |
WriteSingleMessage | FewArguments | NewtonsoftJson | 1,237.1 ns | 18.995 ns | 17.767 ns | 808,353.6 | 0.0038 | 784 B |
ReadSingleMessage | LargeArguments | Json | 7,747.9 ns | 40.263 ns | 35.692 ns | 129,067.9 | 0.2136 | 30904 B |
WriteSingleMessage | LargeArguments | Json | 12,189.6 ns | 115.550 ns | 102.432 ns | 82,037.1 | 0.1526 | 23976 B |
ReadSingleMessage | LargeArguments | MsgPack | 3,090.9 ns | 17.733 ns | 15.720 ns | 323,529.3 | 0.2136 | 30904 B |
WriteSingleMessage | LargeArguments | MsgPack | 2,547.6 ns | 48.178 ns | 45.066 ns | 392,530.1 | 0.1373 | 20528 B |
ReadSingleMessage | LargeArguments | NewtonsoftJson | 54,095.9 ns | 435.854 ns | 407.698 ns | 18,485.7 | 0.3662 | 58840 B |
WriteSingleMessage | LargeArguments | NewtonsoftJson | 30,820.9 ns | 248.092 ns | 232.065 ns | 32,445.5 | 0.1221 | 24768 B |
ReadSingleMessage | ManyArguments | Json | 2,879.0 ns | 40.406 ns | 37.796 ns | 347,348.8 | - | 424 B |
WriteSingleMessage | ManyArguments | Json | 1,212.3 ns | 20.346 ns | 19.032 ns | 824,883.1 | - | 120 B |
ReadSingleMessage | ManyArguments | MsgPack | 927.8 ns | 14.998 ns | 12.524 ns | 1,077,849.4 | 0.0019 | 400 B |
WriteSingleMessage | ManyArguments | MsgPack | 502.5 ns | 6.243 ns | 5.840 ns | 1,990,093.7 | - | 72 B |
ReadSingleMessage | ManyArguments | NewtonsoftJson | 2,685.6 ns | 17.956 ns | 14.994 ns | 372,353.9 | 0.0076 | 1504 B |
WriteSingleMessage | ManyArguments | NewtonsoftJson | 2,390.1 ns | 20.629 ns | 19.296 ns | 418,398.3 | 0.0114 | 1992 B |
ReadSingleMessage | NoArguments | Json | 505.3 ns | 7.987 ns | 7.471 ns | 1,978,974.2 | - | 96 B |
WriteSingleMessage | NoArguments | Json | 231.9 ns | 3.596 ns | 3.364 ns | 4,312,739.9 | 0.0005 | 72 B |
ReadSingleMessage | NoArguments | MsgPack | 295.1 ns | 1.561 ns | 1.219 ns | 3,388,182.0 | 0.0005 | 120 B |
WriteSingleMessage | NoArguments | MsgPack | 156.9 ns | 3.005 ns | 2.663 ns | 6,372,944.3 | 0.0002 | 40 B |
ReadSingleMessage | NoArguments | NewtonsoftJson | 674.8 ns | 5.865 ns | 5.487 ns | 1,481,944.1 | 0.0038 | 584 B |
WriteSingleMessage | NoArguments | NewtonsoftJson | 583.0 ns | 3.126 ns | 2.611 ns | 1,715,365.5 | 0.0019 | 384 B |
RedisProtocolBenchmark (MessagePack v2
)
BenchmarkDotNet=v0.10.13, OS=Windows 10.0.19041
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical cores and 8 physical cores
.NET Core SDK=5.0.100-alpha1-015752
[Host] : .NET Core 5.0.0-alpha.1.19562.8 (CoreCLR 5.0.19.56201, CoreFX 5.0.19.55607), 64bit RyuJIT
Job-VDDHMF : .NET Core 5.0.0-alpha.1.19562.8 (CoreCLR 5.0.19.56201, CoreFX 5.0.19.55607), 64bit RyuJIT
Runtime=Core Server=True Toolchain=.NET Core 5.0
RunStrategy=Throughput
Method | Mean | Error | StdDev | Op/s | Gen 0 | Allocated |
---|---|---|---|---|---|---|
WriteAck | 99.53 ns | 1.0613 ns | 0.9928 ns | 10,047,286.1 | 0.0001 | 32 B |
WriteGroupCommand | 156.25 ns | 1.7986 ns | 1.6825 ns | 6,400,086.4 | 0.0002 | 56 B |
WriteInvocationNoExclusions | 492.51 ns | 4.8058 ns | 4.4954 ns | 2,030,432.1 | 0.0019 | 344 B |
WriteInvocationSmallExclusions | 569.81 ns | 4.0341 ns | 3.7735 ns | 1,754,966.5 | 0.0029 | 448 B |
WriteInvocationLargeExclusions | 1,024.88 ns | 7.7735 ns | 7.2714 ns | 975,720.9 | 0.0057 | 1056 B |
ReadAck | 44.09 ns | 0.8997 ns | 1.8780 ns | 22,681,088.2 | - | 0 B |
ReadGroupCommand | 170.60 ns | 3.3896 ns | 6.1121 ns | 5,861,596.0 | 0.0007 | 120 B |
ReadInvocationNoExclusions | 472.52 ns | 9.2724 ns | 11.7266 ns | 2,116,300.1 | 0.0019 | 328 B |
ReadInvocationSmallExclusions | 520.59 ns | 11.5869 ns | 15.4682 ns | 1,920,882.2 | 0.0029 | 544 B |
ReadInvocationLargeExclusions | 1,272.12 ns | 7.4166 ns | 6.9375 ns | 786,087.4 | 0.0153 | 2272 B |
BenchmarkDotNet=v0.10.13, OS=Windows 10.0.19041
Intel Core i9-9980HK CPU 2.40GHz, 1 CPU, 16 logical cores and 8 physical cores
.NET Core SDK=5.0.100-alpha1-015752
[Host] : .NET Core 5.0.0-alpha.1.19562.8 (CoreCLR 5.0.19.56201, CoreFX 5.0.19.55607), 64bit RyuJIT
Job-MRPWQU : .NET Core 5.0.0-alpha.1.19562.8 (CoreCLR 5.0.19.56201, CoreFX 5.0.19.55607), 64bit RyuJIT
Runtime=Core Server=True Toolchain=.NET Core 5.0
RunStrategy=Throughput
HubProtocolBenchmark (MessagePack v1 versus v2)I'm very very suprised about what i see :
(i'll tree to re-run both to be sure of that diff)
RedisProtocolBenchmark (MessagePack v1 versus v2)On the good side
|
@tebeco Writing is faster because we don't have to allocate and copy memory. Especially when writing large messages, v1 would resize arrays repeatedly which got progressively more expensive each time it did it. With v2 we can write to non-contiguous buffers so we just ask for more memory when we need it, and can write directly into the memory offered by an existing IBufferWriter. Reading is a bit slower because we no longer demand reading from a |
Seems pretty logic explained like this, i just did not excepted that amount of difference to be honest |
It depends on what you're measuring. It's 2X slower or more when you're deserializing an integer. But the cost amortizes out as the size of the message increases. By the time you're deserializing a type with several members in it, the difference becomes insignificant. |
As i said i would re-run, and not to pollute upper table which are already big enough :
For both
|
@BrennanConroy I can rebase on |
Sorry, I was busy dealing with some other things. I'll take a look at this right now.
No need to rebase on Updating to the new version would be nice though! |
src/SignalR/common/Protocols.MessagePack/src/Protocol/MessagePackHubProtocol.cs
Show resolved
Hide resolved
i'll update the version of messagepack tomorrow in the morning (10PM here) |
and i'll ping @dougbu to run the build because he volunteered (even if the trigger worked |
Shouldn't be necessary but if it is sure. (Updating the branch should cause the builds to run.) |
Thanks for the great contribution @tebeco ! |
thx everyone for the patience and various form of help. |
This is great to see! Those reading numbers look at bit worse but I'd expect that because of what @AArnott said. Hopefully it doesn't matter for larger object graphs |
As this PR does not have any Issue related, it does not appear in the release note of the Not sure if it could / should be changed somehow |
Summary of the changes (Less than 80 chars)
As the API from MessagePack v1.x that tended to be "Allocation prone" were reworked in
v2.0
.Related to #18074
As said in the issue i might have to rebase the changes on master :(
I add lots of
// REVIEW
in the code as reviewer here probably have better answer than me :DOut of curiosity, I change the remote URI for the submodule, as it was pointing to aspnet/MessagePack-CSharp (security reason ?) than was a fork from @AArnott repo, that is also a fork from the original repository