rpc: faster JSON generation #20999

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:

server=1
rest=1
rpcport=8332
rpcthreads=32
rpcworkqueue=64
txindex=1
dbcache=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:

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

Results for master, 349f17e:

    Concurrency Level:      12
    Time taken for tests:   26.535 seconds
    Complete requests:      2000
    Failed requests:        0
    Total transferred:      11192226000 bytes
    HTML transferred:       11192124000 bytes
    Requests per second:    75.37 [#/sec] (mean)
    Time per request:       159.208 [ms] (mean)
    Time per request:       13.267 [ms] (mean, across all concurrent requests)
    Transfer rate:          411911.64 [Kbytes/sec] received

    Connection Times (ms)
                min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:    93  158  15.0    156     283
    Waiting:       91  154  14.8    152     280
    Total:         93  158  15.0    156     283

    Percentage of the requests served within a certain time (ms)
    50%    156
    66%    161
    75%    164
    80%    167
    90%    174
    95%    184
    98%    200
    99%    216
    100%    283 (longest request)

Results for this PR, 9b59b15:

    Concurrency Level:      12
    Time taken for tests:   16.801 seconds
    Complete requests:      2000
    Failed requests:        0
    Total transferred:      11192226000 bytes
    HTML transferred:       11192124000 bytes
    Requests per second:    119.04 [#/sec] (mean)
    Time per request:       100.805 [ms] (mean)
    Time per request:       8.400 [ms] (mean, across all concurrent requests)
    Transfer rate:          650556.74 [Kbytes/sec] received

    Connection Times (ms)
                min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:    38  100  19.1     98     211
    Waiting:       36   98  19.0     96     208
    Total:         38  100  19.1     98     211

    Percentage of the requests served within a certain time (ms)
    50%     98
    66%    105
    75%    110
    80%    113
    90%    123
    95%    138
    98%    155
    99%    163
    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