-
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
Reduce FSM<TState, TData>
allocations
#6145
Reduce FSM<TState, TData>
allocations
#6145
Conversation
* Made `Event` a `readonly struct` * Eliminated unnecessary `List<object>` allocations * Cleaned up XML-DOC comments
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 API and memory allocation changes.
@@ -736,7 +736,7 @@ namespace Akka.Actor | |||
public override int GetHashCode() { } | |||
public override string ToString() { } | |||
} | |||
public sealed class Event<TD> : Akka.Actor.INoSerializationVerificationNeeded | |||
public struct Event<TD> : Akka.Actor.INoSerializationVerificationNeeded |
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.
Technically a binary breaking change maybe, but it's source compatible.
@@ -773,10 +773,10 @@ namespace Akka.Actor | |||
{ | |||
public static Akka.Actor.FSMBase.StateTimeout Instance { get; } | |||
} | |||
public class State<TS, TD> : System.IEquatable<Akka.Actor.FSMBase.State<TS, TD>> | |||
public sealed class State<TS, TD> : System.IEquatable<Akka.Actor.FSMBase.State<TS, TD>> |
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.
sealed
the State class for better perf.
@@ -435,7 +435,7 @@ public class State<TS, TD> : IEquatable<State<TS, TD>> | |||
/// <param name="notifies">TBD</param> | |||
public State(TS stateName, TD stateData, TimeSpan? timeout = null, Reason stopReason = null, IReadOnlyList<object> replies = null, bool notifies = true) | |||
{ | |||
Replies = replies ?? new List<object>(); | |||
Replies = replies ?? Array.Empty<object>(); |
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.
Eliminated unnecessary List<object>
allocation.
@@ -539,15 +540,16 @@ public State(TS stateName, TD stateData, TimeSpan? timeout = null, Reason stopRe | |||
/// </summary> | |||
internal State<TS, TD> WithNotification(bool notifies) | |||
{ | |||
// don't bother allocating even a stack type if the notifies value is identical. | |||
if (Notifies == notifies) |
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.
Don't return a new copy of this object if the current Notifies
value matches the new one (it usually does.)
@@ -585,26 +587,26 @@ public override int GetHashCode() | |||
/// which allows pattern matching to extract both state and data. | |||
/// </summary> | |||
/// <typeparam name="TD">The state data for this event</typeparam> | |||
public sealed class Event<TD> : INoSerializationVerificationNeeded | |||
public readonly struct Event<TD> : INoSerializationVerificationNeeded |
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.
We allocate one of these for every message we process, so changing to a readonly struct
here greatly reduces Gen 0 / 1 GC.
src/core/Akka/Actor/FSM.cs
Outdated
@@ -750,7 +752,7 @@ public void StartWith(TState stateName, TData stateData, TimeSpan? timeout = nul | |||
/// <returns>Descriptor for staying in the current state.</returns> | |||
public State<TState, TData> Stay() | |||
{ | |||
return GoTo(_currentState.StateName).WithNotification(false); | |||
return _currentState.WithNotification(false); |
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.
Biggest memory savings - don't allocate a new State
object each time we call Stay
. Re-use the same value.
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.
FWIW, there's no benefit to copying the same state over and over again here. The Reason
, Replies
properties et al don't have any effects when used here in combination with WithNotification(false)
.
Looked at making |
Looks like this change definitely caused an issue with the |
Spec could also be racy - worked on my machine. |
Nope, definitely an error related to my changes. Will investigate. |
Benchmarks based on latest changes. BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.2006 (21H2)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=6.0.201
[Host] : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT
DefaultJob : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT
|
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.
LGTM
* close akkadotnet#2560 - added performance benchmarks for FSM * Improved FSM memory consumption * Made `Event` a `readonly struct` * Eliminated unnecessary `List<object>` allocations * Cleaned up XML-DOC comments * don't return new `State<TState, TData>` during `Stay()` * API approvals * fixed `State<TS.,TD>` errors
Changes
Reduced some unnecessary allocations inside the
FSM<TState, TData>
class.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
Latest
v1.4
BenchmarksThis PR's Benchmarks
48% memory reduction, 14% throughput improvement