Akka.IO.Tcp: cleaning up TBDs and API junk
#7621
Conversation
| public sealed class Tcp : Akka.Actor.ExtensionIdProvider<Akka.IO.TcpExt> | ||
| { | ||
| public static readonly Akka.Actor.SupervisorStrategy ConnectionSupervisorStrategy; | ||
| public static readonly Akka.IO.Tcp Instance; |
There was a problem hiding this comment.
Might be a breaking API change but this isn't how anyone uses Tcp - they almost all use the extension method. Wanted to get rid of this because it shouldn't be necessary, but both this and the Udp class were defined incorrectly from an ActorSystemExtension stand point and so this is a crutch for making it all work. Going to rework these more fully in v1.6 if I can make the time.
There was a problem hiding this comment.
Rest of my changes are just making things nullable that can receive a null and marking A TON of classes as sealed given that they can't effectively be inherited.
| /// failed during processing. | ||
| /// </summary> | ||
| public CommandFailed FailureMessage => _failureMessage; | ||
| public CommandFailed FailureMessage => new CommandFailed(this); |
There was a problem hiding this comment.
Do we intend to create a new instance every time the property is accessed?
There was a problem hiding this comment.
Yes because this is only accessed once, when the command fails. Right now by design this failure message gets pre-allocated even if we don't need it, which makes every successful operation more memory-expensive than it needs to be. I saw a small perf increase in the benchmarks after this change (I'll post it to this thread) and I think this is probably the change that drove that.
src/core/Akka/IO/Tcp.cs
Outdated
| /// Optionally contains the cause why the command failed. | ||
| /// </summary> | ||
| public Option<Exception> Cause { get; private set; } = Option<Exception>.None; | ||
| public Option<Exception> Cause { get; private set; } |
There was a problem hiding this comment.
shouldn't this have no setter and the property should be set by the constructor?
Also, shouldn't it be set to None by default?
There was a problem hiding this comment.
The compiler told me that it was unnecessary to set this so I opted not to, but maybe that's a mistake.
There was a problem hiding this comment.
Ah, I know why this isn't necessary - we always set via the CTOR now. Also, I've removed the private setter on this property too.
| /// <returns>TBD</returns> | ||
| public override string Cause { get; } | ||
|
|
||
| public override string? Cause { get; } |
There was a problem hiding this comment.
Not sure about that actually - let me check
There was a problem hiding this comment.
Ah, yes it can potentially - if the Exception doesn't have a Message associated with it (not saying that's common, I'm just following what the attributes say)
Benchmark Data
|
| Method | MessageLength | ClientsCount | Mean | Error | StdDev | Req/sec |
|---|---|---|---|---|---|---|
| ClientServerCommunication | 10 | 1 | 27.743 μs | 0.6499 μs | 0.9727 μs | 36,044.72 |
| ClientServerCommunication | 10 | 3 | 15.343 μs | 2.4090 μs | 3.6056 μs | 65,174.81 |
| ClientServerCommunication | 10 | 5 | 11.367 μs | 0.8504 μs | 1.2728 μs | 87,977.48 |
| ClientServerCommunication | 10 | 7 | 8.360 μs | 0.4803 μs | 0.7189 μs | 119,612.69 |
| ClientServerCommunication | 10 | 10 | 6.135 μs | 0.4703 μs | 0.7039 μs | 162,999.71 |
| ClientServerCommunication | 10 | 20 | 4.610 μs | 0.4195 μs | 0.6279 μs | 216,921.71 |
| ClientServerCommunication | 10 | 30 | 4.226 μs | 0.4840 μs | 0.7244 μs | 236,641.14 |
| ClientServerCommunication | 10 | 40 | 4.279 μs | 0.3182 μs | 0.4763 μs | 233,704.71 |
| ClientServerCommunication | 100 | 1 | 26.579 μs | 1.2126 μs | 1.8150 μs | 37,623.28 |
| ClientServerCommunication | 100 | 3 | 13.761 μs | 2.3016 μs | 3.4450 μs | 72,666.94 |
| ClientServerCommunication | 100 | 5 | 11.058 μs | 0.7635 μs | 1.1428 μs | 90,433.05 |
| ClientServerCommunication | 100 | 7 | 8.289 μs | 0.5590 μs | 0.8367 μs | 120,635.75 |
| ClientServerCommunication | 100 | 10 | 6.171 μs | 0.4994 μs | 0.7475 μs | 162,042.01 |
| ClientServerCommunication | 100 | 20 | 4.728 μs | 0.4774 μs | 0.7146 μs | 211,516.21 |
| ClientServerCommunication | 100 | 30 | 4.198 μs | 0.3779 μs | 0.5657 μs | 238,228.09 |
| ClientServerCommunication | 100 | 40 | 4.370 μs | 0.4857 μs | 0.7269 μs | 228,829.16 |
This PR
BenchmarkDotNet v0.13.12, Pop!_OS 22.04 LTS
13th Gen Intel Core i7-1360P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 8.0.404
[Host] : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2
LongRun : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2
Job=LongRun Concurrent=True Server=True
InvocationCount=1 IterationCount=10 LaunchCount=3
RunStrategy=Monitoring UnrollFactor=1 WarmupCount=3
| Method | MessageLength | ClientsCount | Mean | Error | StdDev | Req/sec |
|---|---|---|---|---|---|---|
| ClientServerCommunication | 10 | 1 | 26.616 μs | 0.9416 μs | 1.4093 μs | 37,571.77 |
| ClientServerCommunication | 10 | 3 | 15.249 μs | 2.5857 μs | 3.8702 μs | 65,578.51 |
| ClientServerCommunication | 10 | 5 | 11.153 μs | 0.8168 μs | 1.2226 μs | 89,660.99 |
| ClientServerCommunication | 10 | 7 | 8.337 μs | 0.5651 μs | 0.8458 μs | 119,951.12 |
| ClientServerCommunication | 10 | 10 | 6.351 μs | 0.3816 μs | 0.5711 μs | 157,456.49 |
| ClientServerCommunication | 10 | 20 | 4.290 μs | 0.4166 μs | 0.6235 μs | 233,091.51 |
| ClientServerCommunication | 10 | 30 | 4.429 μs | 0.6571 μs | 0.9835 μs | 225,810.02 |
| ClientServerCommunication | 10 | 40 | 3.884 μs | 0.1679 μs | 0.2513 μs | 257,470.57 |
| ClientServerCommunication | 100 | 1 | 26.002 μs | 1.2095 μs | 1.8104 μs | 38,458.53 |
| ClientServerCommunication | 100 | 3 | 13.922 μs | 2.5910 μs | 3.8781 μs | 71,826.62 |
| ClientServerCommunication | 100 | 5 | 11.095 μs | 0.6992 μs | 1.0466 μs | 90,133.69 |
| ClientServerCommunication | 100 | 7 | 8.285 μs | 0.4973 μs | 0.7443 μs | 120,696.53 |
| ClientServerCommunication | 100 | 10 | 6.219 μs | 0.5954 μs | 0.8912 μs | 160,784.71 |
| ClientServerCommunication | 100 | 20 | 5.110 μs | 0.8395 μs | 1.2565 μs | 195,711.56 |
| ClientServerCommunication | 100 | 30 | 4.636 μs | 0.5569 μs | 0.8336 μs | 215,710.81 |
| ClientServerCommunication | 100 | 40 | 4.711 μs | 0.7093 μs | 1.0617 μs | 212,254.14 |
No real perf difference
Changes
Working on trying to prepare for #5988 and some of the abysmal Akka.IO performance we detected in #7620
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):