rpc: move UniValue in blockToJSON #30094

pull willcl-ark wants to merge 1 commits into bitcoin:master from willcl-ark:move-univalue-only changing 1 files +2 −2
  1. willcl-ark commented at 1:38 pm on May 13, 2024: member

    Fixes: #24542 Fixes: #30052

    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.

  2. rpc: move UniValue in blockToJSON 
    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.
    b77bad309e
  3. DrahtBot commented at 1:38 pm on May 13, 2024: contributor

    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.

  4. DrahtBot added the label RPC/REST/ZMQ on May 13, 2024
  5. fanquake added the label Needs backport (27.x) on May 13, 2024
  6. willcl-ark commented at 1:39 pm on May 13, 2024: member

    Before and after of RSS:

    rest-comparison-27-move

  7. maflcko commented at 1:39 pm on May 13, 2024: member
    review ACK b77bad309e92f176f340598eec056eb7bff86f5f
  8. maflcko commented at 1:48 pm on May 13, 2024: member
    Also “fixes” #30052
  9. willcl-ark commented at 1:50 pm on May 13, 2024: member

    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

  10. maflcko commented at 1:51 pm on May 13, 2024: member
    Unrelated to the blockToJSON changes here, as a follow-up, it may be possible to “fix” #25229 in the same way?
  11. ismaelsadeeq commented at 1:53 pm on May 13, 2024: member
    ACK b77bad309e92f176f340598eec056eb7bff86f5f
  12. TheCharlatan approved
  13. TheCharlatan commented at 1:55 pm on May 13, 2024: contributor
    ACK b77bad309e92f176f340598eec056eb7bff86f5f
  14. theuni approved
  15. theuni commented at 3:49 pm on May 13, 2024: member
    utACK b77bad309e92f176f340598eec056eb7bff86f5f
  16. theuni commented at 5:12 pm on May 13, 2024: member
    Actually, looks like this could use a txs.reserve(block.vtx.size()) as well.
  17. hebasto approved
  18. hebasto commented at 5:26 pm on May 13, 2024: member

    ACK b77bad309e92f176f340598eec056eb7bff86f5f, 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.

  19. luke-jr referenced this in commit 6ce4d5a977 on May 13, 2024
  20. BrandonOdiwuor approved
  21. BrandonOdiwuor commented at 6:42 pm on May 13, 2024: contributor
    ACK b77bad309e92f176f340598eec056eb7bff86f5f
  22. willcl-ark commented at 6:44 pm on May 13, 2024: member

    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.

  23. theuni commented at 6:46 pm on May 13, 2024: member

    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 :)

  24. ryanofsky merged this on May 13, 2024
  25. ryanofsky closed this on May 13, 2024
  26. theuni commented at 8:33 pm on May 13, 2024: member

    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 :)

  27. fanquake referenced this in commit 0ba11cf908 on May 14, 2024
  28. fanquake removed the label Needs backport (27.x) on May 14, 2024
  29. fanquake commented at 0:16 am on May 14, 2024: member
    Backported to 27.x in #30092.
  30. glozow referenced this in commit 7d13e6ab51 on May 14, 2024
  31. willcl-ark commented at 10:41 am on May 14, 2024: member

    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): https://github.com/bitcoin/bitcoin/compare/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.

  32. willcl-ark commented at 12:51 pm on May 14, 2024: member

    @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):

    univalue-move-comparison

    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…

  33. glozow commented at 1:33 pm on May 14, 2024: member
    backport for 26.x in #29899
  34. theuni commented at 5:42 pm on May 14, 2024: member

    @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?

  35. theuni commented at 9:13 pm on May 14, 2024: member

    @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 :)

  36. willcl-ark commented at 3:00 pm on May 15, 2024: member

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

    I’ll investigate my own measurements some more in the mean time…


willcl-ark DrahtBot maflcko ismaelsadeeq TheCharlatan theuni hebasto BrandonOdiwuor fanquake glozow

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-27 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me