To support linux better, replace operation queue with dispatch queue in BatchingFutureOperationQueue #31
Conversation
OmSaran
force-pushed
the
batch-dispatchq
branch
2 times, most recently
from
3e16d1f to
d4b5e97
Compare
| self.group = group | ||
| self.operationQueue = OperationQueue(tsf_withName: name, maxConcurrentOperationCount: maxOpCount) | ||
| self.operationQueue.qualityOfService = qualityOfService | ||
| self.dispatchQueue = DispatchQueue(label: name, qos: dispatchQoS, attributes: .concurrent) |
There was a problem hiding this comment.
hmm, .concurrent DispatchQueues are not recommended for a number of years now. Additionally, they're really quite slow to spawn new threads on Linux (can introduce a latency of up to 100 ms).
I'm not quite sure why we need the DispatchQueue at all?
[please ping me personally about the removed code here which doesn't make sense for this repo]
| return self.concurrencyLimiter.withReplenishableLimit(eventLoop: group.any()) { eventLoop in | ||
| let promise = eventLoop.makePromise(of: T.self) | ||
|
|
||
| DispatchQueue(label: self.name, qos: self.qos).async { |
There was a problem hiding this comment.
here you don't need to dispatch off. The body returns a future and therefore will be non-blocking.
There was a problem hiding this comment.
Yes, ideally that should be the case, but if I understand correctly, a rogue body could still do blocking operations? Executing in a dispatch queue would keep us more aligned with the previous implementation where we do not block the event loop in such a case.
For e.g.,
batchFutOpQ.execute {
someBlockingCall()
return elg.makeSucceededFuture(())
}
(Note that this still blocks if there is a blocking call inside flatMap like return elg.makeSucceededFuture(()).flatMap{ someBlockingCall() }. But this would be the same behavior in previous implementation)
There was a problem hiding this comment.
Yes, that's true. But a rogue body could also just crash the program or worse. We shouldn't write overly defensive code and if we find such a rogue body, we should fix it instead. Every dispatch to another thread (and back) costs us performance so we should avoid it as much as possible.
There was a problem hiding this comment.
Understood now, makes sense. While I do agree we need to fix that, I think it is a non trivial task (pls correct me if I'm wrong here) to find and fix all usages. So I think it is better to do that as an enhancement as a different task since the objective of the task is to only get rid of using OperationQueue. Does that sound fair?
There was a problem hiding this comment.
Have you seen any code that uses this which is blocking and returns a future? There shouldn't really be any such code (of course there might but I'd hope it's not widespread).
OmSaran
force-pushed
the
batch-dispatchq
branch
from
764cf1c to
7a52a3e
Compare
…in BatchingFutureOperationQueue The operation queue implementation is deprecated
version of token bucket. Add tests
OmSaran
force-pushed
the
batch-dispatchq
branch
from
7a52a3e to
7ad076c
Compare
OmSaran
force-pushed
the
batch-dispatchq
branch
from
dd77e8c to
e577a58
Compare
The operation queue implementation is replaced