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

1. 
   MarcoFalke commented at 7:07 am on June 24, 2022: member

   Related to, but not intended as a fix for #25229.

   Currently the RPC will have the same data stored thrice:

   • UniValue ret (memory filled by ListTransactions)
   • std::vector<UniValue> vec (constructed by calling push_backV)
   • UniValue result (the actual result, memory filled by push_backV)

   Fix this by filling the memory only once:

   • std::vector<UniValue> ret (memory filled by ListTransactions)
   • Pass iterators to push_backV instead of creating a full copy
   • Move memory into UniValue result instead of copying it
2. rpc: Fix Univalue push_backV OOM in listtransactions fa8a1c0696
3. DrahtBot added the label Refactoring on Jun 24, 2022
4. laanwj added the label RPC/REST/ZMQ on Jun 27, 2022
5. laanwj removed the label Refactoring on Jun 27, 2022
6. shaavan approved
7. 
   shaavan commented at 12:16 pm on June 27, 2022: contributor

   Code Review ACK fa8a1c06961f4b1826696e0db8dce81dce627721

   I agree with the changes done in this PR. To explain my understanding and the reason for my approval of this PR, I would reiterate the changes done in it as I understood them.

   Reasoning:

   I was able to confirm that there were three locations where the same data was being stored:

   1. In the UniValue ret variable
   2. In the txs vector created to store ret.getValues() Memory consumed in creating a temporary vector to input into push_backV function
   3. In the values of UniValue result.

   This PR changes the storing of the values only once by:

   1. Creating a new function, push_backV, which, instead of taking the whole vector as an argument, only takes the first and last iterators, saving up on creating a memory-consuming temporary variable.
   2. Instead of copying the values, moves them by directly using their locations in the memory to store the values in the result variable.

   This change allows using 1/3rd of the memory to achieve the same functionality. And removing the need to create variables and copying the values would also provide a minor speed boost.

   Edit: Thanks, @MarcoFalke, for helping clear up the misunderstood points!

8. 
   MarcoFalke commented at 12:40 pm on June 27, 2022: member

   In the txs vector created to store ret.getValues()

   This is not accurate. The memory used by const std::vector<UniValue>& txs = ret.getValues(); is roughly 0, as getValues returns a reference and the return value is bound as a reference.

   Memory is consumed when creating an unnamed temporary vector, to be passed into push_backV. While the argument of push_backV is a const reference, there is no enforcement that the caller is providing a reference and the language allows any value of the type (lvalue or (p)rvalue) to be passed in.

9. 
   MarcoFalke commented at 9:10 am on June 28, 2022: member

   Does anyone know how to test this accurately?

   I tried massif, but it didn’t seem too useful to check intermittent memory spikes.

   Master:

   This pull:

10. 
    MarcoFalke commented at 9:44 am on June 29, 2022: member

    While the massif chart may not be accurate, the maximum memory usage should be.

    I’ve also confirmed this with /usr/bin/time --format='%M'.

    Results for master:

    • peak memory (%M): 0.507 GB
    • fastest duration (out of 10 RPC invocations): 2.34 seconds

    This pull:

    • peak memory: 0.444 GB
    • fastest duration (out of 10 RPC invocations): 2.19 seconds

    Thus, in my testing, the changes in this pull both speed-up and reduce memory.

11. MarcoFalke renamed this:
    rpc: Fix Univalue push_backV OOM in listtransactions
    rpc: Reduce Univalue push_backV peak memory usage in listtransactions
    on Jun 29, 2022
12. 
    DrahtBot commented at 12:17 pm on June 29, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25551 (refactor: Throw exception on invalid Univalue pushes over silent ignore 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.

13. in src/univalue/include/univalue.h:145 in fa8a1c0696

    138@@ -137,6 +139,14 @@ class UniValue {
    139     friend const UniValue& find_value( const UniValue& obj, const std::string& name);
    140 };
    141 
    142+template <class It>
    143+bool UniValue::push_backV(It first, It last)
    144+{
    145+    if (typ != VARR) return false;
    

    ---

    ---

    

    luke-jr commented at 10:38 pm on July 5, 2022:

    Probably should throw here instead of return a value that is likely to be unchecked (as below)?

    ---

    

    MarcoFalke commented at 10:02 am on July 6, 2022:

    Good idea. Fixed in #25551 for the whole codebase.
14. 
    MarcoFalke commented at 12:04 pm on July 12, 2022: member

    cc @martinus
15. in src/wallet/rpc/transactions.cpp:337 in fa8a1c0696

    334  * @param  filter_ismine  The "is mine" filter flags.
    335  * @param  filter_label   Optional label string to filter incoming transactions.
    336  */
    337-static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
    338+template <class Vec>
    339+static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong, Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
    

    ---

    ---

    

    martinus commented at 12:13 pm on July 12, 2022:

    In line 368 and 409 you could use ret.push_back(std::move(entry)); to safe another copy

    ---

    

    MarcoFalke commented at 2:36 pm on July 12, 2022:

    Good point, but I don’t think this affects the peak memory, so can be done in the follow-up https://github.com/bitcoin/bitcoin/pull/25429
16. martinus approved
17. 
    martinus commented at 12:17 pm on July 12, 2022: contributor

    Code review ACK, looks good to me!
18. fanquake merged this on Jul 13, 2022
19. fanquake closed this on Jul 13, 2022

    ---

20. sidhujag referenced this in commit ab03ad1891 on Jul 13, 2022
21. MarcoFalke deleted the branch on Jul 14, 2022
22. bitcoin locked this on Jul 14, 2023

Contributors
MarcoFalke shaavan DrahtBot luke-jr martinus

Labels
RPC/REST/ZMQ