Skip to content

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

Merged
fanquake merged 1 commit intobitcoin:masterfrom
maflcko:2206-univalue-oom-🛃
Jul 13, 2022

Hidden character warning

The head ref may contain hidden characters: "2206-univalue-oom-\ud83d\udec3"
Merged

rpc: Reduce Univalue push_backV peak memory usage in listtransactions#25464
fanquake merged 1 commit intobitcoin:masterfrom
maflcko:2206-univalue-oom-🛃

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jun 24, 2022

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

shaavan
shaavan approved these changes Jun 27, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK fa8a1c0

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!

@maflcko
Copy link
Member Author

maflcko commented Jun 27, 2022

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.

@maflcko
Copy link
Member Author

maflcko commented Jun 28, 2022

Does anyone know how to test this accurately?

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

Master:
Screenshot from 2022-06-28 11-07-26

This pull:
Screenshot from 2022-06-28 11-07-26

@maflcko
Copy link
Member Author

maflcko commented Jun 29, 2022

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.

@maflcko maflcko mentioned this pull request Jun 29, 2022
Closed
@maflcko maflcko changed the title rpc: Fix Univalue push_backV OOM in listtransactions rpc: Reduce Univalue push_backV peak memory usage in listtransactions Jun 29, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 29, 2022 •
edited
Loading

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.

luke-jr
luke-jr reviewed Jul 5, 2022
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Jul 6, 2022
Merged
@maflcko
Copy link
Member Author

maflcko commented Jul 12, 2022

cc @martinus

martinus
martinus approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@martinus martinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK, looks good to me!

src/wallet/rpc/transactions.cpp
*/
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)
template <class Vec>
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)
Copy link
Contributor

@martinus martinus Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@maflcko maflcko Jul 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but I don't think this affects the peak memory, so can be done in the follow-up #25429

@fanquake fanquake merged commit 081965c into bitcoin:master Jul 13, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 13, 2022
@fanquake
Merge bitcoin#25464: rpc: Reduce Univalue push_backV peak memory usag…
ab03ad1 
…e in listtransactions

fa8a1c0 rpc: Fix Univalue push_backV OOM in listtransactions (MacroFake)

Pull request description:

  Related to, but not intended as a fix for bitcoin#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

ACKs for top commit:
  shaavan:
    Code Review ACK fa8a1c0

Tree-SHA512: 1c3ca40fc8497134a4141195160e4aa9fe72b3c00c5998c972b58ad0eb498ebea05013f9105bb80e7264c9db1d0e7a2032396a8a4af1f326d831fbee20f32ea3
@maflcko maflcko deleted the 2206-univalue-oom-🛃 branch July 14, 2022 08:11
@mjdietzx mjdietzx mentioned this pull request Oct 3, 2022
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@luke-jr luke-jr luke-jr left review comments

@martinus martinus martinus approved these changes

@shaavan shaavan shaavan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@maflcko @DrahtBot @martinus @luke-jr @shaavan @laanwj @fanquake