refactor: Avoid UniValue copies #25429

maflcko · 2022-06-20T15:17:09Z

UniValue copies have been a source of performance related bugs and regressions, so it would be good to remove the copy constructors.

This is the first change toward the goal. This change uses a const UniValue& where possible.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack, hebasto
Stale ACK	martinus, fanquake

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:

#26988 ([rpc]: Add test-only RPC addrmaninfo for new/tried table address count by stratospher)
#26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
#26780 (rpc: simplify scan blocks by furszy)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26612 (refactor: RPC: pass named argument value as string_view by stickies-v)
#26426 (WIP: Fix coinstatsindex overflow issue by fjahr)
#25974 (test, build: Separate read_json function into its own module by hebasto)
#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
#25344 (New outputs argument for bumpfee/psbtbumpfee by rodentrabies)
#15294 (refactor: Extract RipeMd160 by Empact)

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.

martinus · 2022-06-21T17:34:21Z

Nice! concept ACK. How about removing the copy constructor and copy assignment from the UniValue class, as done here? Fixing all copies would make the PR much bigger though.

maflcko · 2022-06-22T05:56:23Z

I am actually not sure how to fix all copies. Sometimes the std lib will copy if asked to:

std::vector<UniValue> a;
std::vector<UniValue> b;
a.insert(a.end(), b.begin(), b.end());

laanwj · 2022-06-22T10:14:52Z

I guess you could add an explicit 'clone' method (like rust) and use that when it's really necessary to make a copy. It will probably make some things much more verbose though.

martinus · 2022-06-22T15:55:02Z

a.insert(a.end(), b.begin(), b.end());

In that case you can use

a.insert(a.end(), std::make_move_iterator(b.begin()), std::make_move_iterator(b.end()));

But I there might still be case where copy constructor is necessary, I remember that I had difficulties in my old PR as well

fanquake · 2022-06-23T12:11:14Z

Concept ACK

fanquake

ACK bbbbcfd - as a first step.

maflcko · 2022-06-29T09:47:55Z

While the changes are correct, I'll try to make them easier to review and more complete. For now, please review actual UniValue-copy bugfixes, like #25464

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

Concept ACK.

…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

…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

std::pair<A, B> is different from std::map<A, B>::value_type (aka std::pair<const A, B>). So fix the type to also avoid a copy.

DrahtBot · 2023-01-30T10:29:54Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the Refactoring label Jun 20, 2022

fanquake approved these changes Jun 28, 2022

View reviewed changes

maflcko marked this pull request as draft June 29, 2022 09:48

maflcko mentioned this pull request Jul 12, 2022

rpc: Reduce Univalue push_backV peak memory usage in listtransactions #25464

Merged

DrahtBot mentioned this pull request Jul 15, 2022

refactor: univalue test cleanups #25617

Merged

This was referenced Jul 26, 2022

refactor: Make const references to avoid unnecessarily copying objects and enable two clang-tidy checks #25707

Merged

rpc: treat univalue type check error as RPC_TYPE_ERROR, not RPC_MISC_ERROR #25737

Merged

DrahtBot added the Needs rebase label Aug 19, 2022

maflcko force-pushed the 2206-uni-copy- branch 3 times, most recently from bbbbcfd to 1b652ff Compare September 1, 2022 16:22

DrahtBot removed the Needs rebase label Sep 1, 2022

DrahtBot mentioned this pull request Sep 8, 2022

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

Merged

DrahtBot added the Needs rebase label Sep 21, 2022

maflcko force-pushed the 2206-uni-copy- branch from 1b652ff to 516ab6c Compare December 21, 2022 15:10

DrahtBot removed the Needs rebase label Dec 21, 2022

maflcko mentioned this pull request Dec 21, 2022

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

Merged

This was referenced Dec 22, 2022

rpc: remove optional from fStateStats fields #26727

Merged

validate package transactions with their in-package ancestor sets #26711

Closed

validation, bugfix: provide more info in *MempoolAcceptResult #26646

Merged

This was referenced Dec 22, 2022

New outputs argument for bumpfee/psbtbumpfee #25344

Merged

doc: Fix incorrect sendmany RPC doc #25093

Merged

refactor: Extract RipeMd160 #15294

Merged

hebasto mentioned this pull request Dec 27, 2022

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

Closed

This was referenced Jan 1, 2023

rpc: simplify scan blocks #26780

Merged

refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc #26840

Closed

DrahtBot mentioned this pull request Jan 11, 2023

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

DrahtBot added the Needs rebase label Jan 11, 2023

maflcko force-pushed the 2206-uni-copy- branch from 516ab6c to 56358e4 Compare January 11, 2023 16:59

DrahtBot removed the Needs rebase label Jan 11, 2023

DrahtBot mentioned this pull request Jan 13, 2023

RPC: make RPCResult::MatchesType return useful errors #26887

Merged

DrahtBot added the Needs rebase label Jan 17, 2023

maflcko force-pushed the 2206-uni-copy- branch from 56358e4 to 625889a Compare January 19, 2023 17:24

DrahtBot removed the Needs rebase label Jan 19, 2023

DrahtBot mentioned this pull request Jan 19, 2023

refactor: RPC: pass named argument value as string_view #26612

Merged

DrahtBot added the Needs rebase label Jan 20, 2023

MacroFake added 3 commits January 21, 2023 11:20

refactor: Avoid UniValue copies

9530e4f

test: Use correct type to avoid copy

7568af2

std::pair<A, B> is different from std::map<A, B>::value_type (aka std::pair<const A, B>). So fix the type to also avoid a copy.

WIP

fc7ce2b

maflcko force-pushed the 2206-uni-copy- branch from 625889a to fc7ce2b Compare January 21, 2023 10:21

DrahtBot removed the Needs rebase label Jan 21, 2023

DrahtBot mentioned this pull request Jan 30, 2023

cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency #26988

Open

DrahtBot added the Needs rebase label Jan 30, 2023

maflcko closed this Feb 23, 2023

maflcko deleted the 2206-uni-copy-👾 branch February 23, 2023 17:52

bitcoin locked and limited conversation to collaborators Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Avoid UniValue copies #25429

refactor: Avoid UniValue copies #25429

Uh oh!

maflcko commented Jun 20, 2022

Uh oh!

maflcko commented Jun 20, 2022

Uh oh!

theStack commented Jun 20, 2022

Uh oh!

fanquake commented Jun 20, 2022

Uh oh!

DrahtBot commented Jun 20, 2022 •

edited

Loading

Uh oh!

martinus commented Jun 21, 2022 •

edited

Loading

Uh oh!

maflcko commented Jun 22, 2022

Uh oh!

laanwj commented Jun 22, 2022

Uh oh!

martinus commented Jun 22, 2022

Uh oh!

fanquake commented Jun 23, 2022

Uh oh!

fanquake left a comment

Uh oh!

maflcko commented Jun 29, 2022

Uh oh!

hebasto commented Dec 27, 2022

Uh oh!

DrahtBot commented Jan 30, 2023

Uh oh!

Uh oh!

refactor: Avoid UniValue copies #25429

refactor: Avoid UniValue copies #25429

Uh oh!

Conversation

maflcko commented Jun 20, 2022

Uh oh!

maflcko commented Jun 20, 2022

Uh oh!

theStack commented Jun 20, 2022

Uh oh!

fanquake commented Jun 20, 2022

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

martinus commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 22, 2022

Uh oh!

laanwj commented Jun 22, 2022

Uh oh!

martinus commented Jun 22, 2022

Uh oh!

fanquake commented Jun 23, 2022

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 29, 2022

Uh oh!

hebasto commented Dec 27, 2022

Uh oh!

DrahtBot commented Jan 30, 2023

Uh oh!

Uh oh!

DrahtBot commented Jun 20, 2022 •

edited

Loading

martinus commented Jun 21, 2022 •

edited

Loading