4.x: TakeLast() improve structure and recursive scheduling #697
Conversation
akarnokd
commented
Jun 28, 2018
akarnokd
commented
- Inline the relevant parent fields into the sinks
- Use a more efficient recursive scheduling disposable-tracking scheme
- Delegate to base.Run where applicable.
| private IDisposable _loopDisposable; | ||
| private IStopwatch _watch; | ||
|
|
||
| public void Run() | ||
| public void Run(IObservable<TSource> source, IScheduler scheduler) |
There was a problem hiding this comment.
scheduler is only needed to get the StartStopwatch before the operator executes.
| { | ||
| if (_queue.Count > 0) | ||
| { | ||
| ForwardOnNext(_queue.Dequeue()); | ||
| recurse(this); |
There was a problem hiding this comment.
Can you elaborate more on the issue with this?
There was a problem hiding this comment.
It is the very same situation with Range and ToObservable: the Action-based recursive scheduling is more expensive.
https://github.com/dotnet/reactive/pull/684/files#diff-76edecc51f6af371eae34be29849253aR62
There was a problem hiding this comment.
Any possibility to optimize the action based recursive scheduling? The syntax is much less noisy.
There was a problem hiding this comment.
I don't see how. It has to maintain some state across schedules. There was an optimization for it in #617, particularly the InvokeRec1State which holds a CompositeDisposable and creates a SingleAssignmentDisposable for each schedule invocation.
There was a problem hiding this comment.
Would local functions help? You can capture the state without needing a delegate.
There was a problem hiding this comment.
I think that still makes an allocation just like a capturing delegate would upon each invocation.
There was a problem hiding this comment.
Yes, it does still have a state variable for the capture, but no delegate variable. depends on the use case
There was a problem hiding this comment.
I think the issue here is, why does the action based scheduling have to keep track of all the schedulings in a CompositeDisposable. Seems odd, at least for tail recursion.
|
Is there something else needed for this PR? |
