rpc: faster JSON generation

martinus commented at 7:07 pm on January 24, 2021: contributor

This PR is not ready: I know univalue is handled in a separate repository, so I guess I have to split the PR up? Where should the univalue changes go to, into https://github.com/bitcoin-core/univalue or into https://github.com/jgarzik/univalue?

Anyways, here is some motivational data for the changes:

For my BitcoinUtxoVisualizer the limiting factor seems to be mostly bitcoind’s JSON generation speed. Here are some optimizations that speed this up quite a bit for me. Here are some numbers:

./bench_bitcoin -filter=BlockToJsonVerbose

ns/op	err%	bra/op	miss%	benchmarked commit
66,877,763.00	0.1%	101,638,376.00	0.4%	349f17e
57,915,056.00	1.2%	85,882,830.00	0.5%	26fd10a
36,585,334.00	1.6%	50,966,277.00	0.4%	93f8240
28,508,105.00	0.2%	40,138,962.00	0.4%	9b59b15

So the benchmark improves from 66.9ms to 28.5ms for me. I ran an a benchmark with bitcoind as well, with that protocol:

start bitcoind, with this bitcoin.conf:

0server=1
1rest=1
2rpcport=8332
3rpcthreads=32
4rpcworkqueue=64
5txindex=1
6dbcache=2000

Wait until fully synced, and log message shows loadblk thread exit, so that bitcoind is idle.

Run apache benchmark: 2000 requests, 12 parallel threads, fetching block nr. 600000:

0ab -n 2000 -c 12 "http://127.0.0.1:8332/rest/block/00000000000000000007316856900e76b4f7a9139cfbfba89842c8d196cd5f91.json"

Results for master, 349f17e:

 0    Concurrency Level:      12
 1    Time taken for tests:   26.535 seconds
 2    Complete requests:      2000
 3    Failed requests:        0
 4    Total transferred:      11192226000 bytes
 5    HTML transferred:       11192124000 bytes
 6    Requests per second:    75.37 [#/sec] (mean)
 7    Time per request:       159.208 [ms] (mean)
 8    Time per request:       13.267 [ms] (mean, across all concurrent requests)
 9    Transfer rate:          411911.64 [Kbytes/sec] received
10
11    Connection Times (ms)
12                min  mean[+/-sd] median   max
13    Connect:        0    0   0.0      0       0
14    Processing:    93  158  15.0    156     283
15    Waiting:       91  154  14.8    152     280
16    Total:         93  158  15.0    156     283
17
18    Percentage of the requests served within a certain time (ms)
19    50%    156
20    66%    161
21    75%    164
22    80%    167
23    90%    174
24    95%    184
25    98%    200
26    99%    216
27    100%    283 (longest request)

Results for this PR, 9b59b15:

 0    Concurrency Level:      12
 1    Time taken for tests:   16.801 seconds
 2    Complete requests:      2000
 3    Failed requests:        0
 4    Total transferred:      11192226000 bytes
 5    HTML transferred:       11192124000 bytes
 6    Requests per second:    119.04 [#/sec] (mean)
 7    Time per request:       100.805 [ms] (mean)
 8    Time per request:       8.400 [ms] (mean, across all concurrent requests)
 9    Transfer rate:          650556.74 [Kbytes/sec] received
10
11    Connection Times (ms)
12                min  mean[+/-sd] median   max
13    Connect:        0    0   0.0      0       0
14    Processing:    38  100  19.1     98     211
15    Waiting:       36   98  19.0     96     208
16    Total:         38  100  19.1     98     211
17
18    Percentage of the requests served within a certain time (ms)
19    50%     98
20    66%    105
21    75%    110
22    80%    113
23    90%    123
24    95%    138
25    98%    155
26    99%    163
27    100%    211 (longest request)

Fix BlockToJsonVerbose benchmark

Currently it was not possible to run just the BlockToJsonVerboes benchmarsk because it did not set up everything it needed, running `bench_bitcoin -filter=BlockToJsonVerbose` caused this assert to fail:

```
bench_bitcoin: chainparams.cpp:506: const CChainParams& Params(): Assertion `globalChainParams' failed.
```

Initializing TestingSetup fixes this.

349f17edc2

univalue optimizations: move semantics, reduce temporary strings

* Introducing move semantics vor pushKV and push_back. In many use cases
  the const& calls unnecessarily create a copy of the argument, when
  objects can be moved this does not happen.

* Reduce creation of temporary strings. `json_escape` now appends to
  an existing string instead of creating a temporary, and UniValue::write
  now uses an UniValue::append where possible.

* Use `std::to_string` instead of `std::ostringstream` for `setNumStr`

In a benchmark with Bitcoin's BlockToJsonVerbose, using the move methods
where possible speeds up JSON generation by about a factor of 2.

26fd10a931

make use of univalue's new move semantic

Prevent unnecessary copies of UniValue objects by using `std::move`
where possible.

This speeds up the `BlockToJsonVerbose` benchmark from 66.9 ms/op to
36.6 ms/op on an intel i7 locked to 3.2GHz.

|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | `BlockToJsonVerbose`
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|       66,877,763.00 |               14.95 |    0.1% |  549,960,313.00 |  211,619,093.00 |  2.599 | 101,638,376.00 |    0.4% |      0.75 | `master`
|       57,915,056.00 |               17.27 |    1.2% |  464,866,030.00 |  183,491,588.00 |  2.533 |  85,882,830.00 |    0.5% |      0.65 | `univalue: move semantics, reduce temporary strings`
|       36,585,334.00 |               27.33 |    1.6% |  310,260,492.00 |  116,584,209.00 |  2.661 |  50,966,277.00 |    0.4% |      0.41 | `make use of univalue's new move semantic`

Note that there are still places where `std::move` could be introduced.

93f82407f9

optimize HexStr: do not use std::string push_back

std::string's push_back can be very time consuming, even with
reserve(). If possible, this should be avoided. In the case of `HexStr`
this is easily possible, and since it is heavily used, the advantage for
`BlockToJsonVerbose` are significant:

|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | `BlockToJsonVerbose`
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|       36,585,334.00 |               27.33 |    1.6% |  310,260,492.00 |  116,584,209.00 |  2.661 |  50,966,277.00 |    0.4% |      0.41 | `make use of univalue's move semantic`
|       28,508,105.00 |               35.08 |    0.2% |  244,933,429.00 |   90,874,112.00 |  2.695 |  40,138,962.00 |    0.4% |      0.32 | `optimize HexStr`

9b59b15c12

laanwj added the label RPC/REST/ZMQ on Jan 24, 2021

laanwj added the label Upstream on Jan 24, 2021

laanwj commented at 7:21 pm on January 24, 2021: member

Nice improvement.

Where should the univalue changes go to, into https://github.com/bitcoin-core/univalue or into https://github.com/jgarzik/univalue?

I think it’s preferable if it can be merged “as upstream as possible” so that we don’t have to deviate from upstream unnecessarily. E.g. in jgarzik’s if possible. But if not, our univalue repository.

DrahtBot commented at 11:44 pm on January 24, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
#20286 (rpc: deprecate addresses and reqSigs from rpc outputs by mjdietzx)
#19438 (Introduce deploymentstatus by ajtowns)
#15448 (bitcoin-cli: Eliminate “Error parsing JSON” errors by ryanofsky)

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.

martinus commented at 6:51 am on January 25, 2021: contributor

I think it’s preferable if it can be merged “as upstream as possible”

Thanks! I will do that.

martinus closed this on Jan 25, 2021

laanwj commented at 5:44 pm on January 25, 2021: member

Thanks for filing it upstream. I hope it gets attention from the maintainer soon. If not, I’m happy to review it in our own fork.

martinus commented at 6:47 pm on January 25, 2021: contributor

I’ll wait a while for jgarzik to reply, but he might not like it because this will increase dependency from c++98 to c++11

laanwj commented at 10:03 pm on January 25, 2021: member

I’m not sure that’s a problem, but we’ll see.

DrahtBot locked this on Aug 16, 2022

rpc: faster JSON generation #20999

Conflicts