RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function

ismaelsadeeq commented at 5:11 pm on October 29, 2024: member

This PR is motivated by #30495 (comment), It adds a reserve member function to UniValue and applies it within the blockToJSON function to pre-allocate memory, minimizing reallocation’s.

On master:

ns/op	op/s	err%	total	benchmark
190,342	5,254	2.3%	0.01	`BlockToJsonVerbose1`
34,812,292	28.73	1.0%	0.38	`BlockToJsonVerbose2`
34,457,167	29.02	1.0%	0.38	`BlockToJsonVerbose3`

On this PR:

ns/op	op/s	err%	total	benchmark
172,278	5,805	0.7%	0.01	`BlockToJsonVerbose1`
33,720,584	29.66	0.4%	0.37	`BlockToJsonVerbose2`
33,884,417	29.51	1.2%	0.38	`BlockToJsonVerbose3`

DrahtBot commented at 5:11 pm on October 29, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31179.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	andrewtoth
Approach ACK	l0rinc
Stale ACK	Eunovo

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:

#31583 (rpc: add gettarget , target getmininginfo field and show next block info by Sjors)

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.

DrahtBot added the label RPC/REST/ZMQ on Oct 29, 2024

andrewtoth commented at 5:27 pm on October 29, 2024: contributor

Concept ACK

andrewtoth commented at 7:13 pm on October 29, 2024: contributor

I think we can take this further and reserve for VOBJ types instead of just VARR. The VOBJ uses both values and keys, so we can reserve both. We can count how many times we do pushKV and reserve that amount for both keys and values.

kennet31526 approved

andrewtoth commented at 0:53 am on October 30, 2024: contributor

I believe you can get more fine grained benchmarks for this using the rpc_blockchain benchmarks.

maflcko commented at 6:38 am on October 30, 2024: member

In theory you could also add UniValue to VectorLikeClasses ( https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html#cmdoption-arg-VectorLikeClasses), but this is probably best done in a follow-up.

ismaelsadeeq force-pushed on Oct 30, 2024

ismaelsadeeq commented at 7:23 am on October 30, 2024: member

I think we can take this further and reserve for VOBJ types instead of just VARR. The VOBJ type uses both values and keys, so we can reserve for both.

Done in the latest push, see diff, but I am reserving keys only when typ is of VOBJ.

We can count how many times we use pushKV and reserve that amount for both keys and values.

+1, this approach is possible but is it maintainable? Manually counting each pushKVs means updating the value thats being passed to .reserve every time we add another element, will be better to reserve for size’s that are determined at runtime or at least use a constant.

Is there a constant for the number of items in a block so that I can reserve for results in blockheaderToJSON?

ismaelsadeeq commented at 7:32 am on October 30, 2024: member

I believe you can get more fine-grained benchmarks for this using the rpc_blockchain benchmarks.

Yes, I can, but I’ll need to add verbosity levels 1 and 2, where we can see some performance improvements.

 0diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
 1index 7e3e2d8e48a..6d431685e66 100644
 2--- a/src/bench/rpc_blockchain.cpp
 3+++ b/src/bench/rpc_blockchain.cpp
 4@@ -45,7 +45,25 @@ struct TestBlockAndIndex {
 5 
 6 } // namespace
 7 
 8-static void BlockToJsonVerbose(benchmark::Bench& bench)
 9+static void BlockToJsonVerbose1(benchmark::Bench& bench)
10+{
11+    TestBlockAndIndex data;
12+    bench.run([&] {
13+        auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_TXID);
14+        ankerl::nanobench::doNotOptimizeAway(univalue);
15+    });
16+}
17+
18+static void BlockToJsonVerbose2(benchmark::Bench& bench)
19+{
20+    TestBlockAndIndex data;
21+    bench.run([&] {
22+        auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS);
23+        ankerl::nanobench::doNotOptimizeAway(univalue);
24+    });
25+}
26+
27+static void BlockToJsonVerbose3(benchmark::Bench& bench)
28 {
29     TestBlockAndIndex data;
30     bench.run([&] {
31@@ -54,7 +72,9 @@ static void BlockToJsonVerbose(benchmark::Bench& bench)
32     });
33 }
34 
35-BENCHMARK(BlockToJsonVerbose, benchmark::PriorityLevel::HIGH);
36+BENCHMARK(BlockToJsonVerbose1, benchmark::PriorityLevel::HIGH);
37+BENCHMARK(BlockToJsonVerbose2, benchmark::PriorityLevel::HIGH);
38+BENCHMARK(BlockToJsonVerbose3, benchmark::PriorityLevel::HIGH);
39 
40 static void BlockToJsonVerboseWrite(benchmark::Bench& bench)
41 {

Benchmark Results

On master:

ns/op	op/s	err%	total	benchmark
190,342	5,254	2.3%	0.01	`BlockToJsonVerbose1`
34,812,292	28.73	1.0%	0.38	`BlockToJsonVerbose2`
34,457,167	29.02	1.0%	0.38	`BlockToJsonVerbose3`

On this PR:

ns/op	op/s	err%	total	benchmark
172,278	5,805	0.7%	0.01	`BlockToJsonVerbose1`
33,720,584	29.66	0.4%	0.37	`BlockToJsonVerbose2`
33,884,417	29.51	1.2%	0.38	`BlockToJsonVerbose3`

andrewtoth commented at 1:28 pm on October 30, 2024: contributor

this approach is possible but is it maintainable?

Hmm, yeah this would end up with reserve(<magic number>) littered throughout the json serialization. Not ideal.

I took a quick look through most of the block parsing, and many of the reservations we would do would be <5, so unlikely to be a big benefit.

The entry in TxToUniv could have up to 12 kv-pairs stored, and in blockheaderToJSON it could have 14. These would also be reserving two vectors so might have some visible benefit.

Also, the witness stack array inside TxToUniv is a VARR that could be reserved with a parameter resolved at runtime.

ismaelsadeeq force-pushed on Nov 1, 2024

ismaelsadeeq commented at 9:56 pm on November 1, 2024: member

Also, the witness stack array inside TxToUniv is a VARR that could be reserved with a parameter resolved at runtime.

Thanks, I’ve added this in the latest push.

Eunovo commented at 6:06 am on November 17, 2024: none

I believe you can get more fine-grained benchmarks for this using the rpc_blockchain benchmarks.

Yes, I can, but I’ll need to add verbosity levels 1 and 2, where we can see some performance improvements.

Show diff Benchmark Results

On master:

ns/op op/s err% total benchmark 190,342 5,254 2.3% 0.01 BlockToJsonVerbose1 34,812,292 28.73 1.0% 0.38 BlockToJsonVerbose2 34,457,167 29.02 1.0% 0.38 BlockToJsonVerbose3 On this PR:

ns/op op/s err% total benchmark 172,278 5,805 0.7% 0.01 BlockToJsonVerbose1 33,720,584 29.66 0.4% 0.37 BlockToJsonVerbose2 33,884,417 29.51 1.2% 0.38 BlockToJsonVerbose3 @ismaelsadeeq, could you commit the diff you’ve provided and include the benchmark results in the commit message? Then, rebase your current changes on top, rerun the benchmarks for https://github.com/bitcoin/bitcoin/pull/31179/commits/952463ccdc2baeacb527c66732fd184cbcbd53ba and https://github.com/bitcoin/bitcoin/pull/31179/commits/28e3392d11355b1160dc1a7a5557081728a02840, and update their commit messages with the results.

This way I can just go through your commits, run the benchmarks and compare with your results.

l0rinc commented at 8:37 pm on December 27, 2024: contributor

@ismaelsadeeq, I ran your benchmarks from #31179 (comment), but I only see a 7% speedup in the TxVerbosity::SHOW_TXID bench - while the rest were exactly the same as before. But if it’s that important, we could easily get a 30% speedup as well there by deduplicating the GetSerializeSize calls:

0static inline int64_t GetBlockWeight(const size_t size_no_witness, const size_t size_with_witness)
1{
2    return size_no_witness * (WITNESS_SCALE_FACTOR - 1) + size_with_witness;
3}
4static inline int64_t GetBlockWeight(const CBlock& block)
5{
6    const size_t size_no_witness{GetSerializeSize(TX_NO_WITNESS(block))};
7    const size_t size_with_witness{GetSerializeSize(TX_WITH_WITNESS(block))};
8    return GetBlockWeight(size_no_witness, size_with_witness);
9}

…

 0UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIndex& tip, const CBlockIndex& blockindex, TxVerbosity verbosity)
 1{
 2    UniValue result = blockheaderToJSON(tip, blockindex);
 3
 4    size_t size_no_witness{GetSerializeSize(TX_NO_WITNESS(block))};
 5    size_t size_with_witness{GetSerializeSize(TX_WITH_WITNESS(block))};
 6    result.pushKV("strippedsize", (int)size_no_witness);
 7    result.pushKV("size", (int)size_with_witness);
 8    result.pushKV("weight", (int)GetBlockWeight(size_no_witness, size_with_witness));
 9    UniValue txs(UniValue::VARR);
10    txs.reserve(block.vtx.size());
11...

Or if you need TxVerbosity::SHOW_DETAILS and TxVerbosity::SHOW_DETAILS_AND_PREVOUT, they can be sped up by ~20% by reviving the abandoned (for lack of interest) #29473 (edited link).

And if BlockToJsonVerboseWrite needs a boost, #31144 will speed up by ~14%.

l0rinc commented at 12:59 pm on December 30, 2024: contributor

@ismaelsadeeq #20999 by @martinus also contains a few optimization attempts (a lot of them were already applied in other PRs since, but a few of the changes can still be done). If RPC speed is important to you, the above suggestions can result in a measurable improvements.

ismaelsadeeq force-pushed on Dec 30, 2024

ismaelsadeeq commented at 10:15 pm on December 30, 2024: member

Thanks for taking a look @Eunovo @l0rinc

could you commit the diff you’ve provided and include the benchmark results in the commit message?

I’ve added 8d4823b383be4f8cd0152176a4f67b447d29549 For the benchmarks and instead update the PR title with the bench results.

I ran your benchmarks from #31179 (comment), but I only see a 7% speedup in the TxVerbosity::SHOW_TXID bench - while the rest were exactly the same as before.

I did that locally and the result is the same as well

But if it’s that important, we could easily get a 30% speedup as well there by deduplicating the GetSerializeSize calls:

I’ll be happy to review that as a PR or might attempt it myself.

Or if you need TxVerbosity::SHOW_DETAILS and TxVerbosity::SHOW_DETAILS_AND_PREVOUT, they can be sped up by ~20% by reviving the abandoned (for lack of interest) #30035.

Interesting I will take a look at that, it seems there are some unaddressed comments by @TheCharlatan at that PR

And if BlockToJsonVerboseWrite needs a boost, #31144 will speed up by ~14%.

I will take a look.

@martinus also contains a few optimization attempts (a lot of them were already applied in other PRs since, but a few of the changes can still be done). If RPC speed is important to you, the above suggestions can result in a measurable improvements.

RPC speed is important not just to me but to all users. For now, I’ve been experimenting with using libbitcoinkernel, and it seems to be more performant than the RPC.

I’ll look into it further.

All those PRs offer improvements that are beneficial to users. I also think having this reservation ability is important to avoid the reallocation overhead. @l0rinc?

l0rinc commented at 10:45 pm on December 30, 2024: contributor

I can implement my suggestions in separate PRs if you say the RPC speedups would be welcome

in src/bench/rpc_blockchain.cpp:48 in f1ec37cc3c outdated

44@@ -45,16 +45,33 @@ struct TestBlockAndIndex {
45 
46 } // namespace
47 
48-static void BlockToJsonVerbose(benchmark::Bench& bench)
49+static void BlockToJson(benchmark::Bench& bench, TxVerbosity verbosity_level)

l0rinc commented at 12:57 pm on December 31, 2024:

:+1: for extracting the common part

nit: in blockToJSON this is simply called verbosity

ismaelsadeeq commented at 2:58 pm on January 2, 2025:

Done, thanks

in src/core_write.cpp:210 in f1ec37cc3c outdated

203@@ -203,6 +204,7 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
204         }
205         if (!tx.vin[i].scriptWitness.IsNull()) {
206             UniValue txinwitness(UniValue::VARR);
207+            txinwitness.reserve(tx.vin[i].scriptWitness.stack.size());
208             for (const auto& item : tx.vin[i].scriptWitness.stack) {
209                 txinwitness.push_back(HexStr(item));
210             }

l0rinc commented at 1:44 pm on December 31, 2024:

we’re repeating the same request 3 times, might as well extract it:

0        if (auto witness{tx.vin[i].scriptWitness}; !witness.IsNull()) {
1            UniValue txinwitness(UniValue::VARR);
2            txinwitness.reserve(witness.stack.size());
3            for (const auto& item : witness.stack) {
4                txinwitness.push_back(HexStr(item));
5            }
6            in.pushKV("txinwitness", std::move(txinwitness));
7        }

ismaelsadeeq commented at 2:58 pm on January 2, 2025:

good idea, pushed. Thanks

l0rinc commented at 3:24 pm on December 31, 2024: contributor

Approach ACK There are a LOT of other places where we will be able to use this - we should do them in follow-up PRs

bench: support benching all verbosity of `BlockToJson` 44363bb8d0

UniValue: add reserve member function

- Only reserve keys when the typ is of `VOBJ`.

1749aef52a

in src/bench/rpc_blockchain.cpp:57 in f1ec37cc3c outdated

55         ankerl::nanobench::doNotOptimizeAway(univalue);
56     });
57 }
58 
59-BENCHMARK(BlockToJsonVerbose, benchmark::PriorityLevel::HIGH);
60+static void BlockToJsonVerbose1(benchmark::Bench& bench)

l0rinc commented at 3:28 pm on December 31, 2024:

the real RPC also reads the block from disk, we could bench that as well - instead of assuming it’s already in memory (this will also enable additional optimization opportunities).

Edit: note that this will reduce the difference between before/after state a lot, but the measurements will reflect a real call more.

ismaelsadeeq commented at 3:01 pm on January 2, 2025:

I think this is benching just blockToJSON . But it’s a good idea to write a bench for ReadRawBlockFromDisk as well.

I will leave that as an idea for a follow-up PR.

l0rinc commented at 5:15 pm on January 2, 2025:

I will leave that as an idea for a follow-up PR.

The problem is that the goal of this PR was to speed up the RPC. But if we benchmark that instead of just a subset of the task, i.e.:

 0diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
 1--- a/src/bench/rpc_blockchain.cpp	(revision bf5c569898d0297de010102a623bf52009607ed8)
 2+++ b/src/bench/rpc_blockchain.cpp	(date 1735836623994)
 3@@ -24,7 +24,6 @@
 4 namespace {
 5 
 6 struct TestBlockAndIndex {
 7-    const std::unique_ptr<const TestingSetup> testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
 8     CBlock block{};
 9     uint256 blockHash{};
10     CBlockIndex blockindex{};
11@@ -47,9 +46,10 @@
12 
13 static void BlockToJson(benchmark::Bench& bench, TxVerbosity verbosity)
14 {
15-    TestBlockAndIndex data;
16+    const std::unique_ptr<const TestingSetup> testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
17     bench.run([&] {
18-        auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, verbosity);
19+        TestBlockAndIndex data{};
20+        auto univalue = blockToJSON(testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, verbosity);
21         ankerl::nanobench::doNotOptimizeAway(univalue);
22     });
23 }
24@@ -75,8 +75,9 @@
25 
26 static void BlockToJsonVerboseWrite(benchmark::Bench& bench)
27 {
28+    const std::unique_ptr<const TestingSetup> testing_setup{MakeNoLogFileContext<const TestingSetup>(ChainType::MAIN)};
29     TestBlockAndIndex data;
30-    auto univalue = blockToJSON(data.testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT);
31+    auto univalue = blockToJSON(testing_setup->m_node.chainman->m_blockman, data.block, data.blockindex, data.blockindex, TxVerbosity::SHOW_DETAILS_AND_PREVOUT);
32     bench.run([&] {
33         auto str = univalue.write();
34         ankerl::nanobench::doNotOptimizeAway(str);

via

build/src/bench/bench_bitcoin -filter=‘BlockToJsonVerbosity.*’ -min-time=10000

unfortunately it reveals:

Before:

ns/op	op/s	err%	total	benchmark
1,317,881.88	758.79	0.3%	10.99	`BlockToJsonVerbosity1`
26,534,215.46	37.69	0.3%	10.95	`BlockToJsonVerbosity2`
26,486,601.35	37.75	0.1%	10.95	`BlockToJsonVerbosity3`

After:

ns/op	op/s	err%	total	benchmark
1,313,779.11	761.16	0.6%	11.00	`BlockToJsonVerbosity1`
26,128,170.14	38.27	0.2%	11.02	`BlockToJsonVerbosity2`
26,143,108.35	38.25	0.2%	11.03	`BlockToJsonVerbosity3`

i.e. barely any speedup for the RPC.

ismaelsadeeq commented at 2:05 pm on January 8, 2025:

The problem is that the goal of this PR was to speed up the RPC.

I’ve updated the description and the title to limit the usage to just blockToJSON

l0rinc commented at 4:26 pm on January 8, 2025:

My concern isn’t with the description (I think reserving space can be a valuable change), but that it doesn’t really help the user calling this RPC, right?

ismaelsadeeq commented at 4:34 pm on January 8, 2025:

Yes end to end bench it’s not significant. I see it locally as well.

l0rinc commented at 4:36 pm on January 8, 2025:

I would be ok with it if we make the benchmarks representative - or not claim that this improved the RPC (I already have the follow-up PR locally using reserve for all other cases, I just want to be honest about it not being a measurable optimization)

ismaelsadeeq commented at 6:20 pm on January 8, 2025:

Ahh I see, Initially I was under the impression that it did, but I was wrong hence I updated the title and description

ismaelsadeeq force-pushed on Jan 2, 2025

Eunovo approved

Eunovo commented at 10:14 am on January 6, 2025: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/31179/commits/bf5c569898d0297de010102a623bf52009607ed8

Confirmed slight improvement in BlockToJson bench. Before:

0|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
2|          438,695.00 |            2,279.49 |    1.3% |    3,818,825.00 |    1,495,260.00 |  2.554 |     682,779.00 |    0.8% |      0.01 | `BlockToJsonVerbosity1`
3|       45,518,930.00 |               21.97 |    0.3% |  354,868,701.00 |  154,939,608.00 |  2.290 |  60,169,811.00 |    1.5% |      0.51 | `BlockToJsonVerbosity2`
4|       45,463,793.00 |               22.00 |    0.2% |  354,866,371.00 |  154,811,098.00 |  2.292 |  60,168,983.00 |    1.4% |      0.51 | `BlockToJsonVerbosity3`

After:

0|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
1|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
2|          394,595.00 |            2,534.24 |    0.2% |    3,649,536.00 |    1,344,645.33 |  2.714 |     661,067.00 |    0.7% |      0.01 | `BlockToJsonVerbosity1`
3|       44,967,386.00 |               22.24 |    0.1% |  353,693,003.00 |  153,066,486.00 |  2.311 |  59,949,244.00 |    1.4% |      0.51 | `BlockToJsonVerbosity2`
4|       44,831,936.00 |               22.31 |    0.5% |  353,691,496.00 |  152,611,234.00 |  2.318 |  59,948,519.00 |    1.4% |      0.51 | `BlockToJsonVerbosity3`

DrahtBot requested review from l0rinc on Jan 6, 2025

DrahtBot requested review from andrewtoth on Jan 6, 2025

ismaelsadeeq renamed this:
~~RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC~~
RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function
on Jan 8, 2025

in src/core_write.cpp:205 in bf5c569898 outdated

201@@ -201,9 +202,10 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
202             o.pushKV("hex", HexStr(txin.scriptSig));
203             in.pushKV("scriptSig", std::move(o));
204         }
205-        if (!tx.vin[i].scriptWitness.IsNull()) {
206+        if (auto witness{tx.vin[i].scriptWitness}; !witness.IsNull()) {

maflcko commented at 3:50 pm on January 8, 2025:

This creates a copy of the witness stack. Previously it did not, so it seems like a pessimisation.

ismaelsadeeq commented at 6:26 pm on January 8, 2025:

Fixed in https://github.com/bitcoin/bitcoin/compare/bf5c569898d0297de010102a623bf52009607ed8..ea62aaed3b1851605c9ff33a9d9f9edcb53828a5

maflcko commented at 3:53 pm on January 8, 2025: member

Is this worth it, when the real end-to-end speedup is less than a percent, according to #31179 (review)?

Edit: I see you had an end-to-end benchmark in the initial description, but it was replaced by the micro-benchmark. I guess the block in the micro-benchmark is too small to make a noticeable difference?

ismaelsadeeq commented at 4:20 pm on January 8, 2025: member

Is this worth it, when the real end-to-end speedup is less than a percent, according to #31179 (review)?

I think it is useful to have 1749aef52af8ea5f436e5b26dc7281fce73d1436 We can use it in places like bf5c569898d0297de010102a623bf52009607ed8 and maybe more like @l0rinc mentioned #31179#pullrequestreview-2526455183.

Edit: I see you had an end-to-end benchmark in the initial description, but it was replaced by the micro-benchmark. I guess the block in the micro-benchmark is too small to make a noticeable difference?

Yeah the first run of the end to end benchmark shows a noticeable difference but then subsequent runs were very slight, so the performance increase due to the reallocation is not much, hence I limit the scope of the change to just adding the reservation feature and using it in blockToJSON.

rpc: reserve space for `UniValue` variables in `blockToJSON`

- Reserving space avoid reallocation, this provide noticeable
  performance increase in verbosity 1.

ea62aaed3b

ismaelsadeeq force-pushed on Jan 8, 2025

RPC: Add reserve member function to UniValue and use it in blockToJSON function #31179

Code Coverage & Benchmarks

Reviews

Conflicts

RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function #31179