refactor: Release cs_main before MaybeSendFeefilter #24427

1. 
   MarcoFalke commented at 9:28 am on February 23, 2022: member

   There is no need for any lock to be held, because net processing is single threaded. So holding the validation lock cs_main for sending a feefilter is confusing and might even degrade blockchain-related RPC performance minimally.
2. refactor: Release cs_main before MaybeSendFeefilter faa329fd46
3. DrahtBot added the label P2P on Feb 23, 2022
4. DrahtBot added the label Refactoring on Feb 23, 2022
5. 
   vasild commented at 11:33 am on February 23, 2022: member

   Approach ACK
6. 
   DrahtBot commented at 9:52 am on February 24, 2022: 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:

   • #24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
   • #22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
   • #21527 (net_processing: lock clean up by ajtowns)
   • #21160 (net/net processing: Move tx inventory into net_processing by jnewbery)

   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.

7. vasild approved
8. 
   vasild commented at 2:55 pm on February 24, 2022: member

   ACK faa329fd46b4d688a5aa728b8b22a8d098987588

   Message processing is single threaded.

   I think all the variables accessed directly or indirectly by MaybeSendFeefilter() are:

   1. not accessed by another thread; or
   2. accessed by another thread after acquiring the appropriate lock (e.g. CChainState::m_chain is accessed by CChainState::IsInitialBlockDownload() but it acquires cs_main before that); or
   3. accessed by another thread without protection but only modified by “init” (::minRelayTxFee), that’s a bit scary but is ok wrt this PR
9. 
   ajtowns commented at 4:59 am on March 2, 2022: member

   ACK faa329fd46b4d688a5aa728b8b22a8d098987588 ; code review only

   I don’t think this breaks anything, or even makes anything more confusing. Not convinced by the “might even degrade performance” though – this patch might equally degrade p2p performance if someone else gains cs_main after it’s released, then also gains mempool.cs which then blocks the currentFilter = .. GetMinFee .. call which also wants mempool.cs.

10. 
    vasild commented at 7:27 am on March 2, 2022: member

    My understanding of the PR description is that holding cs_main for longer “might degrade performance” (which is the case in master without this PR). Not that this PR “might degrade performance”.

    This PR just holds cs_man for a shorter time compared to master.

11. 
    ajtowns commented at 7:29 am on March 2, 2022: member

    Yes – holding cs_main for longer (as in current master) might degrade performance of some other thread trying to gain cs_main; but not holding it (as in this PR) might degrade performance if some other thread gains cs_main then a lock we care about (mempool.cs).
12. 
    MarcoFalke commented at 11:52 am on March 2, 2022: member

    Sure, the net processing thread might be forced to sleep for an unknown amount of time before calling MaybeSendFeefilter. (Unrelated, this may already happen on current master, as the CPU scheduler is free to put any thread to sleep just “for fun” if it wants to.) I don’t think the changes here may decrease average performance (average throughput / the rate at which MaybeSendFeeFilter is called), though? If there are multiple thread that are competing for a lock in a loop with a short time releasing the lock, at any time at most one thread can hold the lock. Assuming a round robin scheduler to hand out the lock to the threads asking for it, I couldn’t come up with an example where the average performance is consistently worse with the patch here, but maybe I am missing something?
13. MarcoFalke added this to the milestone 24.0 on Mar 2, 2022
14. 
    MarcoFalke commented at 2:51 pm on March 2, 2022: member

    Oh, sorry. Quite obviously, assuming a round robin scheduler for giving out the locks to threads, the average performance might decrease if there is more than one thread competing with the message processing thread. (Or alternatively if go further down the route to move more stuff out of cs_main, a single competing thread should be enough).
15. 
    vasild commented at 3:00 pm on March 2, 2022: member

    Ok, but moving stuff out of cs_main is the way to achieve more concurrency and thus better performance of the entire program.
16. 
    ajtowns commented at 3:06 pm on March 2, 2022: member

    Sorry, didn’t mean to make a thing of that – I think the key word for the performance degradation in both cases is “minimally” :)
17. MarcoFalke merged this on Mar 7, 2022
18. MarcoFalke closed this on Mar 7, 2022

    ---

19. MarcoFalke deleted the branch on Mar 7, 2022
20. sidhujag referenced this in commit 4fe332d87c on Mar 7, 2022
21. Fabcien referenced this in commit de321ad806 on Oct 10, 2022
22. DrahtBot locked this on Mar 7, 2023

Contributors
MarcoFalke vasild DrahtBot ajtowns

Labels
Refactoring P2P

Milestone
24.0