4.x: Improve blocking First & Last operators #590
Conversation
| { | ||
| evt.Set(); | ||
| }))) | ||
| consumer.SetUpstream(d); |
There was a problem hiding this comment.
Is this needed? Any completion will unblock the consumer and d will be disposed by the using-block.
There was a problem hiding this comment.
The unblocking may happen any time later and the upstream would still push items to be dropped unnecessarily.
There was a problem hiding this comment.
Any time later, how?
There was a problem hiding this comment.
Pausing and resuming a thread takes time, about 5 microseconds on Windows based on my experience. During this time, the source may generate anything between zero to 1000 items, all of them wasting resources.
There was a problem hiding this comment.
Ok, you're taking that into account.
| namespace System.Reactive.Linq.ObservableImpl | ||
| { | ||
|
|
||
| internal abstract class BaseBlocking<T> : CountdownEvent, IObserver<T> |
There was a problem hiding this comment.
CountdownEvent vs. ManualResetEvent ?
There was a problem hiding this comment.
ManualResetEvent cannot be extended, CountdownEvent can, saving on an allocation.
| internal bool _hasValue; | ||
| internal Exception _error; | ||
|
|
||
| int once; |
There was a problem hiding this comment.
See above, ManualResetEvent would save this field.
There was a problem hiding this comment.
Allocation vs an atomic operation tradeoff. If Signal wouldn't throw by default, we'd not need this.
| { | ||
| this._value = value; | ||
| this._hasValue = true; | ||
| Disposable.TryDispose(ref _upstream); |
There was a problem hiding this comment.
Why no check here (or, why check above) ?
There was a problem hiding this comment.
OnNext is called in a serialized fashion and a previous call already set _hasValue to true. Checking that upfront guarantees this executes at most once, but OnCompleted may still run regardless of _upstream disposed so that we want to avoid doing again there.
There was a problem hiding this comment.
Why is the order (Unblock, Dispose) reversed?
There was a problem hiding this comment.
No practical reason other than the reverse will race the upstream disposition with the using disposing.
|
I'm thinking about whether CountdownEvent.Dispose should be overridden to dispose the upstream, not that it would conceptually be required but simply because there is the disposable field and the instance is disposable. |
|
Calling CountdownEvent.Dispose breaks the CountdownEvent and it cannot be unblocked after that. Methods will start throwing ObjectDisposedException as well. |
| Disposable.TryDispose(ref _upstream); | ||
| } | ||
|
|
||
| public override void OnError(Exception error) |
There was a problem hiding this comment.
Save for the IsDisposed-check, the overriding code is almost identical. Enough reason to push it to the base-class ?
There was a problem hiding this comment.
Last doesn't need that dispose check so why have it there. First needs it so why penalize everybody else?
There was a problem hiding this comment.
At least push some of the code into the base class.
What I mean is, if CountdownEvent.Dispose is called, should the upstream be disposed? |
|
It can't be called here as the class/instance is not exposed. |
|
Still, BaseBlocking implements IDisposable and holds a disposable, it should override and dispose of the upstream. |
|
Why? |
|
Maybe it should be put in a using block then and honour that it implements IDisposable. |
|
No. It makes no sense in this context. You dispose the subscription returned by |
|
Thinking about that, it might definitely make sense since the CountdownEvent probably holds onto an unmanaged resource and should be disposed as soon you're done with it. |
|
WaitAndSetOnce wrapped a ManualResetEvent before and it's in a using-block. |
Optimize the blocking
FirstandLast(==Wait) by inlining the various classes into one container and not waiting if the operation terminated by the timeSubscribereturned.