Add extra thread for scheduler, move TorControl and OpenAddedConnections to scheduler #18925

1. 
   naumenkogs commented at 7:10 pm on May 9, 2020: member

   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.

2. Add an extra thread for scheduler be20ea310f
3. 
   MarcoFalke commented at 7:25 pm on May 9, 2020: member

   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.

4. Move Tor to the scheduler

   Use only one thread for Tor callbacks so that
   callbacks block at most one scheduler thread.

   8919f79f12
5. Use scheduler for connecting to added nodes e3102ced1e
6. naumenkogs force-pushed on May 9, 2020
7. DrahtBot added the label Docs on May 9, 2020
8. DrahtBot added the label P2P on May 9, 2020
9. 
   DrahtBot commented at 3:15 am on May 10, 2020: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #19316 ([net] Cleanup logic around connection types by amitiuttarwar)
   • #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
   • #19064 (refactor: Cleanup thread ctor calls by hebasto)
   • #19028 (test: Set -logthreadnames in unit tests by MarcoFalke)
   • #18991 (Cache responses to GETADDR to prevent topology leaks by naumenkogs)
   • #15421 (torcontrol: Launch a private Tor instance when not already running by luke-jr)

   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.

10. 
    naumenkogs commented at 2:07 pm on May 12, 2020: member

    @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.

11. 
    DrahtBot commented at 4:31 pm on July 1, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

12. DrahtBot added the label Needs rebase on Jul 1, 2020
13. 
    naumenkogs commented at 12:36 pm on September 15, 2021: member

    Closing this because no attention, and the arguments presented are probably oudated.
14. naumenkogs closed this on Sep 15, 2021

    ---

15. DrahtBot locked this on Oct 30, 2022

Contributors
naumenkogs MarcoFalke DrahtBot

Labels
Docs P2P Needs rebase