refactor: Release cs_main before MaybeSendFeefilter #24427
Conversation
|
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. |
There was a problem hiding this comment.
ACK faa329f
Message processing is single threaded.
I think all the variables accessed directly or indirectly by MaybeSendFeefilter() are:
- not accessed by another thread; or
- accessed by another thread after acquiring the appropriate lock (e.g.
CChainState::m_chainis accessed byCChainState::IsInitialBlockDownload()but it acquirescs_mainbefore that); or - accessed by another thread without protection but only modified by "init" (
::minRelayTxFee), that's a bit scary but is ok wrt this PR
|
ACK faa329f ; 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 |
|
My understanding of the PR description is that holding This PR just holds |
|
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). |
|
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.) |
|
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). |
|
Ok, but moving stuff out of |
|
Sorry, didn't mean to make a thing of that -- I think the key word for the performance degradation in both cases is "minimally" :) |
faa329f refactor: Release cs_main before MaybeSendFeefilter (MarcoFalke) Pull request description: 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. ACKs for top commit: ajtowns: ACK faa329f ; code review only vasild: ACK faa329f Tree-SHA512: 3e7f9faff1631cc64c86fc1a354ada67617ad1e7a046625cc741f4711854eb41ca8aad5a51ef0d94ff65947b68dba8345c9f786b20ee0a8b7a2e8741cfced21f
Summary: [[bitcoin/bitcoin#21992 | core#21992]] > Remove -feefilter option > > Feefilter option is debug only and it isn't used in any tests, it's wasteful > to check this option for every peer on every iteration of the message handler > loop. refs [[bitcoin/bitcoin#21545 | core#21545]] > Factor feefilter logic out > > Break SendMessages() function into smaller units to improve readability. > Adds lock assert, as `round()` isn't thread safe. > closes #21545 ---- [[bitcoin/bitcoin#24427 | core#24427]] > refactor: Release cs_main before MaybeSendFeefilter ---- This is a backport of [[bitcoin/bitcoin#21992 | core#21992]] and [[bitcoin/bitcoin#24427 | core#24427]] Depends on D12182 Test Plan: With Debug clang and tsan `ninja all check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12183
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.