refactor: Add `performance-no-automatic-move` clang-tidy check by hebasto · Pull Request #26758 · bitcoin/bitcoin

hebasto · 2022-12-27T15:47:53Z

Split from #26642 as requested.

For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html.

The following types are affected:

std::pair<CAddress, NodeSeconds>
std::vector<CAddress>
UniValue, also see refactor: Avoid UniValue copies #25429
QColor
CBlock
MempoolAcceptResult
std::shared_ptr<CWallet>
std::optional<SelectionResult>
CTransactionRef, which is std::shared_ptr<const CTransaction>

https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html

DrahtBot · 2022-12-27T15:47:56Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	aureleoules, andrewtoth

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26642 (clang-tidy: Add more performance-* checks and related fixes by hebasto)
#26039 (refactor: Run type check against RPCArgs (1/2) by MarcoFalke)
#25778 (fuzz: Modify tx_pool_standard target to test package processing by chinggg)
#25093 (rpc: Check for omitted, but required parameters by MarcoFalke)

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.

aureleoules

ACK 9567bfe

maflcko · 2022-12-28T15:56:31Z

src/rpc/util.cpp

        throw std::runtime_error(ToString());
    }
-    const UniValue ret = m_fun(*this, request);
+    UniValue ret = m_fun(*this, request);


I was trying to benchmark this, but couldn't see a difference. Steps to reproduce:

# start bitcoin core with "-rpcuser=user -rpcpassword=password -server=1 -rpcport=12345 -noconnect" # create a reply with python3 -c 'ff="ff"*2'000'000;print(f"{{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", \"method\": \"echo\", \"params\": [\"{ff}\"]}}")' > /tmp/data.json # Run ab ab -p /tmp/data.json -n 100 -c 1 -A user:password "http://localhost:12345/"

Running with -n 10000 -c 4 I get mean time of 50ms on this branch, 51ms on master.

I don't know if a +-1 difference is more than noise+rounding error when the resolution is quantized to 1ms.

Either the copy is so cheap that it doesn't matter in the pipe, or the compiler has already optimized away the const?

Indeed, but there is clearly no regression and this PR is about introducing the new clang-tidy rule. It is not about any particular performance improvement but allowing us to automatically check for bad practices. Should we not bother trying to add new clang-tidy rules if there's no performance benefit?

this PR is about introducing the new clang-tidy rule. It is not about any particular performance improvement but allowing us to automatically check for bad practices.

These are almost the same words I was going to put in my own comment :)

clang, and gcc likely as well, seem to remove the const already on my machine. So any measurement difference is noise. For reference, on 20ff499 I could see a 6% difference. So if we apply performance related checks, it might be good to prioritize the ones that make a difference. Otherwise we'll get people to change i++ to ++i for integers all over the source code.

Sorry, but I do not realize how not having a performance gain for a particular case with a small UniValue instance on a particular platform makes forcing non-pessimized coding style questionable?

It would be nice to be able to say: "This change has a positive effect on at least one end user or dev" instead of "This change doesn't change the binary and causes merge conflicts"

andrewtoth · 2022-12-28T16:47:54Z

ACK 9567bfe
Verified all lines changed give a performance-no-automatic-move error if the const is not removed, and only those lines changed give that error.

…move` clang-tidy check 9567bfe clang-tidy: Add `performance-no-automatic-move` check (Hennadii Stepanov) Pull request description: Split from bitcoin/bitcoin#26642 as [requested](bitcoin/bitcoin#26642 (comment)). For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html. The following types are affected: - `std::pair<CAddress, NodeSeconds>` - `std::vector<CAddress>` - `UniValue`, also see bitcoin/bitcoin#25429 - `QColor` - `CBlock` - `MempoolAcceptResult` - `std::shared_ptr<CWallet>` - `std::optional<SelectionResult>` - `CTransactionRef`, which is `std::shared_ptr<const CTransaction>` ACKs for top commit: andrewtoth: ACK 9567bfe aureleoules: ACK 9567bfe Tree-SHA512: 9b6a5d539205b41d2c86402d384318ed2e1d89e66333ebd200a48fd7df3ce6f6c60a3e989eda5cc503fb34b8d82526f95e56776e1af51e63b49e3a1fef72dbcb

fanquake · 2023-01-11T15:22:13Z

This has been merged.

…ang-tidy check 9567bfe clang-tidy: Add `performance-no-automatic-move` check (Hennadii Stepanov) Pull request description: Split from bitcoin#26642 as [requested](bitcoin#26642 (comment)). For the problem description see https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html. The following types are affected: - `std::pair<CAddress, NodeSeconds>` - `std::vector<CAddress>` - `UniValue`, also see bitcoin#25429 - `QColor` - `CBlock` - `MempoolAcceptResult` - `std::shared_ptr<CWallet>` - `std::optional<SelectionResult>` - `CTransactionRef`, which is `std::shared_ptr<const CTransaction>` ACKs for top commit: andrewtoth: ACK 9567bfe aureleoules: ACK 9567bfe Tree-SHA512: 9b6a5d539205b41d2c86402d384318ed2e1d89e66333ebd200a48fd7df3ce6f6c60a3e989eda5cc503fb34b8d82526f95e56776e1af51e63b49e3a1fef72dbcb

clang-tidy: Add performance-no-automatic-move check

9567bfe

https://clang.llvm.org/extra/clang-tidy/checks/performance/no-automatic-move.html

hebasto mentioned this pull request Dec 27, 2022

clang-tidy: Add more performance-* checks and related fixes #26642

Merged

This was referenced Dec 28, 2022

refactor: Run type check against RPCArgs (1/2) #26039

Merged

fuzz: Modify tx_pool_standard target to test package processing #25778

Closed

refactor: Avoid UniValue copies #25429

Closed

doc: Fix incorrect sendmany RPC doc #25093

Merged

aureleoules approved these changes Dec 28, 2022

View reviewed changes

maflcko reviewed Dec 28, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 7, 2023

[Draft / POC] Silent Payments #24897

Closed

maflcko changed the title ~~clang-tidy: Add performance-no-automatic-move check~~ refactor: Add performance-no-automatic-move clang-tidy check Jan 11, 2023

DrahtBot added the Refactoring label Jan 11, 2023

fanquake closed this Jan 11, 2023

hebasto deleted the 221227-tidy-nam branch January 12, 2023 15:01

ryanofsky mentioned this pull request Jan 20, 2023

refactor: Use util::Result class for wallet loading #25722

Draft

bitcoin locked and limited conversation to collaborators Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Add `performance-no-automatic-move` clang-tidy check#26758

refactor: Add `performance-no-automatic-move` clang-tidy check#26758
hebasto wants to merge 1 commit intobitcoin:masterfrom
hebasto:221227-tidy-nam

hebasto commented Dec 27, 2022

Uh oh!

DrahtBot commented Dec 27, 2022 •

edited

Loading

Uh oh!

aureleoules left a comment

Uh oh!

maflcko Dec 28, 2022

Uh oh!

andrewtoth Dec 28, 2022

Uh oh!

maflcko Dec 30, 2022

Uh oh!

andrewtoth Dec 30, 2022

Uh oh!

hebasto Dec 31, 2022 •

edited

Loading

Uh oh!

maflcko Dec 31, 2022

Uh oh!

hebasto Dec 31, 2022 •

edited

Loading

Uh oh!

maflcko Jan 11, 2023

Uh oh!

andrewtoth commented Dec 28, 2022

Uh oh!

fanquake commented Jan 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

hebasto commented Dec 27, 2022

Uh oh!

DrahtBot commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

andrewtoth Dec 28, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

andrewtoth Dec 30, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 31, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Dec 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

andrewtoth commented Dec 28, 2022

Uh oh!

fanquake commented Jan 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Dec 27, 2022 •

edited

Loading

hebasto Dec 31, 2022 •

edited

Loading

hebasto Dec 31, 2022 •

edited

Loading