Skip to content

Conversation

martinus
Copy link
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:

  1. start bitcoind, with this bitcoin.conf:
    server=1
    rest=1
    rpcport=8332
    rpcthreads=32
    rpcworkqueue=64
    txindex=1
    dbcache=2000
    
  2. Wait until fully synced, and log message shows loadblk thread exit, so that bitcoind is idle.
  3. 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)

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.
* 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.
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.
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`
@laanwj
Copy link
Member

laanwj commented Jan 24, 2021

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
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor Author

I think it's preferable if it can be merged "as upstream as possible"

Thanks! I will do that.

@laanwj
Copy link
Member

laanwj commented Jan 25, 2021

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
Copy link
Contributor Author

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
Copy link
Member

laanwj commented Jan 25, 2021

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

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants