Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 1 commit into from
Jan 11, 2021
Merged

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

fanquake merged 1 commit into from
Jan 11, 2021

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 29, 2020 •
edited
Loading

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.

@laanwj
Copy link
Member

laanwj commented Apr 30, 2020

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

@maflcko
Copy link
Member Author

maflcko commented Apr 30, 2020

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
Copy link
Member

hebasto commented Apr 30, 2020 •
edited
Loading

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:

bitcoin/src/qt/bitcoin.cpp

Lines 442 to 443 in e5b9308

// IMPORTANT: if CAmount is no longer a typedef use the normal variant above (see https://doc.qt.io/qt-5/qmetatype.html#qRegisterMetaType-1)
qRegisterMetaType<CAmount>("CAmount");

But I'd not exposing CAmount implementation details.

Concept ~0.

EDIT: I've reconsidered this pull.

@maflcko maflcko changed the title net: Remove unused cs_feeFilter net: Replace cs_feeFilter with simple std::atomic May 1, 2020
promag
promag reviewed May 4, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light tested ACK fa321c4afe.

@maflcko
Copy link
Member Author

maflcko commented May 4, 2020

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
Copy link
Member

laanwj commented May 4, 2020

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

@maflcko
Copy link
Member Author

maflcko commented May 4, 2020

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.

@maflcko
Copy link
Member Author

maflcko commented May 4, 2020

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
Copy link
Member

hebasto commented May 5, 2020

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 DrahtBot mentioned this pull request May 22, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented May 27, 2020 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@maflcko maflcko force-pushed the 2004-netNoRecursiveMutex branch from fa321c4 to fa3c2c2 Compare June 8, 2020 14:57
@maflcko
Copy link
Member Author

maflcko commented Jun 9, 2020

Rebased

hebasto
hebasto approved these changes Nov 7, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jnewbery
Copy link
Contributor

jnewbery commented Nov 7, 2020

utACK fa3c2c2

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

@DrahtBot DrahtBot mentioned this pull request Dec 17, 2020
Closed
@DrahtBot DrahtBot mentioned this pull request Jan 6, 2021
Merged
@maflcko maflcko force-pushed the 2004-netNoRecursiveMutex branch from fa3c2c2 to fad1f0f Compare January 7, 2021 14:31
@maflcko
Copy link
Member Author

maflcko commented Jan 7, 2021

Rebased due to trivial conflict in adjacent line

@jnewbery
Copy link
Contributor

jnewbery commented Jan 7, 2021

utACK fad1f0f

@DrahtBot DrahtBot mentioned this pull request Jan 9, 2021
Closed
@practicalswift
Copy link
Contributor

practicalswift commented Jan 10, 2021

cr ACK fad1f0f: patch looks correct

@fanquake fanquake merged commit c4458cc into bitcoin:master Jan 11, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 11, 2021
@fanquake
Merge bitcoin#18819: net: Replace cs_feeFilter with simple std::atomic
2b9456b 
fad1f0f net: Remove unused cs_feeFilter (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  jnewbery:
    utACK fad1f0f
  practicalswift:
    cr ACK fad1f0f: patch looks correct

Tree-SHA512: 647f9b954fbf52e138d3e710937eb9131b390fef0deae03fd6a162d5a18b9f194010800bbddc8f89208d91be2802dff11c3884d04b3dd233865abd12aa3cde06
@maflcko maflcko deleted the 2004-netNoRecursiveMutex branch January 11, 2021 08:03
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @laanwj @hebasto @DrahtBot @jnewbery @practicalswift @promag @fanquake