rpc: avoid copying into UniValue #30115

1. 
   theuni commented at 6:52 pm on May 15, 2024: member

   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
2. 
   DrahtBot commented at 6:52 pm on May 15, 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	achow101, ryanofsky, willcl-ark
   Concept ACK	TheCharlatan, hebasto, laanwj, stickies-v

   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:

   • #30058 (Encapsulate warnings in generalized node::Warnings and remove globals by stickies-v)
   • #29039 (versionbits refactoring by ajtowns)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)
   • #27638 (rpc: show P2(W)SH redeemScript in getrawtransaction #27637 by Riahiamirreza)
   • #25979 ([WIP] wallet: standardize change output detection process by furszy)
   • #21283 (Implement BIP 370 PSBTv2 by achow101)

   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.

3. DrahtBot added the label RPC/REST/ZMQ on May 15, 2024
4. ryanofsky approved
5. 
   ryanofsky commented at 8:45 pm on May 15, 2024: contributor

   Code review ACK 35b5b0ff9e043c1e26a3e29fa3d9d914a0a86ad1. I confirmed this change is only adding std::moves, and also tried to look for cases where an object was being used after a move, since that seems like the most likely type of bug this PR could introduce. I didn’t check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.
6. 
   theuni commented at 1:44 am on May 16, 2024: member

   I didn’t check exhaustively, though, since that would be a lot more work and I think we have a clang-tidy check that would catch those cases.

   Indeed, I believe c-i should be doing that check. But just in case, from the PR description:

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

7. 
   TheCharlatan commented at 11:56 am on May 16, 2024: contributor

   Concept ACK

   Could this be a case for a clang-tidy plugin?

8. 
   theuni commented at 3:45 pm on May 16, 2024: member

   Could this be a case for a clang-tidy plugin?

   I considered this, but I don’t think so. UniValue copies are reasonable in many cases, but our use of them often lends itself to moving. So we can’t detect and disable copies as a rule, and (I assume) if clang-tidy could detect and suggest possible moves as optimizations, it would be offering a generic version of that already.

   Here’s an upstream discussion about it that has apparently gone stale: https://github.com/llvm/llvm-project/issues/53489

9. 
   hebasto commented at 3:45 pm on May 16, 2024: member

   Concept ACK.
10. DrahtBot added the label Needs rebase on May 16, 2024
11. 
    ryanofsky commented at 3:57 pm on May 16, 2024: contributor

    Could this be a case for a clang-tidy plugin?

    I think the ideal thing to do for types that are potentially expensive to copy is to disable implicit copies, but provide explicit T Copy() const and void CopyFrom(const T&) methods so copies can be made where needed. (There was a DISABLE_IMPLICIT_COPIES macro proposed in #24641 that tried to do this, but it didn’t seem possible to generalize the implementation so we never added it.) It think it would be probably be reasonable to allow univalue to be explicitly but not implicitly copied, but I’m sure @theuni would have a better intuition.

12. 
    laanwj commented at 6:29 pm on May 16, 2024: member

    Concept ACK.

    Agree with @ryanofsky that if copy is something expensive (or generally undesirable) to do, it would make sense to make copy explicit, so that it is always a deliberate choice and stands out in code review.

13. 
    stickies-v commented at 9:39 am on May 17, 2024: contributor

    Concept ACK

    Indeed, I believe c-i should be doing that check.

    It does: https://github.com/bitcoin/bitcoin/blob/2f53f2273da020d7fabd7c65a1bc7e69a31249b2/src/.clang-tidy#L6

14. 
    willcl-ark commented at 12:56 pm on May 17, 2024: member

    Concept ACK.

    The code changes all look correct to me too, however

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

    I still have not been able to explain why i see these type of results, but I think it’s most likely user error somewhere along the line.

    Here is one example of a puzzling result I have, in this test fetching every 5000th block via REST (previously largely addressed by #30094). In all screenshots this PR is on the right hand side:

    We can see here this PR made 96m allocations, vs 137m, a strict improvement?

    

    The unnecessary calls to allocator functions are clearly gotten rid of:

    

    However despite that total resource usage increases, albeit only a little, ~10MB (also see first image) :

    

    

    The changes in this PR which affect this test (since #30094) are in core_write.cpp

    I guess this may just be allocator stuff I might never get to the bottom of, but I will continue to investigate this for a little bit before bringing a full ACK back :)

    FWIW these changes did provide a measurable speedup on my test, roughly of this order:

    • this PR: ~ 1:43
    • master: ~ 1:55

    ---

    I also see similar result just fetching a single block (in this case block 800,000). Many fewer allocations, but larger total usage:

    

    

    I think the clues may lie in here, but I am too dumb to know what they are. For some reason after this PR a second ~30MB allocation is made. Without this PR there are 37.2MB and 24.0MB allocations made,, and with this PR those are 38.2MB and 30.9MB, and I don’t understand why:

    

    heaptrack dumps in case anyone else wants to take a look:

    heaptrack_dumps.zip

15. 
    theuni commented at 5:00 pm on May 17, 2024: member

    @willcl-ark Thanks, that’s very helpful. Out of curiosity, what build options are you using for these? Are optimizations on?
16. 
    maflcko commented at 3:05 pm on May 20, 2024: member

    Maybe rebase and re-bench after 75118a608fc22a57567743000d636bc1f969f748, which replaced some copies with moves as well?
17. 
    theuni commented at 3:23 pm on May 20, 2024: member

    Maybe rebase and re-bench after 75118a6, which replaced some copies with moves as well?

    Ah, great, thanks for pointing me to that. I had pretty much this same commit locally to address the third point in this PR’s description:

    • Refactoring functions to accept UniValues by value rather than by const reference

    Though I think there are still other functions that need the same treatment.

18. rpc: avoid copying into UniValue

    These are simple (and hopefully obviously correct) copies that can be moves
    instead.

    d7707d9843
19. theuni force-pushed on May 20, 2024
20. DrahtBot removed the label Needs rebase on May 20, 2024
21. 
    theuni commented at 3:47 pm on May 22, 2024: member

    I think it’s not really worth worrying about benchmarks here, I think it’s pretty clear that moving can only be better.

    I’d like to get this in if there are no objections, as there’s some more low-hanging fruit wrt UniValue moves.

22. 
    achow101 commented at 5:57 pm on May 22, 2024: member

    ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f

    Checked that only std::moves were added, and letting the compiler warn for use after move, which there does not appear to be any.

23. DrahtBot requested review from laanwj on May 22, 2024
24. DrahtBot requested review from stickies-v on May 22, 2024
25. DrahtBot requested review from willcl-ark on May 22, 2024
26. DrahtBot requested review from ryanofsky on May 22, 2024
27. DrahtBot requested review from hebasto on May 22, 2024
28. DrahtBot requested review from TheCharlatan on May 22, 2024
29. ryanofsky approved
30. 
    ryanofsky commented at 6:55 pm on May 22, 2024: contributor

    Code review ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f. 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.

    The fact that “peak allocations” appear to be larger after the change seems interesting. Before the change, the largest allocations are 38.4MB, 26.7MB, and 8.4MB, but after the change they are 39.4MB, 33.7MB, and 10.8MB. I’m not sure what could account for this though.

    In general, it seems like using std::move could increase memory usage with vectors, because if you std::move into a vector, the new vector has the same capacity as the existing vector, but when you copy into a vector, the library could choose a smaller capacity just matching the size, and overall memory usage would be lower after the first vector is destroyed.

31. 
    sipa commented at 8:08 pm on May 22, 2024: member

    It’s unclear to me whether the UniValue type has a move constructor, actually. If not, then these std::moves have no effect. @ryanofsky If you theory is correct, then having a way to invoke shrink_to_fit on UniValue’s internal vector might be useful.
32. 
    theuni commented at 8:16 pm on May 22, 2024: member

    It’s unclear to me whether the UniValue type has a move constructor, actually. If not, then these std::moves have no effect. @sipa I worked up a quick bench to test exactly that: https://github.com/theuni/bitcoin/commit/35d5ffc793b59bd03b8f66bf68847ab773e914f3

    The numbers look as you’d expect.

    0|               ns/op |                op/s |    err% |     total | benchmark
    1|--------------------:|--------------------:|--------:|----------:|:----------
    2|        7,273,304.00 |              137.49 |    0.1% |      0.08 | `UniValueAssignCopy`
    3|        1,040,156.00 |              961.39 |    0.5% |      0.02 | `UniValueAssignMove`
    4|        9,336,776.00 |              107.10 |    0.4% |      0.10 | `UniValueConstructCopy`
    5|          991,626.00 |            1,008.44 |    0.7% |      0.02 | `UniValueConstructMove`
    6|       11,783,927.00 |               84.86 |    0.2% |      0.13 | `UniValuePushKVCopy`
    7|        2,344,899.00 |              426.46 |    0.3% |      0.03 | `UniValuePushKVMove`
    

33. 
    maflcko commented at 10:08 am on May 23, 2024: member

    It’s unclear to me whether the UniValue type has a move constructor, actually. If not, then these std::moves have no effect.

    Since this question keeps coming up (https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2107357612), what about adding static_assert(std::is_move_constructible_v<UniValue>); somewhere?

34. 
    maflcko commented at 11:02 am on May 23, 2024: member

    @ryanofsky If you theory is correct, then having a way to invoke shrink_to_fit on UniValue’s internal vector might be useful.

    Not sure if this is ideal. UniValue is a recursive structure, so calling it on the top level vector only shouldn’t cause any difference? Similarly, if shirking is done recursively, the runtime overhead will be equal to that of a copy, so might as well just do a copy instead? Finally, whenever a UniValue would be ready to shrink, it is usually one step away from being deleted completely anyway. The only missing step is to call write() on it to convert it to bytes to send on the wire. However that leads to an alternative, where a new write_and_destroy function is added, which recursively shrinks (deletes) the UniValue as soon as it was written to the stream. From a runtime-overhead this should be free (as the univalue is destroyed afterward anyway). I haven’t tested or implemented this, but this may be a start:

      0diff --git a/src/common/args.cpp b/src/common/args.cpp
      1index c90eb0c685..e66f18277d 100644
      2--- a/src/common/args.cpp
      3+++ b/src/common/args.cpp
      4@@ -248,7 +248,7 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
      5         const common::SettingsSpan values{*includes};
      6         // Range may be empty if -noincludeconf was passed
      7         if (!values.empty()) {
      8-            error = "-includeconf cannot be used from commandline; -includeconf=" + values.begin()->write();
      9+            error = "-includeconf cannot be used from commandline; -includeconf=" /*+ values.begin()->write_const()*/;
     10             return false; // pick first value as example
     11         }
     12     }
     13@@ -811,7 +811,7 @@ void ArgsManager::logArgsPrefix(
     14         for (const auto& value : arg.second) {
     15             std::optional<unsigned int> flags = GetArgFlags('-' + arg.first);
     16             if (flags) {
     17-                std::string value_str = (*flags & SENSITIVE) ? "****" : value.write();
     18+                std::string value_str = (*flags & SENSITIVE) ? "****" : ""/*value.write_const()*/;
     19                 LogPrintf("%s %s%s=%s\n", prefix, section_str, arg.first, value_str);
     20             }
     21         }
     22@@ -825,7 +825,7 @@ void ArgsManager::LogArgs() const
     23         logArgsPrefix("Config file arg:", section.first, section.second);
     24     }
     25     for (const auto& setting : m_settings.rw_settings) {
     26-        LogPrintf("Setting file arg: %s = %s\n", setting.first, setting.second.write());
     27+        LogPrintf("Setting file arg: %s = %s\n", setting.first, ""/*setting.second.write_const()*/);
     28     }
     29     logArgsPrefix("Command-line arg:", "", m_settings.command_line_options);
     30 }
     31diff --git a/src/common/settings.cpp b/src/common/settings.cpp
     32index c1520dacd2..268ab39f1e 100644
     33--- a/src/common/settings.cpp
     34+++ b/src/common/settings.cpp
     35@@ -99,7 +99,7 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
     36     file.close(); // Done with file descriptor. Release while copying data.
     37 
     38     if (!in.isObject()) {
     39-        errors.emplace_back(strprintf("Found non-object value %s in settings file %s", in.write(), fs::PathToString(path)));
     40+        errors.emplace_back(strprintf("Found non-object value %s in settings file %s", in.write_and_destroy(), fs::PathToString(path)));
     41         return false;
     42     }
     43 
     44@@ -138,7 +138,7 @@ bool WriteSettings(const fs::path& path,
     45         errors.emplace_back(strprintf("Error: Unable to open settings file %s for writing", fs::PathToString(path)));
     46         return false;
     47     }
     48-    file << out.write(/* prettyIndent= */ 4, /* indentLevel= */ 1) << std::endl;
     49+    file << out.write_and_destroy(/* prettyIndent= */ 4, /* indentLevel= */ 1) << std::endl;
     50     file.close();
     51     return true;
     52 }
     53diff --git a/src/httprpc.cpp b/src/httprpc.cpp
     54index 3eb34dbe6a..91b1574afc 100644
     55--- a/src/httprpc.cpp
     56+++ b/src/httprpc.cpp
     57@@ -87,7 +87,7 @@ static void JSONErrorReply(HTTPRequest* req, UniValue objError, const JSONRPCReq
     58     else if (code == RPC_METHOD_NOT_FOUND)
     59         nStatus = HTTP_NOT_FOUND;
     60 
     61-    std::string strReply = JSONRPCReplyObj(NullUniValue, std::move(objError), jreq.id, jreq.m_json_version).write() + "\n";
     62+    std::string strReply = JSONRPCReplyObj(NullUniValue, std::move(objError), jreq.id, jreq.m_json_version).write_and_destroy() + "\n";
     63 
     64     req->WriteHeader("Content-Type", "application/json");
     65     req->WriteReply(nStatus, strReply);
     66@@ -273,7 +273,7 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
     67             throw JSONRPCError(RPC_PARSE_ERROR, "Top-level object parse error");
     68 
     69         req->WriteHeader("Content-Type", "application/json");
     70-        req->WriteReply(HTTP_OK, reply.write() + "\n");
     71+        req->WriteReply(HTTP_OK, reply.write_and_destroy() + "\n");
     72     } catch (UniValue& e) {
     73         JSONErrorReply(req, std::move(e), jreq);
     74         return false;
     75diff --git a/src/rest.cpp b/src/rest.cpp
     76index 9fc5d4af04..f83133e187 100644
     77--- a/src/rest.cpp
     78+++ b/src/rest.cpp
     79@@ -269,7 +269,7 @@ static bool rest_headers(const std::any& context,
     80         for (const CBlockIndex *pindex : headers) {
     81             jsonHeaders.push_back(blockheaderToJSON(*tip, *pindex));
     82         }
     83-        std::string strJSON = jsonHeaders.write() + "\n";
     84+        std::string strJSON = jsonHeaders.write_and_destroy() + "\n";
     85         req->WriteHeader("Content-Type", "application/json");
     86         req->WriteReply(HTTP_OK, strJSON);
     87         return true;
     88@@ -338,7 +338,7 @@ static bool rest_block(const std::any& context,
     89         DataStream block_stream{block_data};
     90         block_stream >> TX_WITH_WITNESS(block);
     91         UniValue objBlock = blockToJSON(chainman.m_blockman, block, *tip, *pblockindex, tx_verbosity);
     92-        std::string strJSON = objBlock.write() + "\n";
     93+        std::string strJSON = objBlock.write_and_destroy() + "\n";
     94         req->WriteHeader("Content-Type", "application/json");
     95         req->WriteReply(HTTP_OK, strJSON);
     96         return true;
     97@@ -472,7 +472,7 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
     98             jsonHeaders.push_back(header.GetHex());
     99         }
    100 
    101-        std::string strJSON = jsonHeaders.write() + "\n";
    102+        std::string strJSON = jsonHeaders.write_and_destroy() + "\n";
    103         req->WriteHeader("Content-Type", "application/json");
    104         req->WriteReply(HTTP_OK, strJSON);
    105         return true;
    106@@ -564,7 +564,7 @@ static bool rest_block_filter(const std::any& context, HTTPRequest* req, const s
    107     case RESTResponseFormat::JSON: {
    108         UniValue ret(UniValue::VOBJ);
    109         ret.pushKV("filter", HexStr(filter.GetEncodedFilter()));
    110-        std::string strJSON = ret.write() + "\n";
    111+        std::string strJSON = ret.write_and_destroy() + "\n";
    112         req->WriteHeader("Content-Type", "application/json");
    113         req->WriteReply(HTTP_OK, strJSON);
    114         return true;
    115@@ -591,7 +591,7 @@ static bool rest_chaininfo(const std::any& context, HTTPRequest* req, const std:
    116         jsonRequest.context = context;
    117         jsonRequest.params = UniValue(UniValue::VARR);
    118         UniValue chainInfoObject = getblockchaininfo().HandleRequest(jsonRequest);
    119-        std::string strJSON = chainInfoObject.write() + "\n";
    120+        std::string strJSON = chainInfoObject.write_and_destroy() + "\n";
    121         req->WriteHeader("Content-Type", "application/json");
    122         req->WriteReply(HTTP_OK, strJSON);
    123         return true;
    124@@ -634,7 +634,7 @@ static bool rest_deploymentinfo(const std::any& context, HTTPRequest* req, const
    125         }
    126 
    127         req->WriteHeader("Content-Type", "application/json");
    128-        req->WriteReply(HTTP_OK, getdeploymentinfo().HandleRequest(jsonRequest).write() + "\n");
    129+        req->WriteReply(HTTP_OK, getdeploymentinfo().HandleRequest(jsonRequest).write_and_destroy() + "\n");
    130         return true;
    131     }
    132     default: {
    133@@ -685,9 +685,9 @@ static bool rest_mempool(const std::any& context, HTTPRequest* req, const std::s
    134             if (verbose && mempool_sequence) {
    135                 return RESTERR(req, HTTP_BAD_REQUEST, "Verbose results cannot contain mempool sequence values. (hint: set \"verbose=false\")");
    136             }
    137-            str_json = MempoolToJSON(*mempool, verbose, mempool_sequence).write() + "\n";
    138+            str_json = MempoolToJSON(*mempool, verbose, mempool_sequence).write_and_destroy() + "\n";
    139         } else {
    140-            str_json = MempoolInfoToJSON(*mempool).write() + "\n";
    141+            str_json = MempoolInfoToJSON(*mempool).write_and_destroy() + "\n";
    142         }
    143 
    144         req->WriteHeader("Content-Type", "application/json");
    145@@ -747,7 +747,7 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string
    146     case RESTResponseFormat::JSON: {
    147         UniValue objTx(UniValue::VOBJ);
    148         TxToUniv(*tx, /*block_hash=*/hashBlock, /*entry=*/ objTx);
    149-        std::string strJSON = objTx.write() + "\n";
    150+        std::string strJSON = objTx.write_and_destroy() + "\n";
    151         req->WriteHeader("Content-Type", "application/json");
    152         req->WriteReply(HTTP_OK, strJSON);
    153         return true;
    154@@ -940,7 +940,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
    155         objGetUTXOResponse.pushKV("utxos", utxos);
    156 
    157         // return json string
    158-        std::string strJSON = objGetUTXOResponse.write() + "\n";
    159+        std::string strJSON = objGetUTXOResponse.write_and_destroy() + "\n";
    160         req->WriteHeader("Content-Type", "application/json");
    161         req->WriteReply(HTTP_OK, strJSON);
    162         return true;
    163@@ -992,7 +992,7 @@ static bool rest_blockhash_by_height(const std::any& context, HTTPRequest* req,
    164         req->WriteHeader("Content-Type", "application/json");
    165         UniValue resp = UniValue(UniValue::VOBJ);
    166         resp.pushKV("blockhash", pblockindex->GetBlockHash().GetHex());
    167-        req->WriteReply(HTTP_OK, resp.write() + "\n");
    168+        req->WriteReply(HTTP_OK, resp.write_and_destroy() + "\n");
    169         return true;
    170     }
    171     default: {
    172diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
    173index f5a2e9eb63..4c1d64eb35 100644
    174--- a/src/rpc/util.cpp
    175+++ b/src/rpc/util.cpp
    176@@ -166,7 +166,7 @@ std::string HelpExampleCliNamed(const std::string& methodname, const RPCArgList&
    177     for (const auto& argpair: args) {
    178         const auto& value = argpair.second.isStr()
    179                 ? argpair.second.get_str()
    180-                : argpair.second.write();
    181+                : ""/*argpair.second.write()*/;
    182         result += " " + argpair.first + "=" + ShellQuoteIfNeeded(value);
    183     }
    184     result += "\n";
    185@@ -187,7 +187,7 @@ std::string HelpExampleRpcNamed(const std::string& methodname, const RPCArgList&
    186     }
    187 
    188     return "> curl --user myusername --data-binary '{\"jsonrpc\": \"2.0\", \"id\": \"curltest\", "
    189-           "\"method\": \"" + methodname + "\", \"params\": " + params.write() + "}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n";
    190+           "\"method\": \"" + methodname + "\", \"params\": " + params.write_and_destroy() + "}' -H 'content-type: text/plain;' http://127.0.0.1:8332/\n";
    191 }
    192 
    193 // Converts a hex string to a public key if possible
    194@@ -631,7 +631,7 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
    195         }
    196     }
    197     if (!arg_mismatch.empty()) {
    198-        throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Wrong type passed:\n%s", arg_mismatch.write(4)));
    199+        throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Wrong type passed:\n%s", arg_mismatch.write_and_destroy(4)));
    200     }
    201     CHECK_NONFATAL(m_req == nullptr);
    202     m_req = &request;
    203@@ -650,8 +650,8 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
    204         if (!mismatch.isNull()) {
    205             std::string explain{
    206                 mismatch.empty() ? "no possible results defined" :
    207-                mismatch.size() == 1 ? mismatch[0].write(4) :
    208-                mismatch.write(4)};
    209+                mismatch.size() == 1 ? ""/* mismatch[0].write_and_destroy(4) */:
    210+                mismatch.write_and_destroy(4)};
    211             throw std::runtime_error{
    212                 strprintf("Internal bug detected: RPC call \"%s\" returned incorrect type:\n%s\n%s %s\nPlease report this issue here: %s\n",
    213                           m_name, explain,
    214@@ -950,7 +950,7 @@ std::string RPCArg::ToDescriptionString(bool is_named_arg) const
    215     if (m_fallback.index() == 1) {
    216         ret += ", optional, default=" + std::get<RPCArg::DefaultHint>(m_fallback);
    217     } else if (m_fallback.index() == 2) {
    218-        ret += ", optional, default=" + std::get<RPCArg::Default>(m_fallback).write();
    219+        ret += ", optional, default=" ;//+ std::get<RPCArg::Default>(m_fallback).write();
    220     } else {
    221         switch (std::get<RPCArg::Optional>(m_fallback)) {
    222         case RPCArg::Optional::OMITTED: {
    223diff --git a/src/univalue/include/univalue.h b/src/univalue/include/univalue.h
    224index da12157555..cc741eb84f 100644
    225--- a/src/univalue/include/univalue.h
    226+++ b/src/univalue/include/univalue.h
    227@@ -73,7 +73,9 @@ public:
    228     void getObjMap(std::map<std::string,UniValue>& kv) const;
    229     bool checkObject(const std::map<std::string,UniValue::VType>& memberTypes) const;
    230     const UniValue& operator[](const std::string& key) const;
    231+    UniValue& operator[](const std::string& key);
    232     const UniValue& operator[](size_t index) const;
    233+    UniValue& operator[](size_t index);
    234     bool exists(const std::string& key) const { size_t i; return findKey(key, i); }
    235 
    236     bool isNull() const { return (typ == VNULL); }
    237@@ -94,8 +96,7 @@ public:
    238     void pushKV(std::string key, UniValue val);
    239     void pushKVs(UniValue obj);
    240 
    241-    std::string write(unsigned int prettyIndent = 0,
    242-                      unsigned int indentLevel = 0) const;
    243+    std::string write_and_destroy(unsigned int prettyIndent = 0,                       unsigned int indentLevel = 0) ;
    244 
    245     bool read(std::string_view raw);
    246 
    247@@ -107,8 +108,8 @@ private:
    248 
    249     void checkType(const VType& expected) const;
    250     bool findKey(const std::string& key, size_t& retIdx) const;
    251-    void writeArray(unsigned int prettyIndent, unsigned int indentLevel, std::string& s) const;
    252-    void writeObject(unsigned int prettyIndent, unsigned int indentLevel, std::string& s) const;
    253+    void writeArray(unsigned int prettyIndent, unsigned int indentLevel, std::string& s);
    254+    void writeObject(unsigned int prettyIndent, unsigned int indentLevel, std::string& s);
    255 
    256 public:
    257     // Strict type-specific getters, these throw std::runtime_error if the
    258diff --git a/src/univalue/lib/univalue.cpp b/src/univalue/lib/univalue.cpp
    259index 656d2e8203..8ceab1fc31 100644
    260--- a/src/univalue/lib/univalue.cpp
    261+++ b/src/univalue/lib/univalue.cpp
    262@@ -197,6 +197,13 @@ const UniValue& UniValue::operator[](const std::string& key) const
    263     return values.at(index);
    264 }
    265 
    266+UniValue& UniValue::operator[](const std::string& key)
    267+{
    268+    size_t index(-1);
    269+    if (!findKey(key, index)) index=-1;
    270+    return values.at(index);
    271+}
    272+
    273 const UniValue& UniValue::operator[](size_t index) const
    274 {
    275     if (typ != VOBJ && typ != VARR)
    276@@ -207,6 +214,11 @@ const UniValue& UniValue::operator[](size_t index) const
    277     return values.at(index);
    278 }
    279 
    280+UniValue& UniValue::operator[](size_t index)
    281+{
    282+    return values.at(index);
    283+}
    284+
    285 void UniValue::checkType(const VType& expected) const
    286 {
    287     if (typ != expected) {
    288diff --git a/src/univalue/lib/univalue_write.cpp b/src/univalue/lib/univalue_write.cpp
    289index 4a2219061a..df10467fb6 100644
    290--- a/src/univalue/lib/univalue_write.cpp
    291+++ b/src/univalue/lib/univalue_write.cpp
    292@@ -28,8 +28,8 @@ static std::string json_escape(const std::string& inS)
    293 }
    294 
    295 // NOLINTNEXTLINE(misc-no-recursion)
    296-std::string UniValue::write(unsigned int prettyIndent,
    297-                            unsigned int indentLevel) const
    298+std::string UniValue::write_and_destroy(unsigned int prettyIndent,
    299+                            unsigned int indentLevel) 
    300 {
    301     std::string s;
    302     s.reserve(1024);
    303@@ -59,6 +59,7 @@ std::string UniValue::write(unsigned int prettyIndent,
    304         break;
    305     }
    306 
    307+*this = {}; // delete
    308     return s;
    309 }
    310 
    311@@ -68,7 +69,7 @@ static void indentStr(unsigned int prettyIndent, unsigned int indentLevel, std::
    312 }
    313 
    314 // NOLINTNEXTLINE(misc-no-recursion)
    315-void UniValue::writeArray(unsigned int prettyIndent, unsigned int indentLevel, std::string& s) const
    316+void UniValue::writeArray(unsigned int prettyIndent, unsigned int indentLevel, std::string& s)
    317 {
    318     s += "[";
    319     if (prettyIndent)
    320@@ -77,7 +78,7 @@ void UniValue::writeArray(unsigned int prettyIndent, unsigned int indentLevel, s
    321     for (unsigned int i = 0; i < values.size(); i++) {
    322         if (prettyIndent)
    323             indentStr(prettyIndent, indentLevel, s);
    324-        s += values[i].write(prettyIndent, indentLevel + 1);
    325+        s += values[i].write_and_destroy(prettyIndent, indentLevel + 1);
    326         if (i != (values.size() - 1)) {
    327             s += ",";
    328         }
    329@@ -91,7 +92,7 @@ void UniValue::writeArray(unsigned int prettyIndent, unsigned int indentLevel, s
    330 }
    331 
    332 // NOLINTNEXTLINE(misc-no-recursion)
    333-void UniValue::writeObject(unsigned int prettyIndent, unsigned int indentLevel, std::string& s) const
    334+void UniValue::writeObject(unsigned int prettyIndent, unsigned int indentLevel, std::string& s)
    335 {
    336     s += "{";
    337     if (prettyIndent)
    338@@ -103,7 +104,7 @@ void UniValue::writeObject(unsigned int prettyIndent, unsigned int indentLevel,
    339         s += "\"" + json_escape(keys[i]) + "\":";
    340         if (prettyIndent)
    341             s += " ";
    342-        s += values.at(i).write(prettyIndent, indentLevel + 1);
    343+        s += values.at(i).write_and_destroy(prettyIndent, indentLevel + 1);
    344         if (i != (values.size() - 1))
    345             s += ",";
    346         if (prettyIndent)
    

35. willcl-ark approved
36. 
    willcl-ark commented at 1:08 pm on May 23, 2024: member

    ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f

    Whilst I still measure slightly increased resource usage vs master, even after adding some strategic calls to shrink_to_fit() on the Univalue.values member (which likely didn’t work for the reasons @maflcko described above), I am now convinced this is not something to worry about in practice.

    I’ve done more measurements over the last few days, and even when testing codepaths which create very large numbers of UniValues, I only ever see a few MB more RAM being used at peak. In the order of single digit percentage points. And, this excess does not appear to grow over time.

    Therefore in my opinion this effect, where it’s measurable/noticable at all, is more than offset by the reduced number of allocations (e.g. I’ve seen 106,000,000 allocations being reduced to 60,000,000 in larger tests using e.g. blockToJSON), and the speedups shown using @theuni’s benchmark.

37. ryanofsky merged this on May 23, 2024
38. ryanofsky closed this on May 23, 2024

    ---

39. 
    ryanofsky commented at 2:58 pm on May 23, 2024: contributor

    Went ahead and merged this. It seems the increased peak memory usage is not a major concern, and other metrics like number of allocations and run time do seem improved.

    Marco seems right that naively calling shrink_to_fit might not help much, or could be basically equivalent to copying, and there might be better ways to decrease memory usage, such as with a write_and_destroy method.

    (Note: I also edited PR description to remove @ from @willcl-ark, so Will doesn’t receive github spam when the merge commit is pulled into other repositories.)

40. 
    theuni commented at 3:03 pm on May 23, 2024: member

    @ryanofsky Thanks.

    I’m going to continue working on the more complicated copies in follow-up PRs. I’ll play around with your Copy/CopyFrom idea while I’m at it. If nothing else, that would be a good way to help me track down the remaining cases.

Contributors
theuni DrahtBot ryanofsky TheCharlatan hebasto laanwj stickies-v willcl-ark maflcko achow101 sipa

Review Requested
laanwj TheCharlatan hebasto stickies-v

Labels
RPC/REST/ZMQ