Akka.Cluster.Sharding: perf optimize message extraction, automate StartEntity and ShardEnvelope handling
#6863
Conversation
|
Not compiling yet. |
|
Going to need to run these benchmarks on a different machine that's not being actively used by yours truly to do their daily work.... |
|
Updated baseline numbers:
|
This PR
|
|
Looking at these numbers - these change have definitely helped by just reducing the amount of work being performed each time a message is routed via the |
|
Doh, compilation error - I'll need to fix that tomorrow. |
| return wr.Ref; | ||
| } | ||
| if (!_entities.TryGetValue(entityId, out var state)) return null; | ||
| if (state is WithRef wr) |
There was a problem hiding this comment.
NIT:
Not sure whether this would be better:
if (_entities.TryGetValue(entityId, out var state) && state is WithRef wr)
{
return wr.Ref;
}
else
{
return null;
}
But figured it may be worth the ask. If nothing else it lowers dependence on compiler/jitter for epilog.
| { | ||
| self.Tell(new LeaseLost(reason)); | ||
| }).ContinueWith(r => | ||
| lease.Acquire(reason => { self.Tell(new LeaseLost(reason)); }).ContinueWith(r => |
There was a problem hiding this comment.
weird churn compared to the rest of the churn....
There was a problem hiding this comment.
I think that was just me auto-formatting the document when I was doing things like removing nested switch statements
This reverts commit 7c0a7f4.
Aaronontheweb
force-pushed
the
fix-6717-take-2
branch
from
c4e396b
to
b431ebc
Compare
| @@ -5,15 +5,18 @@ | |||
| // </copyright> | |||
| //----------------------------------------------------------------------- | |||
|
|
|||
| #nullable enable | |||
There was a problem hiding this comment.
Making older legacy code support nullable as we go
| /// | ||
| /// Used to automatically handle built-in sharding messages when used with ClusterSharding. | ||
| /// </summary> | ||
| internal sealed class ExtractorAdapter : IMessageExtractor |
There was a problem hiding this comment.
This is the primary piece of functionality for handling built-in sharding messages automatically, namely ShardEnvelope and StartEntity
| { | ||
| return message switch | ||
| { | ||
| ShardingEnvelope se => se.EntityId, |
There was a problem hiding this comment.
Looks like I need to add handling for StartEntity here - although I think there was a reason why I didn't (old PR -I'll look)
| @@ -136,6 +190,12 @@ public virtual string ShardId(object message) | |||
|
|
|||
| return _cachedIds[(Math.Abs(MurmurHash.StringHash(id)) % MaxNumberOfShards)]; | |||
| } | |||
|
|
|||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public virtual string ShardId(string entityId, object? messageHint = null) | |||
There was a problem hiding this comment.
So this is a really significant change and the source of our performance improvements: we need to have the ability to compute ShardIds when the EntityId is known already. Right now none of the existing IMessageExtractor implementations support this and instead require you to perform the following round trip:
sequenceDiagram
participant User as User
participant IMessageExtractor as IMessageExtractor
Note over User, IMessageExtractor: Message Routing with Pre-Calculated EntityId
User->>IMessageExtractor: Pass entire message
Note right of IMessageExtractor: Method: IMessageExtractor.ShardId(object)
IMessageExtractor->>IMessageExtractor: Compute EntityId
Note right of IMessageExtractor: EntityId already known, but re-computed
IMessageExtractor->>IMessageExtractor: Compute ShardId from EntityId
Note right of IMessageExtractor: ShardId derived from EntityId
We have now changed this flow to support:
sequenceDiagram
participant User as User
participant System as System
Note over User, System: Message Routing with Pre-Calculated EntityId
User->>System: Route message with EntityId
Note right of User: EntityId is already calculated and available
System->>System: Compute ShardId from EntityId
Note right of System: ShardId computed directly from EntityId without evaluating message
There was a problem hiding this comment.
Worth noting: this method can't ever really be called without knowing the entityId in advance, at least from the internals of Akka.Cluster.Sharding.
| @@ -564,12 +622,11 @@ public static Config DefaultConfig() | |||
| IShardAllocationStrategy allocationStrategy, | |||
| object handOffStopMessage) | |||
| { | |||
| return Start( | |||
| return InternalStart( | |||
There was a problem hiding this comment.
Added these methods so we can convert the MessageExtractor / ShardExtractor delegates back into an IMessageExtractor and use that internally instead.
| @@ -952,15 +950,16 @@ Address GetNodeAddress(IActorRef shardOrRegionRef) | |||
| try | |||
| { | |||
| var entityId = getEntityLocation.EntityId; | |||
| var shardId = _extractShardId(new StartEntity(getEntityLocation.EntityId)); | |||
| var shardId = _messageExtractor.ShardId(getEntityLocation.EntityId, | |||
There was a problem hiding this comment.
Another benefit of this change is that it greatly simplifies shard location queries
| @@ -1094,29 +1099,30 @@ private void HandleCoordinatorMessage(ShardCoordinator.ICoordinatorMessage messa | |||
| switch (message) | |||
| { | |||
| case ShardCoordinator.HostShard hs: | |||
| { | |||
There was a problem hiding this comment.
All of these changes are just code reformatting - no substantive changes
| @@ -1152,54 +1160,56 @@ private void HandleCoordinatorMessage(ShardCoordinator.ICoordinatorMessage messa | |||
| TryRequestShardBufferHomes(); | |||
| break; | |||
| case ShardCoordinator.BeginHandOff bho: | |||
| { | |||
| var shard = ho.Shard; | ||
| _log.Debug("{0}: HandOff shard [{1}]", _typeName, shard); | ||
| { | ||
| var shard = ho.Shard; |
| if (_shardsByRef.Values.All(shardId => shardId != id)) | ||
| { | ||
| _log.Debug("{0}: Starting shard [{1}] in region", _typeName, id); | ||
| if (_shards.TryGetValue(id, out var shard)) return shard ?? ActorRefs.Nobody; |
…ka.net into fix-6717-take-2
|
Discussed with @Arkatufus - going to add some upgrade advisories for Akka.NET v1.5.15 since it includes a few major new changes. |
laying the groundwork for akkadotnet/akka.net#6863 here - besides, it looks like the old delegates simply can't handle certain messages automatically due to akkadotnet/akka.net#7051
laying the groundwork for akkadotnet/akka.net#6863 here - besides, it looks like the old delegates simply can't handle certain messages automatically due to akkadotnet/akka.net#7051
Changes
Fixes #6717
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
ShardingEnvelopeandStartEntitywithout forcing user to define handlers #6717Latest
devBenchmarksInclude data from the relevant benchmark prior to this change here.
This PR's Benchmarks
Include data from after this change here.