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.Actor: tuck all scheduled Tell messages into IScheduledMsg envelope #6461

Merged
merged 11 commits into from Aug 25, 2023

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Feb 27, 2023

Changes

Adds some overhead to actor message processing (minor), but makes it possible for us to detect scheduled messages and filter them out from telemetry. Doesn't need to ship as part of v1.5 - can be done after.

Checklist

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

@Aaronontheweb
Copy link
Member Author

Motivation for this is primarily telemetry - making it easier to determine what is and is not a scheduled message. I'd probably rather make this a 1.5.1 thing though since I think I need more time to let it cook...

@Arkatufus Arkatufus marked this pull request as ready for review August 23, 2023 22:01
@Aaronontheweb
Copy link
Member Author

@Arkatufus if the tests look good, next thing we need to run are benchmarks - InMemoryPingPong and RemotePingPong, since this touches the hotpath in the ActorCell.

@Arkatufus
Copy link
Contributor

Arkatufus commented Aug 24, 2023

RemotePingPong Benchmark

Values are a median of 5 runs

New Code

Num clients Total [msg] Msg/sec Total [ms] Start Threads End Threads
1 200000 138409 1445.25 131 158
5 1000000 611996 1634.23 166 174
10 2000000 676590 2956.19 177 181
15 3000000 666815 4499.7 189 193
20 4000000 664784 6017.72 207 199
25 5000000 670152 7461.99 199 191
30 6000000 665042 9022.91 199 181

Dev Branch

Num clients Total [msg] Msg/sec Total [ms] Start Threads End Threads
1 200000 131927 1516.93 132 159
5 1000000 608273 1644.59 166 171
10 2000000 674082 2967.81 179 180
15 3000000 669942 4478.95 189 200
20 4000000 664452 6020.36 207 219
25 5000000 670961 7452.86 207 191
30 6000000 668748 8972.71 200 182

@Arkatufus
Copy link
Contributor

Akka.Benchmark PingPongBenchmarks Result

Dev Branch

Method Mean Error StdDev Req/sec
Actor_ping_pong_single_pair_in_memory 384.2 ns 5.89 ns 17.38 ns 2,603,016.96

New Code

Method Mean Error StdDev Req/sec
Actor_ping_pong_single_pair_in_memory 381.9 ns 3.95 ns 11.64 ns 2,618,609.51

@Aaronontheweb
Copy link
Member Author

No negative performance impact - that's good.

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.

Left some suggestions

namespace Akka.Actor.Scheduler
{
[Akka.Annotations.InternalApiAttribute()]
public interface IScheduledTellMsg : Akka.Actor.INoSerializationVerificationNeeded, Akka.Actor.IWrappedMessage { }
Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixing up some bad test changes - looks fine

src/core/Akka/Actor/Scheduler/HashedWheelTimerScheduler.cs Outdated Show resolved Hide resolved
@Aaronontheweb Aaronontheweb merged commit a91bb5e into akkadotnet:dev Aug 25, 2023
10 of 12 checks passed
@Aaronontheweb Aaronontheweb deleted the scheduler-envelope branch August 25, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants