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

Akka.Cluster.Sharding: perf optimize message extraction, automate StartEntity and ShardEnvelope handling #6863

Merged
merged 17 commits into from
Jan 4, 2024

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Jul 31, 2023

Changes

Fixes #6717

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Latest dev Benchmarks

Include data from the relevant benchmark prior to this change here.

This PR's Benchmarks

Include data from after this change here.

@Aaronontheweb
Copy link
Member Author

Not compiling yet.

@Aaronontheweb
Copy link
Member Author

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

@Aaronontheweb
Copy link
Member Author

Updated baseline numbers:


BenchmarkDotNet v0.13.6, Windows 10 (10.0.19045.3086/22H2/2022Update)
Intel Core i7-8850H CPU 2.60GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 7.0.101
  [Host]     : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
  Job-MDNESA : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2

InvocationCount=1  LaunchCount=10  RunStrategy=Monitoring  
UnrollFactor=1  WarmupCount=10  

Method StateMode Mean Error StdDev Median Req/sec
SingleRequestResponseToLocalEntity Persistence 5,516.2 ns 243.88 ns 433.50 ns 5,517.0 ns 181,285.01
StreamingToLocalEntity Persistence 329.8 ns 80.76 ns 53.42 ns 308.1 ns 3,032,011.98
SingleRequestResponseToRemoteEntity Persistence 439,118.3 ns 14,448.48 ns 25,682.16 ns 454,157.2 ns 2,277.29
SingleRequestResponseToRemoteEntityWithLocalProxy Persistence 440,988.8 ns 10,242.54 ns 30,200.35 ns 422,129.6 ns 2,267.63
StreamingToRemoteEntity Persistence 34,053.8 ns 1,352.97 ns 3,989.27 ns 35,017.9 ns 29,365.32
SingleRequestResponseToLocalEntity DData 5,914.5 ns 141.14 ns 416.14 ns 5,885.0 ns 169,077.37
StreamingToLocalEntity DData 556.8 ns 78.01 ns 230.00 ns 507.7 ns 1,795,877.67
SingleRequestResponseToRemoteEntity DData 484,771.2 ns 8,693.48 ns 25,632.93 ns 481,258.1 ns 2,062.83
SingleRequestResponseToRemoteEntityWithLocalProxy DData 481,280.8 ns 12,037.04 ns 35,491.48 ns 485,805.3 ns 2,077.79
StreamingToRemoteEntity DData 32,029.4 ns 1,282.13 ns 3,780.38 ns 32,204.8 ns 31,221.36

@Aaronontheweb
Copy link
Member Author

This PR


BenchmarkDotNet v0.13.6, Windows 10 (10.0.19045.3086/22H2/2022Update)
Intel Core i7-8850H CPU 2.60GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 7.0.101
  [Host]     : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
  Job-XPEEFP : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2

InvocationCount=1  LaunchCount=10  RunStrategy=Monitoring  
UnrollFactor=1  WarmupCount=10  

Method StateMode Mean Error StdDev Median Req/sec
SingleRequestResponseToLocalEntity Persistence 5,279.3 ns 136.74 ns 403.17 ns 5,253.8 ns 189,418.71
StreamingToLocalEntity Persistence 316.6 ns 30.45 ns 54.12 ns 296.1 ns 3,158,175.65
SingleRequestResponseToRemoteEntity Persistence 436,199.2 ns 33,187.63 ns 38,218.93 ns 439,133.7 ns 2,292.53
SingleRequestResponseToRemoteEntityWithLocalProxy Persistence 495,668.7 ns 9,278.84 ns 27,358.87 ns 505,838.7 ns 2,017.48
StreamingToRemoteEntity Persistence 33,484.1 ns 697.96 ns 2,057.94 ns 32,555.8 ns 29,864.89
SingleRequestResponseToLocalEntity DData 5,402.0 ns 168.90 ns 498.00 ns 5,336.8 ns 185,117.83
StreamingToLocalEntity DData 504.3 ns 32.56 ns 95.99 ns 489.9 ns 1,983,053.62
SingleRequestResponseToRemoteEntity DData 457,946.0 ns 10,014.75 ns 29,528.70 ns 478,323.6 ns 2,183.66
SingleRequestResponseToRemoteEntityWithLocalProxy DData 454,190.9 ns 7,707.67 ns 22,726.24 ns 444,002.9 ns 2,201.72
StreamingToRemoteEntity DData 26,895.4 ns 692.54 ns 2,041.98 ns 26,105.7 ns 37,181.02

@Aaronontheweb
Copy link
Member Author

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 ShardRegion. Some of the improvements are more marginal in the most expensive benchmarks (i.e. SingleRequestResponseToRemoteEntityWithLocalProxy) but the trend is clear. Going to try one more thing to boot.

@Aaronontheweb
Copy link
Member Author

Doh, compilation error - I'll need to fix that tomorrow.

Copy link
Member

@to11mtm to11mtm left a comment

Choose a reason for hiding this comment

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

Started reviewing, will continue....

return wr.Ref;
}
if (!_entities.TryGetValue(entityId, out var state)) return null;
if (state is WithRef wr)
Copy link
Member

Choose a reason for hiding this comment

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

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 =>
Copy link
Member

Choose a reason for hiding this comment

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

weird churn compared to the rest of the churn....

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that was just me auto-formatting the document when I was doing things like removing nested switch statements

Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Detailed my changes

@@ -5,15 +5,18 @@
// </copyright>
//-----------------------------------------------------------------------

#nullable enable
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Choose a reason for hiding this comment

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

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,
Copy link
Member Author

Choose a reason for hiding this comment

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

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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Loading

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
Loading

Copy link
Member Author

Choose a reason for hiding this comment

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

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(
Copy link
Member Author

Choose a reason for hiding this comment

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

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,
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 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:
{
Copy link
Member Author

Choose a reason for hiding this comment

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

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:
{
Copy link
Member Author

Choose a reason for hiding this comment

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

More reformatting

var shard = ho.Shard;
_log.Debug("{0}: HandOff shard [{1}]", _typeName, shard);
{
var shard = ho.Shard;
Copy link
Member Author

Choose a reason for hiding this comment

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

More reformatting

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;
Copy link
Member Author

Choose a reason for hiding this comment

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

Reformatting

@Aaronontheweb Aaronontheweb marked this pull request as ready for review January 4, 2024 18:03
@Aaronontheweb
Copy link
Member Author

Discussed with @Arkatufus - going to add some upgrade advisories for Akka.NET v1.5.15 since it includes a few major new changes.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb merged commit fd41a82 into akkadotnet:dev Jan 4, 2024
12 checks passed
@Aaronontheweb Aaronontheweb deleted the fix-6717-take-2 branch January 4, 2024 19:56
Aaronontheweb added a commit to Aaronontheweb/Akka.Hosting that referenced this pull request Jan 9, 2024
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
Arkatufus pushed a commit to akkadotnet/Akka.Hosting that referenced this pull request Jan 9, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants