-
Notifications
You must be signed in to change notification settings - Fork 782
4.x: CurrentThreadScheduler fast path Schedule #599
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
4.x: CurrentThreadScheduler fast path Schedule #599
Conversation
|
The fast-path could be added to the timed case as well (i.e., sleep before executing the plain action, then trampoline if necessary). |
| /// <param name="action">Action to be executed.</param> | ||
| /// <returns>The disposable object used to cancel the scheduled action (best effort).</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="action"/> is <c>null</c>.</exception> | ||
| public override IDisposable Schedule<TState>(TState state, Func<IScheduler, TState, IDisposable> action) |
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.
Could you pack it into the timed overload for the dueTime == 0 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.
Maybe. I'm trying to avoid the time arithmetic overhead.
| } | ||
| finally | ||
| { | ||
| SetQueue(null); |
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.
If the queue is sufficiently small (maybe < 16 or so), we could think about just clearing the queue instead of recreating one next time.
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 don't know if the current capacity is actually available on this type of queue
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.
Nope, it doesn't work. There is no current capacity to ScheduledQueue and the PriorityQueue shrinks itself when the number of items is 25% of its internal capacity upon remove/dequeue.
| SetQueue(queue); | ||
| if (dueTime > TimeSpan.Zero) | ||
| { | ||
| ConcurrencyAbstractionLayer.Current.Sleep(dueTime); |
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.
Why can we omit the normalization of dueTime here?
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.
reactive/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.Windows.cs
Line 67 in d6d954f
| Normalize(timeout) |
reactive/Rx.NET/Source/src/System.Reactive/Concurrency/ConcurrencyAbstractionLayerImpl.cs
Line 64 in 11506de
| public void Sleep(TimeSpan timeout) => System.Threading.Thread.Sleep(Normalize(timeout)); |
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.
We don't mind if it is negative and Sleep will normalize it anyway.
This PR modifies the schedule logic of the
CurrentThreadSchedulerto avoid allocating the queue for the first task on an idle scheduler. If the action schedules further tasks in a reentrant manner, those will get queued up just like now.Since this scheduler is per thread, there is no need for locks/synchronization as each caller thread will only see its own thread-local structures and it can't cross-post to some other thread.