net: Replace cs_feeFilter with simple std::atomic #18819

MarcoFalke commented at 7:16 PM on April 29, 2020: member

A RecursiveMutex is overkill for setting or reading a plain integer. Even a Mutex is overkill, when a plain std::atomic can be used.

This removes 11 lines of code. Also, it is cutting down on the number of locks put on the stack at the same time, which complicates review looking out for potential lock contention.

MarcoFalke added the label Refactoring on Apr 29, 2020

MarcoFalke added the label P2P on Apr 29, 2020

laanwj commented at 1:48 PM on April 30, 2020: member

Not sure if we care about this case, but this adds a strict assumption that CAmount is a typedef of an primitive integer type.

MarcoFalke commented at 2:05 PM on April 30, 2020: member

Yeah, I thought about making a simple setter/getter to not leak the std::atomic implementation detail from net to net_processing, but that seemed over-engineering that can be done when it is needed.

hebasto commented at 8:56 PM on April 30, 2020: member

Not sure if we care about this case, but this adds a strict assumption that CAmount is a typedef of an primitive integer type.

This assumption is already used: https://github.com/bitcoin/bitcoin/blob/e5b9308920a151946b83694fe1701d90316a2a9e/src/qt/bitcoin.cpp#L442-L443

~But I'd not exposing CAmount implementation details.~

~Concept ~0.~

EDIT: I've reconsidered this pull.

MarcoFalke renamed this:
~~net: Remove unused cs_feeFilter~~
net: Replace cs_feeFilter with simple std::atomic
on May 1, 2020

in src/net_processing.cpp:4390 in fa321c4afe outdated

3853 | @@ -3855,11 +3854,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3854 |                  if (fSendTrickle && pto->m_tx_relay->fSendMempool) {
3855 |                      auto vtxinfo = m_mempool.infoAll();
3856 |                      pto->m_tx_relay->fSendMempool = false;
3857 | -                    CFeeRate filterrate;
3858 | -                    {
3859 | -                        LOCK(pto->m_tx_relay->cs_feeFilter);
3860 | -                        filterrate = CFeeRate(pto->m_tx_relay->minFeeFilter);
3861 | -                    }
3862 | +                    const CFeeRate filterrate{pto->m_tx_relay->minFeeFilter.load()};

promag commented at 10:24 AM on May 4, 2020:

Alternative is const CFeeRate filterrate{CAmount(pto->m_tx_relay->minFeeFilter)} but I prefer being explicit with atomic load.

hebasto commented at 7:44 AM on May 14, 2020:

For consistency, could be used both load() and store(), or none of them?

MarcoFalke commented at 2:33 PM on January 7, 2021:

Going to leave as is

promag commented at 10:25 AM on May 4, 2020: member

Light tested ACK fa321c4afe.

MarcoFalke commented at 11:12 AM on May 4, 2020: member

But I'd not exposing CAmount implementation details. @hebasto I can't parse this sentence. Mind to explain? We already assume that the type is CAmount everywhere. The only thing this pull changes is removing a bunch of boilerplate code used for overkill locking behavior.

laanwj commented at 1:31 PM on May 4, 2020: member

Unless a performance improvement can be demonstrated I'm not sure this change is really worth it.

MarcoFalke commented at 1:35 PM on May 4, 2020: member

It is mostly about the removed code (11 lines), and cutting down on the number of locks put on the stack at the same time, which complicates review looking out for potential lock contention.

MarcoFalke commented at 1:38 PM on May 4, 2020: member

Performance wise this should have no effect. Reading from the mempool is orders of magnitude slower than taking a lock on a recursive mutex (even with lock contention debugging enabled).

hebasto commented at 10:29 AM on May 5, 2020: member

But I'd not exposing CAmount implementation details.

@hebasto I can't parse this sentence. Mind to explain? We already assume that the type is CAmount everywhere. The only thing this pull changes is removing a bunch of boilerplate code used for overkill locking behavior. @MarcoFalke Apologies for my confusing comment. I forgot that std::atomic template may be instantiated with any suitable type, not only with built-in ones (typedef of what CAmount currently is).

DrahtBot cross-referenced this on May 22, 2020 from issue refactor: replace CNode pointers by references within net_processing.{h,cpp} by theStack

DrahtBot commented at 9:40 PM on May 27, 2020: contributor

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Jun 4, 2020

MarcoFalke force-pushed on Jun 8, 2020

DrahtBot removed the label Needs rebase on Jun 8, 2020

MarcoFalke commented at 12:51 AM on June 9, 2020: member

Rebased

in src/net.cpp:604 in fa3c2c25ed outdated

 550 | @@ -551,7 +551,6 @@ void CNode::copyStats(CNodeStats &stats, const std::vector<bool> &m_asmap)
 551 |      X(m_legacyWhitelisted);
 552 |      X(m_permissionFlags);
 553 |      if (m_tx_relay != nullptr) {
 554 | -        LOCK(m_tx_relay->cs_feeFilter);
 555 |          stats.minFeeFilter = m_tx_relay->minFeeFilter;

hebasto commented at 8:53 AM on November 7, 2020:

nit: Make atomic operations explicit:

        stats.minFeeFilter = m_tx_relay->minFeeFilter.load();

MarcoFalke commented at 2:33 PM on January 7, 2021:

Going to leave as is to minimize the diff

in src/net_processing.cpp:3715 in fa3c2c25ed outdated

3505 | @@ -3506,7 +3506,6 @@ bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
3506 |          vRecv >> newFeeFilter;
3507 |          if (MoneyRange(newFeeFilter)) {
3508 |              if (pfrom.m_tx_relay != nullptr) {
3509 | -                LOCK(pfrom.m_tx_relay->cs_feeFilter);
3510 |                  pfrom.m_tx_relay->minFeeFilter = newFeeFilter;

hebasto commented at 8:54 AM on November 7, 2020:

nit: Make atomic operations explicit:

                pfrom.m_tx_relay->minFeeFilter.store(newFeeFilter);

MarcoFalke commented at 2:32 PM on January 7, 2021:

Going to leave as is to minimize the diff

hebasto approved

hebasto commented at 8:55 AM on November 7, 2020: member

ACK fa3c2c25ed2f8e8c343d4840065e3404a5e6f761, I have reviewed the code and it looks OK, I agree it can be merged.

jnewbery commented at 11:32 AM on November 7, 2020: member

utACK fa3c2c25e

All this stuff should move to Peer in net_processing, but there's no harm simplifying it where it is now.

DrahtBot cross-referenced this on Dec 17, 2020 from issue Replace m_tx_relay/nullptr checks in net_processing.cpp by sdaftuar

DrahtBot cross-referenced this on Jan 6, 2021 from issue net: Move SocketSendData lock annotation to header by MarcoFalke

net: Remove unused cs_feeFilter fad1f0fd33

MarcoFalke force-pushed on Jan 7, 2021

MarcoFalke commented at 2:31 PM on January 7, 2021: member

Rebased due to trivial conflict in adjacent line

jnewbery commented at 3:00 PM on January 7, 2021: member

utACK fad1f0fd33e5e7a65b702237c7ca8e1b694852d2

DrahtBot cross-referenced this on Jan 9, 2021 from issue net-processing refactoring -- lose globals, move implementation details from .h to .cpp by ajtowns

practicalswift commented at 6:57 PM on January 10, 2021: contributor

cr ACK fad1f0fd33e5e7a65b702237c7ca8e1b694852d2: patch looks correct

fanquake merged this on Jan 11, 2021

fanquake closed this on Jan 11, 2021

sidhujag referenced this in commit 2b9456b75c on Jan 11, 2021

MarcoFalke deleted the branch on Jan 11, 2021

bitcoin locked this on Aug 16, 2022