refactor: Avoid UniValue copies #25429

1. 
   maflcko commented at 3:17 pm on June 20, 2022: member

   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:

   • https://github.com/bitcoin-core/univalue-subtree/pull/27
   • https://github.com/bitcoin/bitcoin/pull/15974
2. 
   maflcko commented at 3:21 pm on June 20, 2022: member

   Review note: const will compile, but kill performance when std::move or RVO is intended.
3. DrahtBot added the label Refactoring on Jun 20, 2022
4. 
   theStack commented at 7:51 pm on June 20, 2022: contributor

   Concept ACK
5. 
   fanquake commented at 9:28 pm on June 20, 2022: member

   cc @martinus
6. 
   DrahtBot commented at 11:56 pm on June 20, 2022: contributor

   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
   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.

7. 
   martinus commented at 5:34 pm on June 21, 2022: contributor

   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.
8. 
   maflcko commented at 5:56 am on June 22, 2022: member

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

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

9. 
   laanwj commented at 10:14 am on June 22, 2022: member

   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.
10. 
    martinus commented at 3:55 pm on June 22, 2022: contributor

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

    In that case you can use

    0a.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

11. 
    fanquake commented at 12:11 pm on June 23, 2022: member

    Concept ACK
12. fanquake approved
13. 
    fanquake commented at 5:30 pm on June 28, 2022: member

    ACK bbbbcfdb8e87f3151229ebc8c7b812a420d89b14 - as a first step.
14. 
    maflcko commented at 9:47 am on June 29, 2022: member

    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 https://github.com/bitcoin/bitcoin/pull/25464
15. maflcko marked this as a draft on Jun 29, 2022
16. DrahtBot added the label Needs rebase on Aug 19, 2022
17. maflcko force-pushed on Sep 1, 2022
18. maflcko force-pushed on Sep 1, 2022
19. maflcko force-pushed on Sep 1, 2022
20. DrahtBot removed the label Needs rebase on Sep 1, 2022
21. DrahtBot added the label Needs rebase on Sep 21, 2022
22. maflcko force-pushed on Dec 21, 2022
23. DrahtBot removed the label Needs rebase on Dec 21, 2022
24. 
    hebasto commented at 3:53 pm on December 27, 2022: member

    Concept ACK.
25. DrahtBot added the label Needs rebase on Jan 11, 2023
26. maflcko referenced this in commit 9887fc7898 on Jan 11, 2023
27. sidhujag referenced this in commit 3b0597af60 on Jan 11, 2023
28. maflcko force-pushed on Jan 11, 2023
29. DrahtBot removed the label Needs rebase on Jan 11, 2023
30. DrahtBot added the label Needs rebase on Jan 17, 2023
31. maflcko force-pushed on Jan 19, 2023
32. DrahtBot removed the label Needs rebase on Jan 19, 2023
33. DrahtBot added the label Needs rebase on Jan 20, 2023
34. refactor: Avoid UniValue copies 9530e4f515
35. test: Use correct type to avoid copy

    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.

    7568af2c41
36. WIP fc7ce2b464
37. maflcko force-pushed on Jan 21, 2023
38. DrahtBot removed the label Needs rebase on Jan 21, 2023
39. DrahtBot added the label Needs rebase on Jan 30, 2023
40. 
    DrahtBot commented at 10:29 am on January 30, 2023: contributor

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

41. maflcko closed this on Feb 23, 2023

    ---

42. maflcko deleted the branch on Feb 23, 2023
43. bitcoin locked this on Feb 23, 2024

Contributors
maflcko theStack fanquake DrahtBot martinus laanwj hebasto

Labels
Refactoring Needs rebase