-
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
Improve performance on IActorRef.Child
API
#5242
Improve performance on IActorRef.Child
API
#5242
Conversation
@@ -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) { } |
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.
Source-compatible, but not necessarily a binary-compatible change.
Looks like this may have broken cluster formation based on some of the testing I'm doing locally. I'll check it out some once I get reports back from the test suite. |
Looks like there's some work still to be done with the |
So once #5243 gets merged, new baseline numbers for New numbers BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
Numbers for this PR: BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
Top level actor resolves (i.e. ones that go through Akka.Cluster.Sharding) got a lot faster, but ones beneath the top-level actors stayed pretty much the same. |
…heweb/akka.net into perf/improve-ResolveAPI
This reverts commit 8cce9b7.
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
On this PR: Forgot to add baseline numbers for this: BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
|
|
||
public T this[int index] | ||
{ | ||
get => _array.ElementAt(Offset + index); |
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.
Since _array
is an IReadOnlyList<T>
here we may be better off using the Indexer on that interface rather than ElementAt
.
- If
_array
does not also implementIList<T>
we will force an enumeration .ElementAt
adds additional null and type checks even in best case.
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.
Ah yes, that's a bit of leftovers from when I was using IReadOnlyCollection
instead of IReadOnlyList
- the former doesn't have an indexer. I'll update that.
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
Even better numbers after fixing the indexer operation BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
|
Although IMHO those differences are probably in the margin of error |
I wonder if getting rid of these deeply nested |
Perf numbers from simplified branching: BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
|
Perf numbers from removing redundant BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19041.1165 (2004/May2020Update/20H1)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=5.0.302
[Host] : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
DefaultJob : .NET Core 3.1.17 (CoreCLR 4.700.21.31506, CoreFX 4.700.21.31502), X64 RyuJIT
|
@@ -93,7 +93,6 @@ namespace Akka.Actor | |||
public System.Collections.Generic.IEnumerable<Akka.Actor.IInternalActorRef> GetChildren() { } | |||
public static Akka.Actor.IActorRef GetCurrentSelfOrNoSender() { } | |||
public static Akka.Actor.IActorRef GetCurrentSenderOrNoSender() { } | |||
[System.ObsoleteAttribute("Use TryGetSingleChild [0.7.1]")] |
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.
Removed TryGetSingleChild
since no one was actually using its boolean functionality at all
|
||
var child = GetChild(concatenatedChildNames); | ||
var child = GetChild(concatenatedChildNames.ToList()); |
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.
Extra allocation in remote deployments here.... might see if I can go about fixing that still....
@@ -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()); |
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.
Another allocation in remote deployments
@@ -353,58 +353,37 @@ private bool TryGetChildRestartStatsByName(string name, out ChildRestartStats ch | |||
/// </summary> | |||
/// <param name="name">N/A</param> | |||
/// <returns>N/A</returns> | |||
[Obsolete("Use TryGetSingleChild [0.7.1]")] | |||
public IInternalActorRef GetSingleChild(string name) |
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.
Half the perf optimizations happened here:
- Simplified branching
- Remove
Try... out
stuff since it was all functionally being converted back intoActorRefs.Nobody
anyway.
/// <see cref="ArraySegment{T}"/> but for <see cref="IReadOnlyList{T}"/> | ||
/// </summary> | ||
/// <typeparam name="T"></typeparam> | ||
internal struct ListSlice<T> : IList<T>, IReadOnlyList<T> |
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.
Basically an ArraySegment
that takes a type of IReadOnlyList<T>
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
Builds upon #5241, but introduces some breaking API changes.
Performance numbers:
dev
This PR