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.

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

Improve performance on IActorRef.Child API #5242

Merged
merged 20 commits into from
Sep 2, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions src/benchmark/Akka.Benchmarks/Actor/GetChildBenchmark.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ protected override void OnReceive(object message)
private ActorWithChild.Get _getMessage = new ActorWithChild.Get("food");
private ActorWithChild.Create _createMessage = new ActorWithChild.Create("food");

private ActorCell _cell;
private IActorContext _cell;
private RepointableActorRef _repointableActorRef;
private LocalActorRef _localActorRef;
private VirtualPathContainer _virtualPathContainer;
Expand Down Expand Up @@ -129,7 +129,7 @@ public async Task Setup()
[Benchmark]
public void ResolveChild()
{
_cell.TryGetSingleChild(_getMessage.Name, out var child);
_cell.Child(_getMessage.Name);
}

[Benchmark]
Expand Down
10 changes: 5 additions & 5 deletions src/core/Akka.API.Tests/CoreAPISpec.ApproveCore.approved.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,7 @@ namespace Akka.Actor
bool IsTerminated { get; }
Akka.Actor.IInternalActorRef Parent { get; }
Akka.Actor.IActorRefProvider Provider { get; }
Akka.Actor.IActorRef GetChild(System.Collections.Generic.IEnumerable<string> name);
Akka.Actor.IActorRef GetChild(System.Collections.Generic.IReadOnlyList<string> name);
void Restart(System.Exception cause);
void Resume(System.Exception causedByFailure = null);
[System.ObsoleteAttribute("Use SendSystemMessage(message) [1.1.0]")]
Expand Down Expand Up @@ -1202,7 +1202,7 @@ namespace Akka.Actor
public abstract bool IsTerminated { get; }
public abstract Akka.Actor.IInternalActorRef Parent { get; }
public abstract Akka.Actor.IActorRefProvider Provider { get; }
public abstract Akka.Actor.IActorRef GetChild(System.Collections.Generic.IEnumerable<string> name);
public abstract Akka.Actor.IActorRef GetChild(System.Collections.Generic.IReadOnlyList<string> name);
public abstract void Restart(System.Exception cause);
public abstract void Resume(System.Exception causedByFailure = null);
[System.ObsoleteAttribute("Use SendSystemMessage(message) instead [1.1.0]")]
Expand Down Expand Up @@ -1245,7 +1245,7 @@ namespace Akka.Actor
protected Akka.Actor.IInternalActorRef Supervisor { get; }
protected Akka.Actor.ActorSystem System { get; }
public override Akka.Actor.ICell Underlying { get; }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IEnumerable<string> name) { }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IReadOnlyList<string> name) { }
public override Akka.Actor.IInternalActorRef GetSingleChild(string name) { }
protected virtual Akka.Actor.ActorCell NewActorCell(Akka.Actor.Internal.ActorSystemImpl system, Akka.Actor.IInternalActorRef self, Akka.Actor.Props props, Akka.Dispatch.MessageDispatcher dispatcher, Akka.Actor.IInternalActorRef supervisor) { }
public override void Restart(System.Exception cause) { }
Expand Down Expand Up @@ -1317,7 +1317,7 @@ namespace Akka.Actor
[System.ObsoleteAttribute("Use Context.Watch and Receive<Terminated> [1.1.0]")]
public override bool IsTerminated { get; }
public override Akka.Actor.IInternalActorRef Parent { get; }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IEnumerable<string> name) { }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IReadOnlyList<string> name) { }
public override void Restart(System.Exception cause) { }
public override void Resume(System.Exception causedByFailure = null) { }
public override void SendSystemMessage(Akka.Dispatch.SysMsg.ISystemMessage message) { }
Expand Down Expand Up @@ -1533,7 +1533,7 @@ namespace Akka.Actor
public override Akka.Actor.ActorPath Path { get; }
public override Akka.Actor.IActorRefProvider Provider { get; }
public override Akka.Actor.ICell Underlying { get; }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IEnumerable<string> name) { }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IReadOnlyList<string> name) { }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source-compatible, but not necessarily a binary-compatible change.

public override Akka.Actor.IInternalActorRef GetSingleChild(string name) { }
public Akka.Actor.RepointableActorRef Initialize(bool async) { }
protected virtual Akka.Actor.ActorCell NewCell() { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ namespace Akka.Remote
public override Akka.Actor.IInternalActorRef Parent { get; }
public override Akka.Actor.ActorPath Path { get; }
public override Akka.Actor.IActorRefProvider Provider { get; }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IEnumerable<string> name) { }
public override Akka.Actor.IActorRef GetChild(System.Collections.Generic.IReadOnlyList<string> name) { }
public bool IsWatchIntercepted(Akka.Actor.IActorRef watchee, Akka.Actor.IActorRef watcher) { }
public override void Restart(System.Exception cause) { }
public override void Resume(System.Exception causedByFailure = null) { }
Expand Down
10 changes: 5 additions & 5 deletions src/core/Akka.Remote/RemoteActorRef.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Akka.Annotations;
using Akka.Dispatch.SysMsg;
using Akka.Event;
using Akka.Util.Internal.Collections;

namespace Akka.Remote
{
Expand Down Expand Up @@ -106,17 +107,16 @@ public RemoteActorRef(RemoteTransport remote, Address localAddressToUse, ActorPa
/// <param name="name">The name.</param>
/// <returns>ActorRef.</returns>
/// <exception cref="System.NotImplementedException">TBD</exception>
public override IActorRef GetChild(IEnumerable<string> name)
public override IActorRef GetChild(IReadOnlyList<string> name)
{
var items = name.ToList();
switch (items.FirstOrDefault())
switch (name.FirstOrDefault())
{
case null:
return this;
case "..":
return Parent.GetChild(items.Skip(1));
return Parent.GetChild(name.NoCopySlice(1));
default:
return new RemoteActorRef(Remote, LocalAddressToUse, Path / items, ActorRefs.Nobody, Props.None, Deploy.None);
return new RemoteActorRef(Remote, LocalAddressToUse, Path / name, ActorRefs.Nobody, Props.None, Deploy.None);
}
}

Expand Down
10 changes: 4 additions & 6 deletions src/core/Akka.Remote/RemoteSystemDaemon.cs
Original file line number Diff line number Diff line change
Expand Up @@ -156,11 +156,9 @@ protected override void TellInternal(object message, IActorRef sender)
}
}

var t = Rec(ImmutableList<string>.Empty);
var concatenatedChildNames = t.Item1;
var m = t.Item2;
var (concatenatedChildNames, m) = Rec(ImmutableList<string>.Empty);

var child = GetChild(concatenatedChildNames);
var child = GetChild(concatenatedChildNames.ToList());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra allocation in remote deployments here.... might see if I can go about fixing that still....

if (child.IsNobody())
{
var emptyRef = new EmptyLocalActorRef(_system.Provider,
Expand Down Expand Up @@ -302,7 +300,7 @@ private void HandleDaemonMsgCreate(DaemonMsgCreate message)
/// </summary>
/// <param name="name">The name.</param>
/// <returns>ActorRef.</returns>
public override IActorRef GetChild(IEnumerable<string> name)
public override IActorRef GetChild(IReadOnlyList<string> name)
{
var path = name.Join("/");
var n = 0;
Expand All @@ -313,7 +311,7 @@ public override IActorRef GetChild(IEnumerable<string> name)
{
if (uid != ActorCell.UndefinedUid && uid != child.Path.Uid)
return Nobody.Instance;
return n == 0 ? child : child.GetChild(name.TakeRight(n));
return n == 0 ? child : child.GetChild(name.TakeRight(n).ToList());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another allocation in remote deployments

}

var last = path.LastIndexOf("/", StringComparison.Ordinal);
Expand Down
2 changes: 1 addition & 1 deletion src/core/Akka.TestKit/TestActorRefBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ ISurrogate ISurrogated.ToSurrogate(ActorSystem system)

bool IInternalActorRef.IsTerminated { get { return _internalRef.IsTerminated; } }

IActorRef IInternalActorRef.GetChild(IEnumerable<string> name)
IActorRef IInternalActorRef.GetChild(IReadOnlyList<string> name)
{
return _internalRef.GetChild(name);
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/Akka.TestKit/TestProbe.cs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ ISurrogate ISurrogated.ToSurrogate(ActorSystem system)

bool IInternalActorRef.IsTerminated { get { return ((IInternalActorRef)TestActor).IsTerminated; } }

IActorRef IInternalActorRef.GetChild(IEnumerable<string> name)
IActorRef IInternalActorRef.GetChild(IReadOnlyList<string> name)
{
return ((IInternalActorRef)TestActor).GetChild(name);
}
Expand Down
11 changes: 5 additions & 6 deletions src/core/Akka/Actor/ActorCell.Children.cs
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,7 @@ protected void SetTerminated()
{
get
{
var terminating = ChildrenContainer as TerminatingChildrenContainer;
return terminating != null && terminating.Reason is SuspendReason.IWaitingForChildren;
return ChildrenContainer is TerminatingChildrenContainer terminating && terminating.Reason is SuspendReason.IWaitingForChildren;
}
}

Expand Down Expand Up @@ -325,8 +324,7 @@ public bool TryGetChildStatsByName(string name, out IChildStats child) //This
/// </summary>
private bool TryGetChildRestartStatsByName(string name, out ChildRestartStats child)
{
IChildStats stats;
if (ChildrenContainer.TryGetByName(name, out stats))
if (ChildrenContainer.TryGetByName(name, out var stats))
{
child = stats as ChildRestartStats;
if (child != null)
Expand Down Expand Up @@ -358,10 +356,11 @@ protected bool TryGetChildStatsByRef(IActorRef actor, out ChildRestartStats chil
[Obsolete("Use TryGetSingleChild [0.7.1]")]
public IInternalActorRef GetSingleChild(string name)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Half the perf optimizations happened here:

  • Simplified branching
  • Remove Try... out stuff since it was all functionally being converted back into ActorRefs.Nobody anyway.

{
IInternalActorRef child;
return TryGetSingleChild(name, out child) ? child : ActorRefs.Nobody;
return TryGetSingleChild(name, out var child) ? child : ActorRefs.Nobody;
}



/// <summary>
/// TBD
/// </summary>
Expand Down
5 changes: 4 additions & 1 deletion src/core/Akka/Actor/ActorCell.cs
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,10 @@ public void Init(bool sendSupervise, MailboxType mailboxType)

IActorRef IActorContext.Child(string name)
{
return TryGetSingleChild(name, out var child) ? child : ActorRefs.Nobody;
if (TryGetChildStatsByName(name, out var child) && child is ChildRestartStats s)
return s.Child;

return ActorRefs.Nobody;
}

/// <summary>
Expand Down
22 changes: 10 additions & 12 deletions src/core/Akka/Actor/ActorRef.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using Akka.Event;
using Akka.Util;
using Akka.Util.Internal;
using Akka.Util.Internal.Collections;

namespace Akka.Actor
{
Expand Down Expand Up @@ -422,7 +423,7 @@ public interface IInternalActorRef : IActorRef, IActorRefScope
/// </summary>
/// <param name="name">The path elements.</param>
/// <returns>The <see cref="IActorRef"/>, or if the requested path does not exist, returns <see cref="Nobody"/>.</returns>
IActorRef GetChild(IEnumerable<string> name);
IActorRef GetChild(IReadOnlyList<string> name);

/// <summary>
/// Resumes an actor if it has been suspended.
Expand Down Expand Up @@ -481,7 +482,7 @@ public abstract class InternalActorRefBase : ActorRefBase, IInternalActorRef
public abstract IActorRefProvider Provider { get; }

/// <inheritdoc cref="IInternalActorRef"/>
public abstract IActorRef GetChild(IEnumerable<string> name); //TODO: Refactor this to use an IEnumerator instead as this will be faster instead of enumerating multiple times over name, as the implementations currently do.
public abstract IActorRef GetChild(IReadOnlyList<string> name); //TODO: Refactor this to use an IEnumerator instead as this will be faster instead of enumerating multiple times over name, as the implementations currently do.

/// <inheritdoc cref="IInternalActorRef"/>
public abstract void Resume(Exception causedByFailure = null);
Expand Down Expand Up @@ -530,9 +531,9 @@ public override IInternalActorRef Parent
}

/// <inheritdoc cref="InternalActorRefBase"/>
public override IActorRef GetChild(IEnumerable<string> name)
public override IActorRef GetChild(IReadOnlyList<string> name)
{
if (name.All(string.IsNullOrEmpty))
if (name.All(x => string.IsNullOrEmpty(x)))
return this;
return ActorRefs.Nobody;
}
Expand Down Expand Up @@ -874,20 +875,17 @@ override def getChild(name: Iterator[String]): InternalActorRef = {
/// </summary>
/// <param name="name">TBD</param>
/// <returns>TBD</returns>
public override IActorRef GetChild(IEnumerable<string> name)
public override IActorRef GetChild(IReadOnlyList<string> name)
{
//Using enumerator to avoid multiple enumerations of name.
var enumerator = name.GetEnumerator();
if (!enumerator.MoveNext())
{
//name was empty
if (name.Count == 0)
return this;
}
var firstName = enumerator.Current;

var firstName = name[0];
if (string.IsNullOrEmpty(firstName))
return this;
if (_children.TryGetValue(firstName, out var child))
return child.GetChild(new Enumerable<string>(enumerator));
return child.GetChild(name.NoCopySlice(1));
return ActorRefs.Nobody;
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/Akka/Actor/ActorRefProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ public IActorRef ResolveActorRef(ActorPath path)
/// <param name="actorRef">TBD</param>
/// <param name="pathElements">TBD</param>
/// <returns>TBD</returns>
internal IInternalActorRef ResolveActorRef(IInternalActorRef actorRef, IReadOnlyCollection<string> pathElements)
internal IInternalActorRef ResolveActorRef(IInternalActorRef actorRef, IReadOnlyList<string> pathElements)
{
if (pathElements.Count == 0)
{
Expand Down
53 changes: 28 additions & 25 deletions src/core/Akka/Actor/LocalActorRef.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Akka.Dispatch;
using Akka.Dispatch.SysMsg;
using Akka.Util.Internal;
using Akka.Util.Internal.Collections;

namespace Akka.Actor
{
Expand Down Expand Up @@ -52,7 +53,8 @@ public class LocalActorRef : ActorRefWithCell, ILocalRef
/// <param name="mailboxType">TBD</param>
/// <param name="supervisor">TBD</param>
/// <param name="path">TBD</param>
public LocalActorRef(ActorSystemImpl system, Props props, MessageDispatcher dispatcher, MailboxType mailboxType, IInternalActorRef supervisor, ActorPath path)
public LocalActorRef(ActorSystemImpl system, Props props, MessageDispatcher dispatcher, MailboxType mailboxType,
IInternalActorRef supervisor, ActorPath path)
{
_system = system;
_props = props;
Expand All @@ -61,18 +63,19 @@ public LocalActorRef(ActorSystemImpl system, Props props, MessageDispatcher disp
_supervisor = supervisor;
_path = path;

/*
* Safe publication of this class’s fields is guaranteed by Mailbox.SetActor()
* which is called indirectly from ActorCell.init() (if you’re wondering why
* this is at all important, remember that under the CLR readonly fields are only
* frozen at the _end_ of the constructor, but we are publishing "this" before
* that is reached).
* This means that the result of NewActorCell needs to be written to the field
* _cell before we call init and start, since we can start using "this"
* object from another thread as soon as we run init.
*/
/*
* Safe publication of this class’s fields is guaranteed by Mailbox.SetActor()
* which is called indirectly from ActorCell.init() (if you’re wondering why
* this is at all important, remember that under the CLR readonly fields are only
* frozen at the _end_ of the constructor, but we are publishing "this" before
* that is reached).
* This means that the result of NewActorCell needs to be written to the field
* _cell before we call init and start, since we can start using "this"
* object from another thread as soon as we run init.
*/
// ReSharper disable once VirtualMemberCallInConstructor
_cell = NewActorCell(_system, this, _props, _dispatcher, _supervisor); // _cell needs to be assigned before Init is called.
_cell = NewActorCell(_system, this, _props, _dispatcher,
_supervisor); // _cell needs to be assigned before Init is called.
_cell.Init(true, MailboxType);
}

Expand Down Expand Up @@ -211,14 +214,13 @@ public override IInternalActorRef GetSingleChild(string name)
}

/// <inheritdoc/>
public override IActorRef GetChild(IEnumerable<string> name)
public override IActorRef GetChild(IReadOnlyList<string> name)
{
var current = (IActorRef)this;
IActorRef current = this;
int index = 0;
foreach (string element in name)
foreach (var element in name)
{
var currentLocalActorRef = current as LocalActorRef;
if (currentLocalActorRef != null)
if (current is LocalActorRef currentLocalActorRef)
{
switch (element)
{
Expand All @@ -232,20 +234,21 @@ public override IActorRef GetChild(IEnumerable<string> name)
break;
}
}
else
else if (current is IInternalActorRef internalActorRef)
{
//Current is not a LocalActorRef
if (current != null)
{
var rest = name.Skip(index).ToList();
return current.AsInstanceOf<IInternalActorRef>().GetChild(rest);
}
var rest = name.NoCopySlice(index);
return internalActorRef.GetChild(rest);
}
else // not a LocalActorRef or an IInternalActorRef
{
throw new NotSupportedException("Bug, we should not get here");
}

index++;
}

return current;
}
}
}

}
Loading