-
Notifications
You must be signed in to change notification settings - Fork 37.7k
rpc: faster JSON generation #20999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc: faster JSON generation #20999
Conversation
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`
Nice improvement.
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. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
Thanks! I will do that. |
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. |
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 |
I'm not sure that's a problem, but we'll see. |
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
So the benchmark improves from 66.9ms to 28.5ms for me. I ran an a benchmark with
bitcoind
as well, with that protocol:bitcoin.conf
:loadblk thread exit
, so that bitcoind is idle.Results for master, 349f17e:
Results for this PR, 9b59b15: