Add extra thread for scheduler, move TorControl and OpenAddedConnections to scheduler #18925
Conversation
|
So for every new heavy task that is added to the scheduler we have to add one more thread to not degrade the worst case performance? Imagine the wallet is flushed on scheduler thread 1 and the dns is queried on scheduler thread 2. Both block for minutes, so a third scheduler thread is needed. Sounds like this is missing the goal, and you might as well start a plain thread for each heavy task to begin with. I think what is needed is to have different queues. Maybe one for the validation interface (which validation might block on in the worst case), and other stuff like the wallet flushing or dns requests. |
Use only one thread for Tor callbacks so that callbacks block at most one scheduler thread.
naumenkogs
force-pushed
the
2020_05_extra_scheduler_thread
branch
from
8878cfe to
e3102ce
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
@MarcoFalke right, this solution really doesn't help much in the long term, beyond #18421. If we want something more long-term, we'd want a separate queue (with a dedicated thread). A trivial way to implement it is a second instance of a scheduler. But we may want something smarter, like modifying the existing scheduler. Not sure how to achieve that right now, but I will take another look. Suggestions welcome :) Note that the suggested changes still apply: we definitely should be able to free a thread by moving those 2 things into scheduler. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Closing this because no attention, and the arguments presented are probably oudated. |
3 participants
I’m exploring the opportunity to “save” threads by moving some of the stuff to the scheduler.
I need this so that we can make some room for blocking execution of #18421 and other similar features in future. Later I want to add #18421 to the scheduler too.
@laanwj initially suggested to use libevent for #18421, but @TheBlueMatt pointed out that it’s undesirable to add that extra dependency on somewhat limited non-standard libevent functions.
Matt also suggested an idea of this PR as an alternative.
I suggest to compare the safety of this approach to status-quo (which seems to be working well).
If we’ll have 2 threads for scheduler, the requirement is at most one long blocking task is allowed.
Right now, everything in scheduling is pretty fast. Tor stuff and OpenAddedConnections both seem to be fast too (that’s where I’d use some extra eyes and experience!), so we’re safe, and there’s even room for an extra task. (#18421).
If there are any concerns, note that we can drop either Tor or OpenAddedConnections from the scheduler, and still be fine. Or we can add third thread to the scheduler, and be even with status quo.