performance-no-automatic-move clang-tidy check
#26758
Split from bitcoin/bitcoin#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 bitcoin/bitcoin#25429QColorCBlockMempoolAcceptResultstd::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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Reviewers, this pull request conflicts with the following ones:
performance-* checks and related fixes by hebasto)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.
578@@ -579,7 +579,7 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
579 if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
580 throw std::runtime_error(ToString());
581 }
582- const UniValue ret = m_fun(*this, request);
583+ UniValue ret = m_fun(*this, request);
I was trying to benchmark this, but couldn’t see a difference. Steps to reproduce:
0# start bitcoin core with "-rpcuser=user -rpcpassword=password -server=1 -rpcport=12345 -noconnect"
1# create a reply with
2python3 -c 'ff="ff"*2'000'000;print(f"{{\"jsonrpc\": \"1.0\", \"id\": \"curltest\", \"method\": \"echo\", \"params\": [\"{ff}\"]}}")' > /tmp/data.json
3# Run ab
4ab -p /tmp/data.json -n 100 -c 1 -A user:password "http://localhost:12345/"
-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?
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 :)
const already on my machine. So any measurement difference is noise. For reference, on 20ff4991e548630d7bb5e491fa4d69ec49369872 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.
UniValue instance on a particular platform makes forcing non-pessimized coding style questionable?
performance-no-automatic-move error if the const is not removed, and only those lines changed give that error.
