-
Notifications
You must be signed in to change notification settings - Fork 1k
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
AkkaPduCodec performance fixes [remoting] #3299
AkkaPduCodec performance fixes [remoting] #3299
Conversation
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.
Described changes
@@ -228,14 +228,19 @@ protected AkkaPduCodec(ActorSystem system) | |||
/// <returns>TBD</returns> | |||
public virtual ByteString EncodePdu(IAkkaPdu pdu) | |||
{ | |||
ByteString finalBytes = null; | |||
pdu.Match() |
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.
Get rid of the old .Match
extension method, which allocates. Replace with a C# 7 switch
statement instead. Does not allocate.
return finalBytes; | ||
switch (pdu) | ||
{ | ||
case Payload p: |
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.
Change the order in which we filter for events in Akka.Remote serialization; Payload
makes up the vast majority of network traffic, since that's all of the messages that aren't part of the Akka.Remote association protocol itself. Payload
was previously handled second, so we always had one case we needed to fall through each time.
Heartbeat
is the second most frequently used message type, since these are emitted once per second.
* Since there's never any ActorSystem-specific information coded directly | ||
* into the heartbeat messages themselves (i.e. no handshake info,) there's no harm in caching in the | ||
* same heartbeat byte buffer and re-using it. | ||
*/ |
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.
As explained in the comment above: Heartbeat
messages don't contain any handshake information, therefore the same "heartbeat" message can be used across many different associations and even many different ActorSystems
without having to be created from scratch each time. We cache this value in a static mutable field on the serializer now. We could probably make this immutable....
* into the heartbeat messages themselves (i.e. no handshake info,) there's no harm in caching in the | ||
* same heartbeat byte buffer and re-using it. | ||
*/ | ||
private static readonly ByteString HeartbeatPdu = ConstructControlMessagePdu(CommandType.Heartbeat); |
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.
Was able to make this static and immutable as well.
I'll let the NBench stuff do its thing here, but I'm also going to run a PingPong benchmark locally for comparison. These optimizations are all fairly "micro," but they should noticeably reduce allocations per-remote-message sent by at least 1. |
Bearing in mind that these values aren't scientific because I'm ran them on my personal computer, which has tons of stuff running in the background: Beforeλ dotnet run .\RemotePingPong --framework netcoreapp1.1 Num clients, Total [msg], Msgs/sec, Total [ms] λ dotnet run .\RemotePingPong --framework net461 Num clients, Total [msg], Msgs/sec, Total [ms] Afterλ dotnet run .\RemotePingPong --framework netcoreapp1.1 Num clients, Total [msg], Msgs/sec, Total [ms] λ dotnet run .\RemotePingPong --framework net461 Num clients, Total [msg], Msgs/sec, Total [ms] IMHO, looks to me like the change made a material difference on .NET 4.6.1 but none at all on .NET Core 1.1. I'll wait and see what the build server reports from NBench though. |
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.
Changes looks good. The results of benchmarks you've shown looks disturbing thou. No performance improvement?
@Horusiath looks like there was one in the .NET 4.6.1 pipeline but not .NET Core 1.1. Guess in this instance the amount of objects being allocated wasn't enough to make a material difference to the frequency at which the garbage collector runs. In other words, there's bigger bottlenecks / bigger allocators than the |
* AkkaPduCodec performance fixes * made HeartbeatPdu immutable * made all internal formatting methods static
* AkkaPduCodec performance fixes * made HeartbeatPdu immutable * made all internal formatting methods static
Noticed some low-hanging fruit for performance in the
AkkaPduCodec
fixes while I was working on implementing a custom serializer for Akka.Remote today.This is the class responsible for serializing 100% of message traffic that passes over each Akka.Remote connection.