Yet another change to reduce recursive mempool locking #19917
Conversation
|
@hebasto this is an example of stuff that I think we can do before moving locks around. The first and second commits refactor |
|
@vasild @ryanofsky your comment here would be nice too. |
|
Why draft? |
|
Yeah I don't mind setting it ready for review, the goal was to show an example rather than adding noise to your PR. |
I'll be happy to postpone #19872 until this PR is reviewed and merged :) |
src/net_processing.cpp
Outdated
| { | ||
| LOCK(cs_main); | ||
| for (const auto& elem : relay_transactions) { | ||
| RelayTransaction(elem.first, elem.second, m_connman); | ||
| } | ||
| } |
There was a problem hiding this comment.
8f30df2
Does this approach decrease concurrency wrt to ::cs_main uninterruptible locking?
There was a problem hiding this comment.
RelayTransaction doesn't hold cs_main that long.
|
Concept ACK. |
|
The changes in What is the purpose of this patch? There is no description and commit messages are a bit scarce, missing answer to "Why are we doing this?". |
|
@vasild sure I can detail the intention. See #19917 (comment). |
8f4307c to
dde441c
Compare
| @@ -421,7 +421,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) | |||
| for (const CTxIn& txin : it->GetTx().vin) | |||
| mapNextTx.erase(txin.prevout); | |||
|
|
|||
| RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ ); | |||
| WITH_LOCK(cs, RemoveUnbroadcastTx(hash, true /* add logging because unchecked */ )); | |||
| void PeerManager::ReattemptInitialBroadcast(CScheduler& scheduler) const | ||
| { | ||
| std::map<uint256, uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs(); | ||
|
|
||
| for (const auto& elem : unbroadcast_txids) { | ||
| // Sanity check: all unbroadcast txns should exist in the mempool | ||
| if (m_mempool.exists(elem.first)) { | ||
| LOCK(cs_main); | ||
| std::vector<std::pair<uint256, uint256>> relay_transactions; | ||
| { | ||
| LOCK(m_mempool.cs); | ||
| std::map<uint256, uint256> unbroadcast_txids = m_mempool.GetUnbroadcastTxs(); | ||
| relay_transactions.reserve(unbroadcast_txids.size()); | ||
| for (const auto& elem : unbroadcast_txids) { | ||
| // Sanity check: all unbroadcast txns should exist in the mempool | ||
| if (m_mempool.exists(elem.first)) { | ||
| relay_transactions.push_back(elem); | ||
| } else { | ||
| m_mempool.RemoveUnbroadcastTx(elem.first, true); | ||
| } | ||
| } | ||
| } | ||
| { | ||
| LOCK(cs_main); | ||
| for (const auto& elem : relay_transactions) { | ||
| RelayTransaction(elem.first, elem.second, m_connman); | ||
| } else { | ||
| m_mempool.RemoveUnbroadcastTx(elem.first, true); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This can be simplified to:
void PeerManager::ReattemptInitialBroadcast(CScheduler& scheduler) const
{
for (const auto& elem : WITH_LOCK(m_mempool.cs, return m_mempool.GetUnbroadcastTxs())) {
LOCK(cs_main);
RelayTransaction(elem.first, elem.second, m_connman);
}
// Schedule next run for 10-15 minutes in the future.
...
}Because m_mempool.exists() will always return true for a tx returned by m_mempool.GetUnbroadcastTxs() if we don't release m_mempool.cs between the two calls.
Also, the tx could be removed after we release m_mempool.cs and before we call RelayTransaction() and this is ok and is handled just fine.
cc @gzhao408
There was a problem hiding this comment.
But is it really necessary to have cs_main lock in each iteration?
There was a problem hiding this comment.
Since it was locked in each iteration before this PR, the question should rather be "If we move LOCK(cs_main) before the loop, why would we do that?"
The frequent lock/unlock allows other threads to proceed. I don't see a reason to change it.
There was a problem hiding this comment.
The frequent lock/unlock allows other threads to proceed. I don't see a reason to change it.
Agree (#19917 (comment)).
There was a problem hiding this comment.
@vasild I don't agree with that. Other threads can proceed but the current thread will wait unnecessarily in each iteration for the lock and as such other things will be delayed, not mentioning the mutex overhead. See https://stackoverflow.com/a/3652428.
In this case RelayTransaction is pretty quick, nothing that can potentially cause a big lock on cs_main.
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Rebase hell. |
First 2 commits avoid unlock/lock
mempool.csandcs_maininterchangeably by turning a loop in two loops - each mutex is locked throughout each corresponding loop.Then explicit lock in
CTxMemPool::RemoveUnbroadcastTx,CTxMemPool::GetUnbroadcastTxsandCTxMemPool::existsis removed forcing just 3 explicitWITH_LOCKwhereexists()is called and just 1 whereGetUnbroadcastTxs()is called. This can be improved by adding an auxiliary function that locks and calls the original.