-
Notifications
You must be signed in to change notification settings - Fork 36k
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.
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
refactor: Avoid UniValue copies #25429
The head ref may contain hidden characters: "2206-uni-copy-\u{1F47E}"
Conversation
Review note: |
Concept ACK |
cc @martinus |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
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. |
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()); |
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. |
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 |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK bbbbcfd - as a first step.
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 |
bbbbcfd
to
1b652ff
Compare
1b652ff
to
516ab6c
Compare
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
516ab6c
to
56358e4
Compare
56358e4
to
625889a
Compare
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.
625889a
to
fc7ce2b
Compare
🐙 This pull request conflicts with the target branch and needs rebase. |
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.See also: