Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#30115: rpc: avoid copying into UniValue
Browse files Browse the repository at this point in the history 
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 bitcoin/bitcoin#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
  • Loading branch information
@ryanofsky
ryanofsky committed May 23, 2024
2 parents e163d86 + d7707d9 commit 6300438
Show file tree
Hide file tree
Showing 27 changed files with 183 additions and 183 deletions.
6 changes: 3 additions & 3 deletions src/bitcoin-cli.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ class AddrinfoRequestHandler : public BaseRequestHandler
total += counts.at(i);
}
addresses.pushKV("total", total);
result.pushKV("addresses_known", addresses);
result.pushKV("addresses_known", std::move(addresses));
return JSONRPCReplyObj(std::move(result), NullUniValue, /*id=*/1, JSONRPCVersion::V1_LEGACY);
}
};
Expand Down Expand Up @@ -348,7 +348,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
connections.pushKV("in", batch[ID_NETWORKINFO]["result"]["connections_in"]);
connections.pushKV("out", batch[ID_NETWORKINFO]["result"]["connections_out"]);
connections.pushKV("total", batch[ID_NETWORKINFO]["result"]["connections"]);
result.pushKV("connections", connections);
result.pushKV("connections", std::move(connections));

result.pushKV("networks", batch[ID_NETWORKINFO]["result"]["networks"]);
result.pushKV("difficulty", batch[ID_BLOCKCHAININFO]["result"]["difficulty"]);
Expand Down Expand Up @@ -940,7 +940,7 @@ static void GetWalletBalances(UniValue& result)
const UniValue& balance = getbalances.find_value("result")["mine"]["trusted"];
balances.pushKV(wallet_name, balance);
}
result.pushKV("balances", balances);
result.pushKV("balances", std::move(balances));
}

/**
Expand Down
18 changes: 9 additions & 9 deletions src/core_write.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,14 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
UniValue o(UniValue::VOBJ);
o.pushKV("asm", ScriptToAsmStr(txin.scriptSig, true));
o.pushKV("hex", HexStr(txin.scriptSig));
in.pushKV("scriptSig", o);
in.pushKV("scriptSig", std::move(o));
}
if (!tx.vin[i].scriptWitness.IsNull()) {
UniValue txinwitness(UniValue::VARR);
for (const auto& item : tx.vin[i].scriptWitness.stack) {
txinwitness.push_back(HexStr(item));
}
in.pushKV("txinwitness", txinwitness);
in.pushKV("txinwitness", std::move(txinwitness));
}
if (have_undo) {
const Coin& prev_coin = txundo->vprevout[i];
Expand All @@ -224,14 +224,14 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
p.pushKV("generated", bool(prev_coin.fCoinBase));
p.pushKV("height", uint64_t(prev_coin.nHeight));
p.pushKV("value", ValueFromAmount(prev_txout.nValue));
p.pushKV("scriptPubKey", o_script_pub_key);
in.pushKV("prevout", p);
p.pushKV("scriptPubKey", std::move(o_script_pub_key));
in.pushKV("prevout", std::move(p));
}
}
in.pushKV("sequence", (int64_t)txin.nSequence);
vin.push_back(in);
vin.push_back(std::move(in));
}
entry.pushKV("vin", vin);
entry.pushKV("vin", std::move(vin));

UniValue vout(UniValue::VARR);
for (unsigned int i = 0; i < tx.vout.size(); i++) {
Expand All @@ -244,14 +244,14 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry

UniValue o(UniValue::VOBJ);
ScriptToUniv(txout.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
out.pushKV("scriptPubKey", o);
vout.push_back(out);
out.pushKV("scriptPubKey", std::move(o));
vout.push_back(std::move(out));

if (have_undo) {
amt_total_out += txout.nValue;
}
}
entry.pushKV("vout", vout);
entry.pushKV("vout", std::move(vout));

if (have_undo) {
const CAmount fee = amt_total_in - amt_total_out;
Expand Down
2 changes: 1 addition & 1 deletion src/net_types.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ UniValue BanMapToJson(const banmap_t& bans)
const auto& ban_entry = it.second;
UniValue j = ban_entry.ToJson();
j.pushKV(BANMAN_JSON_ADDR_KEY, address.ToString());
bans_json.push_back(j);
bans_json.push_back(std::move(j));
}
return bans_json;
}
Expand Down
6 changes: 3 additions & 3 deletions src/rest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -934,10 +934,10 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
// include the script in a json output
UniValue o(UniValue::VOBJ);
ScriptToUniv(coin.out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
utxo.pushKV("scriptPubKey", o);
utxos.push_back(utxo);
utxo.pushKV("scriptPubKey", std::move(o));
utxos.push_back(std::move(utxo));
}
objGetUTXOResponse.pushKV("utxos", utxos);
objGetUTXOResponse.pushKV("utxos", std::move(utxos));

// return json string
std::string strJSON = objGetUTXOResponse.write() + "\n";
Expand Down
24 changes: 12 additions & 12 deletions src/rpc/blockchain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1025,9 +1025,9 @@ static RPCHelpMan gettxoutsetinfo()
unspendables.pushKV("bip30", ValueFromAmount(stats.total_unspendables_bip30 - prev_stats.total_unspendables_bip30));
unspendables.pushKV("scripts", ValueFromAmount(stats.total_unspendables_scripts - prev_stats.total_unspendables_scripts));
unspendables.pushKV("unclaimed_rewards", ValueFromAmount(stats.total_unspendables_unclaimed_rewards - prev_stats.total_unspendables_unclaimed_rewards));
block_info.pushKV("unspendables", unspendables);
block_info.pushKV("unspendables", std::move(unspendables));

ret.pushKV("block_info", block_info);
ret.pushKV("block_info", std::move(block_info));
}
} else {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
Expand Down Expand Up @@ -1111,7 +1111,7 @@ static RPCHelpMan gettxout()
ret.pushKV("value", ValueFromAmount(coin.out.nValue));
UniValue o(UniValue::VOBJ);
ScriptToUniv(coin.out.scriptPubKey, /*out=*/o, /*include_hex=*/true, /*include_address=*/true);
ret.pushKV("scriptPubKey", o);
ret.pushKV("scriptPubKey", std::move(o));
ret.pushKV("coinbase", (bool)coin.fCoinBase);

return ret;
Expand Down Expand Up @@ -1161,7 +1161,7 @@ static void SoftForkDescPushBack(const CBlockIndex* blockindex, UniValue& softfo
// one below the activation height
rv.pushKV("active", DeploymentActiveAfter(blockindex, chainman, dep));
rv.pushKV("height", chainman.GetConsensus().DeploymentHeight(dep));
softforks.pushKV(DeploymentName(dep), rv);
softforks.pushKV(DeploymentName(dep), std::move(rv));
}

static void SoftForkDescPushBack(const CBlockIndex* blockindex, UniValue& softforks, const ChainstateManager& chainman, Consensus::DeploymentPos id)
Expand Down Expand Up @@ -1214,7 +1214,7 @@ static void SoftForkDescPushBack(const CBlockIndex* blockindex, UniValue& softfo
statsUV.pushKV("threshold", statsStruct.threshold);
statsUV.pushKV("possible", statsStruct.possible);
}
bip9.pushKV("statistics", statsUV);
bip9.pushKV("statistics", std::move(statsUV));

std::string sig;
sig.reserve(signals.size());
Expand All @@ -1230,9 +1230,9 @@ static void SoftForkDescPushBack(const CBlockIndex* blockindex, UniValue& softfo
rv.pushKV("height", chainman.m_versionbitscache.StateSinceHeight(blockindex, chainman.GetConsensus(), id));
}
rv.pushKV("active", ThresholdState::ACTIVE == next_state);
rv.pushKV("bip9", bip9);
rv.pushKV("bip9", std::move(bip9));

softforks.pushKV(DeploymentName(id), rv);
softforks.pushKV(DeploymentName(id), std::move(rv));
}

// used by rest.cpp:rest_chaininfo, so cannot be static
Expand Down Expand Up @@ -1498,7 +1498,7 @@ static RPCHelpMan getchaintips()
}
obj.pushKV("status", status);

res.push_back(obj);
res.push_back(std::move(obj));
}

return res;
Expand Down Expand Up @@ -1978,7 +1978,7 @@ static RPCHelpMan getblockstats()
ret_all.pushKV("avgfeerate", total_weight ? (totalfee * WITNESS_SCALE_FACTOR) / total_weight : 0); // Unit: sat/vbyte
ret_all.pushKV("avgtxsize", (block.vtx.size() > 1) ? total_size / (block.vtx.size() - 1) : 0);
ret_all.pushKV("blockhash", pindex.GetBlockHash().GetHex());
ret_all.pushKV("feerate_percentiles", feerates_res);
ret_all.pushKV("feerate_percentiles", std::move(feerates_res));
ret_all.pushKV("height", (int64_t)pindex.nHeight);
ret_all.pushKV("ins", inputs);
ret_all.pushKV("maxfee", maxfee);
Expand Down Expand Up @@ -2262,9 +2262,9 @@ static RPCHelpMan scantxoutset()
unspent.pushKV("coinbase", coin.IsCoinBase());
unspent.pushKV("height", (int32_t)coin.nHeight);

unspents.push_back(unspent);
unspents.push_back(std::move(unspent));
}
result.pushKV("unspents", unspents);
result.pushKV("unspents", std::move(unspents));
result.pushKV("total_amount", ValueFromAmount(total_in));
} else {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid action '%s'", action));
Expand Down Expand Up @@ -2504,7 +2504,7 @@ static RPCHelpMan scanblocks()

ret.pushKV("from_height", start_block_height);
ret.pushKV("to_height", start_index->nHeight); // start_index is always the last scanned block here
ret.pushKV("relevant_blocks", blocks);
ret.pushKV("relevant_blocks", std::move(blocks));
ret.pushKV("completed", completed);
}
else {
Expand Down
2 changes: 1 addition & 1 deletion src/rpc/client.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
// Use pushKVEnd instead of pushKV to avoid overwriting an explicit
// "args" value with an implicit one. Let the RPC server handle the
// request as given.
params.pushKVEnd("args", positional_args);
params.pushKVEnd("args", std::move(positional_args));
}

return params;
Expand Down
4 changes: 2 additions & 2 deletions src/rpc/external_signer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ static RPCHelpMan enumeratesigners()
UniValue signer_res = UniValue::VOBJ;
signer_res.pushKV("fingerprint", signer.m_fingerprint);
signer_res.pushKV("name", signer.m_name);
signers_res.push_back(signer_res);
signers_res.push_back(std::move(signer_res));
}
} catch (const std::exception& e) {
throw JSONRPCError(RPC_MISC_ERROR, e.what());
}
UniValue result(UniValue::VOBJ);
result.pushKV("signers", signers_res);
result.pushKV("signers", std::move(signers_res));
return result;
}
};
Expand Down
12 changes: 6 additions & 6 deletions src/rpc/fees.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ static RPCHelpMan estimatesmartfee()
result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK()));
} else {
errors.push_back("Insufficient data or no feerate found");
result.pushKV("errors", errors);
result.pushKV("errors", std::move(errors));
}
result.pushKV("blocks", feeCalc.returnedTarget);
return result;
Expand Down Expand Up @@ -198,18 +198,18 @@ static RPCHelpMan estimaterawfee()
horizon_result.pushKV("feerate", ValueFromAmount(feeRate.GetFeePerK()));
horizon_result.pushKV("decay", buckets.decay);
horizon_result.pushKV("scale", (int)buckets.scale);
horizon_result.pushKV("pass", passbucket);
horizon_result.pushKV("pass", std::move(passbucket));
// buckets.fail.start == -1 indicates that all buckets passed, there is no fail bucket to output
if (buckets.fail.start != -1) horizon_result.pushKV("fail", failbucket);
if (buckets.fail.start != -1) horizon_result.pushKV("fail", std::move(failbucket));
} else {
// Output only information that is still meaningful in the event of error
horizon_result.pushKV("decay", buckets.decay);
horizon_result.pushKV("scale", (int)buckets.scale);
horizon_result.pushKV("fail", failbucket);
horizon_result.pushKV("fail", std::move(failbucket));
errors.push_back("Insufficient data or no feerate found which meets threshold");
horizon_result.pushKV("errors", errors);
horizon_result.pushKV("errors", std::move(errors));
}
result.pushKV(StringForFeeEstimateHorizon(horizon), horizon_result);
result.pushKV(StringForFeeEstimateHorizon(horizon), std::move(horizon_result));
}
return result;
},
Expand Down
34 changes: 17 additions & 17 deletions src/rpc/mempool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ static RPCHelpMan testmempoolaccept()
auto it = package_result.m_tx_results.find(tx->GetWitnessHash());
if (exit_early || it == package_result.m_tx_results.end()) {
// Validation unfinished. Just return the txid and wtxid.
rpc_result.push_back(result_inner);
rpc_result.push_back(std::move(result_inner));
continue;
}
const auto& tx_result = it->second;
Expand All @@ -229,8 +229,8 @@ static RPCHelpMan testmempoolaccept()
for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) {
effective_includes_res.push_back(wtxid.ToString());
}
fees.pushKV("effective-includes", effective_includes_res);
result_inner.pushKV("fees", fees);
fees.pushKV("effective-includes", std::move(effective_includes_res));
result_inner.pushKV("fees", std::move(fees));
}
} else {
result_inner.pushKV("allowed", false);
Expand All @@ -241,7 +241,7 @@ static RPCHelpMan testmempoolaccept()
result_inner.pushKV("reject-reason", state.GetRejectReason());
}
}
rpc_result.push_back(result_inner);
rpc_result.push_back(std::move(result_inner));
}
return rpc_result;
},
Expand Down Expand Up @@ -295,7 +295,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
fees.pushKV("modified", ValueFromAmount(e.GetModifiedFee()));
fees.pushKV("ancestor", ValueFromAmount(e.GetModFeesWithAncestors()));
fees.pushKV("descendant", ValueFromAmount(e.GetModFeesWithDescendants()));
info.pushKV("fees", fees);
info.pushKV("fees", std::move(fees));

const CTransaction& tx = e.GetTx();
std::set<std::string> setDepends;
Expand All @@ -311,14 +311,14 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
depends.push_back(dep);
}

info.pushKV("depends", depends);
info.pushKV("depends", std::move(depends));

UniValue spent(UniValue::VARR);
for (const CTxMemPoolEntry& child : e.GetMemPoolChildrenConst()) {
spent.push_back(child.GetTx().GetHash().ToString());
}

info.pushKV("spentby", spent);
info.pushKV("spentby", std::move(spent));

// Add opt-in RBF status
bool rbfStatus = false;
Expand Down Expand Up @@ -347,7 +347,7 @@ UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempoo
// Mempool has unique entries so there is no advantage in using
// UniValue::pushKV, which checks if the key already exists in O(N).
// UniValue::pushKVEnd is used instead which currently is O(1).
o.pushKVEnd(e.GetTx().GetHash().ToString(), info);
o.pushKVEnd(e.GetTx().GetHash().ToString(), std::move(info));
}
return o;
} else {
Expand All @@ -364,7 +364,7 @@ UniValue MempoolToJSON(const CTxMemPool& pool, bool verbose, bool include_mempoo
return a;
} else {
UniValue o(UniValue::VOBJ);
o.pushKV("txids", a);
o.pushKV("txids", std::move(a));
o.pushKV("mempool_sequence", mempool_sequence);
return o;
}
Expand Down Expand Up @@ -474,7 +474,7 @@ static RPCHelpMan getmempoolancestors()
const uint256& _hash = e.GetTx().GetHash();
UniValue info(UniValue::VOBJ);
entryToJSON(mempool, info, e);
o.pushKV(_hash.ToString(), info);
o.pushKV(_hash.ToString(), std::move(info));
}
return o;
}
Expand Down Expand Up @@ -539,7 +539,7 @@ static RPCHelpMan getmempooldescendants()
const uint256& _hash = e.GetTx().GetHash();
UniValue info(UniValue::VOBJ);
entryToJSON(mempool, info, e);
o.pushKV(_hash.ToString(), info);
o.pushKV(_hash.ToString(), std::move(info));
}
return o;
}
Expand Down Expand Up @@ -653,7 +653,7 @@ static RPCHelpMan gettxspendingprevout()
o.pushKV("spendingtxid", spendingTx->GetHash().ToString());
}

result.push_back(o);
result.push_back(std::move(o));
}

return result;
Expand Down Expand Up @@ -992,20 +992,20 @@ static RPCHelpMan submitpackage()
for (const auto& wtxid : tx_result.m_wtxids_fee_calculations.value()) {
effective_includes_res.push_back(wtxid.ToString());
}
fees.pushKV("effective-includes", effective_includes_res);
fees.pushKV("effective-includes", std::move(effective_includes_res));
}
result_inner.pushKV("fees", fees);
result_inner.pushKV("fees", std::move(fees));
for (const auto& ptx : it->second.m_replaced_transactions) {
replaced_txids.insert(ptx->GetHash());
}
break;
}
tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), result_inner);
tx_result_map.pushKV(tx->GetWitnessHash().GetHex(), std::move(result_inner));
}
rpc_result.pushKV("tx-results", tx_result_map);
rpc_result.pushKV("tx-results", std::move(tx_result_map));
UniValue replaced_list(UniValue::VARR);
for (const uint256& hash : replaced_txids) replaced_list.push_back(hash.ToString());
rpc_result.pushKV("replaced-transactions", replaced_list);
rpc_result.pushKV("replaced-transactions", std::move(replaced_list));
return rpc_result;
},
};
Expand Down

0 comments on commit 6300438

Please sign in to comment.