-
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
Add new ChannelTaskScheduler Extension #5403
Add new ChannelTaskScheduler Extension #5403
Conversation
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.
The code looks fine - I think we also need to add:
- Some configuration specs, validating that default values can be successfully changed via HOCON
- Some documentation additions to https://getakka.net/articles/actors/dispatchers.html explaining how to configure this and when you might want to use it
- Address some of my inline comments, which ask for more clarification on the internals so other contributors / maintainers can reason about the algorithm.
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.
I added the comments and rollback dependency to 5.0.0
About the https://getakka.net/articles/actors/dispatchers.html where to edit it, can u do it? |
I’ll reply with a link to the .MD file once I’m back at my desk. On mobile at the moment.
…Sent from my iPhone
On Dec 3, 2021, at 8:11 AM, Andreas Dirnberger ***@***.***> wrote:
About the https://getakka.net/articles/actors/dispatchers.html where to edit it, can u do it?
The only thing that changed between the last channel-executor and this,
is the channel-executor.priority config instead of the max-concurrency calculated from fork-join-executor settings
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Yep, I can do it. |
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
|
||
sealed class PriorityTaskScheduler : TaskScheduler, IDisposable | ||
{ | ||
readonly Channel<Task> _channel; |
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.
Would there be any benefit to just using a minimal abstraction over ConcurrentQueue<Task>
for the common case of multiple readers (where an UnboundedChannel would be created)?
I mention this because UnboundedChannel.TryWrite
involves a lock on a parent SyncObject, which is primarily used for waiting completions and async readers. This will likely impact max throughput. We do use the Async Completions in the control section but I wonder if there is a way we can work around that too?
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.
Good point, I will take a look the the Channel source code too.
In the end the queue should lock-free queued and dequeued,
where adding side should have the heavier processing (splitting in priorities)
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.
I looked up and yes there is a brief lock.
The lock will not have much perf. issue on the executing thread itself,
but of course on locking threads. Don't know why it is not implemented in a free-lock style.
Maybe the lock collation just don't happen that often to take it into effect?
I trust "stephentoub" in his decision to use a lock here.
The lock
statement is on itself is not "bad" but misused it can be very fast.
The main benefit of the Channel class is following line, it is reducing the idle cpu to near zero:
https://github.com/dotnet/runtime/blob/386f87139c7fe48f6b733e3cae6d09128c827192/src/libraries/System.Threading.Channels/src/System/Threading/Channels/UnboundedChannel.cs#L295
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.
I rechecked now the UnboundedChannel.TryWrite
code
- it would be absolutely hard not to use a sync lock
- Collisions are very unlikely, even under heavy queue load
As requested inlining https://github.com/Zetanova/Akka.Experimental.ChannelTaskScheduler into akka and using it as the default
channel-executor
usage like before, the
channel-executor.priority
config value is already set in the default config.to activate use:
All akka components can thread-safe access the priority TaskSchedulers over: