rpc: move UniValue in blockToJSON #30094

willcl-ark · 2024-05-13T13:38:03Z

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects.

Used by rest_block and getblock RPC.

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC.

DrahtBot · 2024-05-13T13:38:05Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, ismaelsadeeq, TheCharlatan, theuni, hebasto, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

willcl-ark · 2024-05-13T13:39:23Z

Before and after of RSS:

maflcko · 2024-05-13T13:39:34Z

review ACK b77bad3

maflcko · 2024-05-13T13:48:32Z

Also "fixes" #30052

willcl-ark · 2024-05-13T13:50:34Z

Also "fixes" #30052

Ah yes, the actual reason I started investigating this in the first place! Thanks.

I was slightly unsure whether I should claim that this is fully fixing #24525, but it will fix #30052

maflcko · 2024-05-13T13:51:03Z

Unrelated to the blockToJSON changes here, as a follow-up, it may be possible to "fix" #25229 in the same way?

ismaelsadeeq

ACK b77bad3

TheCharlatan

ACK b77bad3

theuni

utACK b77bad3

theuni · 2024-05-13T17:12:45Z

Actually, looks like this could use a txs.reserve(block.vtx.size()) as well.

hebasto

ACK b77bad3, I have reviewed the code and it looks OK.

I would be great to check all UniValue::push_back and UniValue::pushKV invocations in the codebase.

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC. Github-Pull: bitcoin#30094 Rebased-From: b77bad3

BrandonOdiwuor

ACK b77bad3

willcl-ark · 2024-05-13T18:44:13Z

Unrelated to the blockToJSON changes here, as a follow-up, it may be possible to "fix" #25229 in the same way?

I can push a similar commit here if it's worth merging the two. Or open a separate PR? Perhaps with 3 ACKs a separate PR is preferable? I also am only halfway through making a "big wallet" (using achow101's make-big-wallet script, so I have no suitable wallet to test such a commit against currently.

Actually, looks like this could use a txs.reserve(block.vtx.size()) as well.

I don't see reserve on UniValue. Perhaps I a missing what you mean here?

I would be great to check all UniValue::push_back and UniValue::pushKV invocations in the codebase.

I can check the remainder of the codebase for other occurrences too.

theuni · 2024-05-13T18:46:16Z

Actually, looks like this could use a txs.reserve(block.vtx.size()) as well.

I don't see reserve on UniValue. Perhaps I a missing what you mean here?

Whoops, of course you're right. It probably should though :)

theuni · 2024-05-13T20:33:57Z

I would be great to check all UniValue::push_back and UniValue::pushKV invocations in the codebase.

I can check the remainder of the codebase for other occurrences too.

I was already working on this and... wow. You've opened a big can of worms here @willcl-ark :). But that's a good thing because it should reduce memusage and increase performance I'd assume.

See here for my first pass. It's a beast. https://github.com/theuni/bitcoin/commits/less-univalue-copies/

The first commit mostly just does the naive thing of adding std::move when possible. In a few cases I shuffled code around to avoid a move-after-use. I moved pretty quickly though and might have introduced some bugs. A pass with clang-tidy would be useful.

The second commit adds // TODO for any copy being made. Most of these are probably legit, but I assume some could be avoided by changing surrounding code/params.

These are changes required for bitcoind only. I haven't done tests/fuzz/bench/etc.

To flesh them out, I changed the function signatures of the corresponding UniValue functions to accept rvalues only.

Edit: added 2 more commits to avoid copying strings. @willcl-ark let me know if you've already been working on this some and how you'd like to collaborate on it. There's quite a bit of work to do here :)

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC. Github-Pull: bitcoin#30094 Rebased-From: b77bad3

fanquake · 2024-05-14T00:16:28Z

Backported to 27.x in #30092.

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC. Github-Pull: bitcoin#30094 Rebased-From: b77bad3

willcl-ark · 2024-05-14T10:41:46Z

I was already working on this and... wow. You've opened a big can of worms here @willcl-ark :). But that's a good thing because it should reduce memusage and increase performance I'd assume.

See here for my first pass. It's a beast. theuni/bitcoin@less-univalue-copies (commits)

Wow! This looks like a great start! Will begin to review...

Edit: added 2 more commits to avoid copying strings. @willcl-ark let me know if you've already been working on this some and how you'd like to collaborate on it. There's quite a bit of work to do here :)

I have not yet started to look at this so will avoid duplicating the effort! All I had started was a branch to add explicit move semantics (which it turns out is not needed): master...willcl-ark:bitcoin:univalue-move

Happy to collaborate though if you have any specific things in mind you'd like help with :)

I do have a somewhat-reproducible benchmark set up for this specifc REST call, it's just querying every 5000th block. I was also using @achow101 's make-big-wallet to make a big wallet last night (wow-- it took 12 hours to generate!) in order to try and benchmark the listtransactions callsite from #25229

I don't think it's strictly needed, as moving a large UniValue is almost certainly an improvement over a large copy, but I still like to see for myself.

willcl-ark · 2024-05-14T12:51:48Z

@theuni FWIW I do see some increased RSS measurements on your current branch vs master (with my one, single test using listtransactions rpc on a large wallet):

This is also what I saw on my linked "univalue-move" branch above. I didn't investigate why exactly this happens, yet. It seems like move might not always be advantageous for us...

glozow · 2024-05-14T13:33:31Z

backport for 26.x in #29899

theuni · 2024-05-14T17:42:10Z

@willcl-ark Thanks!

Huh, I can't think of any reason why that would be the case. Saving on copies should be nothing but a win. Odd.

I think I'll start by opening a PR for the simplest/most obvious moves. Maybe you could help me bench those?

theuni · 2024-05-14T21:13:23Z

@willcl-ark I added a quick/dirty bench to my branch that shows the difference between copies and moves. It seems correct to me.

Do you have any interest in doing the same for memory usage, just as a sanity check that nothing funky is happening low-level? Beyond that, I'm not really sure where to start with your graph... it really doesn't make any sense to me :)

willcl-ark · 2024-05-15T15:00:30Z

You benchmark looks good and reports the result I'd expect too!

I'll investigate my own measurements some more in the mean time...

d7707d9 rpc: avoid copying into UniValue (Cory Fields) Pull request description: These are the simple (and hopefully obviously correct) copies that can be moves instead. This is a follow-up from #30094 (comment) As it turns out, there are hundreds of places where we copy UniValues needlessly. It should be the case that moves are always preferred over copies, so there should be no downside to these changes. willcl-ark, however, noticed that memory usage may increase in some cases. Logically this makes no sense to me. The only plausible explanation imo is that because the moves are faster, more ops/second occur in some cases. This list of moves was obtained by changing the function signatures of the UniValue functions to accept only rvalues, then compiling and fixing them up one by one. There still exist many places where copies are being made. These can/should be fixed up, but weren't done here for the sake of doing the easy ones first. I ran these changes through clang-tidy with `performance-move-const-arg` and `bugprone-use-after-move` and no bugs were detected (though that's obviously not to say it can be trusted 100%). As stated above, there are still lots of other less trivial fixups to do after these including: - Using non-const UniValues where possible so that moves can happen - Refactoring code in order to be able to move a UniValue without introducing a use-after-move - Refactoring functions to accept UniValues by value rather than by const reference ACKs for top commit: achow101: ACK d7707d9 ryanofsky: Code review ACK d7707d9. No changes since last review other than rebase. I agree benchmarks showing increased peak memory usage and RSS are surprising, but number of allocations is down as expected, and runtime is also decreased. willcl-ark: ACK d7707d9 Tree-SHA512: 7f511be73984553c278186286a7d161a34b2574c7f5f1a0edc87c2913b4c025a0af5241ef9af2df17547f2e4ef79710aa5bbb762fc9472435781c0488dba3435

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC. Github-Pull: bitcoin#30094 Rebased-From: b77bad3

rpc: move UniValue in blockToJSON

b77bad3

Without explicitly declaring the move, these UniValues get copied, causing increased memory usage. Fix this by explicitly moving the UniValue objects. Used by `rest_block` and `getblock` RPC.

DrahtBot added the RPC/REST/ZMQ label May 13, 2024

fanquake added the Needs backport (27.x) label May 13, 2024

ismaelsadeeq reviewed May 13, 2024

View reviewed changes

TheCharlatan approved these changes May 13, 2024

View reviewed changes

theuni approved these changes May 13, 2024

View reviewed changes

theuni mentioned this pull request May 13, 2024

refactor: reserve memory allocation for transaction outputs #30093

Open

hebasto approved these changes May 13, 2024

View reviewed changes

BrandonOdiwuor approved these changes May 13, 2024

View reviewed changes

ryanofsky merged commit 0503cbe into bitcoin:master May 13, 2024
16 checks passed

fanquake mentioned this pull request May 14, 2024

[27.x] Backports and rc1 #30092

Open

fanquake removed the Needs backport (27.x) label May 14, 2024

glozow mentioned this pull request May 14, 2024

[26.x] archive 26.1 release notes + backports #29899

Open

theuni mentioned this pull request May 15, 2024

rpc: avoid copying into UniValue #30115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: move UniValue in blockToJSON #30094

rpc: move UniValue in blockToJSON #30094

willcl-ark commented May 13, 2024 •

edited

DrahtBot commented May 13, 2024 •

edited

willcl-ark commented May 13, 2024

maflcko commented May 13, 2024 •

edited

maflcko commented May 13, 2024

willcl-ark commented May 13, 2024

maflcko commented May 13, 2024

ismaelsadeeq left a comment

TheCharlatan left a comment

theuni left a comment

theuni commented May 13, 2024

hebasto left a comment

BrandonOdiwuor left a comment

willcl-ark commented May 13, 2024

theuni commented May 13, 2024

theuni commented May 13, 2024 •

edited

fanquake commented May 14, 2024

willcl-ark commented May 14, 2024

willcl-ark commented May 14, 2024

glozow commented May 14, 2024

theuni commented May 14, 2024

theuni commented May 14, 2024

willcl-ark commented May 15, 2024

rpc: move UniValue in blockToJSON #30094

rpc: move UniValue in blockToJSON #30094

Conversation

willcl-ark commented May 13, 2024 • edited

DrahtBot commented May 13, 2024 • edited

Code Coverage

Reviews

willcl-ark commented May 13, 2024

maflcko commented May 13, 2024 • edited

maflcko commented May 13, 2024

willcl-ark commented May 13, 2024

maflcko commented May 13, 2024

ismaelsadeeq left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

theuni left a comment

Choose a reason for hiding this comment

theuni commented May 13, 2024

hebasto left a comment

Choose a reason for hiding this comment

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

willcl-ark commented May 13, 2024

theuni commented May 13, 2024

theuni commented May 13, 2024 • edited

fanquake commented May 14, 2024

willcl-ark commented May 14, 2024

willcl-ark commented May 14, 2024

glozow commented May 14, 2024

theuni commented May 14, 2024

theuni commented May 14, 2024

willcl-ark commented May 15, 2024

willcl-ark commented May 13, 2024 •

edited

DrahtBot commented May 13, 2024 •

edited

maflcko commented May 13, 2024 •

edited

theuni commented May 13, 2024 •

edited