Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reduce FSM<TState, TData> allocations #6145

Merged
merged 7 commits into from
Oct 7, 2022
Merged

Reduce FSM<TState, TData> allocations #6145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5d7d98
close #2560 - added performance benchmarks for FSM
Aaronontheweb Oct 5, 2022
b06a0d7
Improved FSM memory consumption
Aaronontheweb Oct 5, 2022
3227a6a
don't return new `State<TState, TData>` during `Stay()`
Aaronontheweb Oct 6, 2022
1b72186
Merge branch 'v1.4' into fsm-actor-allocations
Aaronontheweb Oct 6, 2022
9aad12b
API approvals
Aaronontheweb Oct 6, 2022
cb2b110
Merge branch 'v1.4' into fsm-actor-allocations
Aaronontheweb Oct 6, 2022
33cb3cb
fixed `State<TS.,TD>` errors
Aaronontheweb Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

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.

{
public Event(object fsmEvent, TD stateData) { }
public object FsmEvent { get; }
Expand Down Expand Up @@ -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>>
Copy link
Member Author

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.

{
public State(TS stateName, TD stateData, System.Nullable<System.TimeSpan> timeout = null, Akka.Actor.FSMBase.Reason stopReason = null, System.Collections.Generic.IReadOnlyList<object> replies = null, bool notifies = True) { }
public System.Collections.Generic.IReadOnlyList<object> Replies { get; set; }
public System.Collections.Generic.IReadOnlyList<object> Replies { get; }
public TD StateData { get; }
public TS StateName { get; }
public Akka.Actor.FSMBase.Reason StopReason { get; }
Expand Down
8 changes: 4 additions & 4 deletions src/core/Akka.Tests/Actor/FSMActorSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,20 +495,20 @@ public void FSMActor_must_cancel_all_timers_when_terminated()
var fsmRef = new TestFSMRef<StopTimersFSM, string, object>(Sys, Props.Create(
() => new StopTimersFSM(TestActor, timerNames)));

Action<bool> checkTimersActive = active =>
void CheckTimersActive(bool active)
{
foreach (var timer in timerNames)
{
fsmRef.IsTimerActive(timer).Should().Be(active);
fsmRef.IsStateTimerActive().Should().Be(active);
}
};
}

checkTimersActive(false);
CheckTimersActive(false);

fsmRef.Tell("start");
ExpectMsg("starting", 1.Seconds());
checkTimersActive(true);
CheckTimersActive(true);

fsmRef.Tell("stop");
ExpectMsg("stopped", 1.Seconds());
Expand Down
70 changes: 44 additions & 26 deletions src/core/Akka/Actor/FSM.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public override bool Equals(object obj)
{
if (ReferenceEquals(null, obj)) return false;
if (ReferenceEquals(this, obj)) return true;
return obj is CurrentState<TS> && Equals((CurrentState<TS>)obj);
return obj is CurrentState<TS> state && Equals(state);
}


Expand Down Expand Up @@ -129,7 +129,7 @@ public override bool Equals(object obj)
{
if (ReferenceEquals(null, obj)) return false;
if (ReferenceEquals(this, obj)) return true;
return obj is Transition<TS> && Equals((Transition<TS>)obj);
return obj is Transition<TS> transition && Equals(transition);
}


Expand Down Expand Up @@ -206,7 +206,7 @@ public abstract class Reason { }
/// </summary>
public sealed class Normal : Reason
{
internal Normal() { }
private Normal() { }

/// <summary>
/// Singleton instance of Normal
Expand All @@ -220,7 +220,7 @@ internal Normal() { }
/// </summary>
public sealed class Shutdown : Reason
{
internal Shutdown() { }
private Shutdown() { }

/// <summary>
/// Singleton instance of Shutdown
Expand Down Expand Up @@ -258,7 +258,7 @@ public Failure(object cause)
/// </summary>
public sealed class StateTimeout
{
internal StateTimeout() { }
private StateTimeout() { }

/// <summary>
/// Singleton instance of StateTimeout
Expand Down Expand Up @@ -422,7 +422,7 @@ public override string ToString()
/// </summary>
/// <typeparam name="TS">The name of the state</typeparam>
/// <typeparam name="TD">The data of the state</typeparam>
public class State<TS, TD> : IEquatable<State<TS, TD>>
public sealed class State<TS, TD> : IEquatable<State<TS, TD>>
{
/// <summary>
/// Initializes a new instance of the State
Expand All @@ -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>();
Copy link
Member Author

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.

StopReason = stopReason;
Timeout = timeout;
StateData = stateData;
Expand All @@ -444,32 +444,33 @@ public State(TS stateName, TD stateData, TimeSpan? timeout = null, Reason stopRe
}

/// <summary>
/// TBD
/// The name of this state
/// </summary>
public TS StateName { get; }

/// <summary>
/// TBD
/// The data belonging to this sate
/// </summary>
public TD StateData { get; }

/// <summary>
/// TBD
/// Optional. The state timeout.
/// </summary>
public TimeSpan? Timeout { get; }

/// <summary>
/// TBD
/// Optional - the reason why we're stopping.
/// </summary>
public Reason StopReason { get; }

/// <summary>
/// TBD
/// Optional - the set of replies to send to subscribers.
/// </summary>
public IReadOnlyList<object> Replies { get; protected set; }
public IReadOnlyList<object> Replies { get; }

/// <summary>
/// TBD
/// INTERNAL API. Indicates whether or not we're sending transition notifications
/// for this state change.
/// </summary>
internal bool Notifies { get; }

Expand All @@ -482,9 +483,24 @@ public State(TS stateName, TD stateData, TimeSpan? timeout = null, Reason stopRe
/// <returns>TBD</returns>
internal State<TS, TD> Copy(TimeSpan? timeout, Reason stopReason = null, IReadOnlyList<object> replies = null)
{
// otherwise, return a new copy.
return new State<TS, TD>(StateName, StateData, timeout, stopReason ?? StopReason, replies ?? Replies, Notifies);
}

/// <summary>
/// Remove all of the unwanted bits we don't need during a Stay() operation.
/// </summary>
/// <remarks>
/// This is a performance optimization designed to
/// </remarks>
/// <returns>Returns a "clean" copy of a state object the first time it's called. Idempotent after.</returns>
internal State<TS, TD> SpecialCopyForStay()
{
if(Replies.Count > 0 || StopReason != null || Timeout != null || Notifies)
return new State<TS, TD>(StateName, StateData, notifies:false);
return this;
}

/// <summary>
/// Modify the state transition descriptor to include a state timeout for the
/// next state. This timeout overrides any default timeout set for the next state.
Expand Down Expand Up @@ -539,15 +555,16 @@ internal State<TS, TD> WithStopReason(Reason reason)
/// </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)
Copy link
Member Author

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.)

return this;
return new State<TS, TD>(StateName, StateData, Timeout, StopReason, Replies, notifies);
}


public override string ToString()
{
return $"{StateName}, {StateData}";
}


public bool Equals(State<TS, TD> other)
{
Expand Down Expand Up @@ -585,26 +602,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
Copy link
Member Author

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.

{
/// <summary>
/// Initializes a new instance of the Event
/// </summary>
/// <param name="fsmEvent">TBD</param>
/// <param name="stateData">TBD</param>
/// <param name="fsmEvent">The message received by the FSM.</param>
/// <param name="stateData">The current state data of the FSM.</param>
public Event(object fsmEvent, TD stateData)
{
StateData = stateData;
FsmEvent = fsmEvent;
}

/// <summary>
/// TBD
/// The message received by the FSM.
/// </summary>
public object FsmEvent { get; }

/// <summary>
/// TBD
/// The current state data of the FSM.
/// </summary>
public TD StateData { get; }

Expand Down Expand Up @@ -750,7 +767,8 @@ public State<TState, TData> GoTo(TState nextStateName, TData stateData)
/// <returns>Descriptor for staying in the current state.</returns>
public State<TState, TData> Stay()
{
return GoTo(_currentState.StateName).WithNotification(false);
// don't want to allocate a new state if we don't have to
return _currentState.SpecialCopyForStay();
}

/// <summary>
Expand Down Expand Up @@ -940,7 +958,7 @@ public TState StateName
{
get
{
if (_currentState != null)
if (_currentState != null)
return _currentState.StateName;
throw new IllegalStateException("You must call StartWith before calling StateName.");
}
Expand Down Expand Up @@ -1175,7 +1193,7 @@ private void ProcessEvent(Event<TData> fsmEvent, object source)
var stateFunc = _stateFunctions[_currentState.StateName];
var oldState = _currentState;

State<TState, TData> nextState = null;
State<TState, TData> nextState = default;

if (stateFunc != null)
{
Expand All @@ -1195,7 +1213,7 @@ private void ProcessEvent(Event<TData> fsmEvent, object source)
}
}

private string GetSourceString(object source)
private static string GetSourceString(object source)
{
if (source is string s)
return s;
Expand Down Expand Up @@ -1249,7 +1267,7 @@ private void MakeTransition(State<TState, TData> nextState)
_nextState = nextState;
HandleTransition(_currentState.StateName, nextState.StateName);
Listeners.Gossip(new Transition<TState>(Self, _currentState.StateName, nextState.StateName));
_nextState = null;
_nextState = default(State<TState, TData>);
}
_currentState = nextState;

Expand Down