Page MenuHomePhabricator

bench: Benchmark MempoolToJSON
ClosedPublic

Authored by fpelliccioni on Oct 10 2019, 17:43.

Details

Summary

This is used in production (e.g. https://jochen-hoenicke.de/queue/#0,24h), so add a benchmark to avoid making it even slower.

Backport of Bitcoin Core PR15473
https://github.com/bitcoin/bitcoin/pull/15473

Test Plan
  1. Build with Clang in Debug mode:
CXX=clang++ CC=clang cmake .. -D CMAKE_CXX_FLAGS="-Werror=thread-safety-analysis" -GNinja -DCMAKE_BUILD_TYPE=Debug
ninja check-all
  1. Verify that the compiler has not emitted a thread-safety warning.
  2. Run the node: ./src/bitcoind -regtest
  3. Verify that text similar to "Assertion failed: lock ... not held ..." is not printed on stderr.
  1. Run benchmarks:
cmake .. -GNinja
ninja bench-bitcoin
./src/bench/bitcoin-bench

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D4234
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7862
Build 13743: Bitcoin ABC Buildbot (legacy)
Build 13742: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Oct 10 2019, 22:11

The test is failing.

This revision now requires changes to proceed.Oct 10 2019, 22:11
deadalnix requested changes to this revision.Oct 12 2019, 13:04

It looks like you have dependencies to backport first.

doc/REST-interface.md
87 ↗(On Diff #13508)

This is not part of the original patch. Is that something we missed somewhere else?

src/bench/rpc_mempool.cpp
18 ↗(On Diff #13508)

There is something missing here. This API is redundant, the txid is already retrievable from the tx in the CTxMemPoolEntry. This is not required int he original PR, so there is some dependency missing.

35 ↗(On Diff #13508)

This isn't what the original PR is doing.

37 ↗(On Diff #13508)

dito

src/init.cpp
240 ↗(On Diff #13508)

This is not part of the original PR. Once again, this hints at a missing dependency.

src/rpc/blockchain.cpp
1490 ↗(On Diff #13508)

This is not part of the original PR.

1515 ↗(On Diff #13508)

dito

src/rpc/blockchain.h
14 ↗(On Diff #13508)

That ordering choice is puzzling.

This revision now requires changes to proceed.Oct 12 2019, 13:04

It looks like you have dependencies to backport first.

I messed up with a dependent (D4243) Diff.
It is fixed right now. Sorry

deadalnix requested changes to this revision.Oct 17 2019, 01:28

It's not clear you go the dependency in the right order.in any case, it seems that you have https://github.com/bitcoin/bitcoin/pull/11062 to go through first.

This revision now requires changes to proceed.Oct 17 2019, 01:28

It's not clear you go the dependency in the right order.in any case, it seems that you have https://github.com/bitcoin/bitcoin/pull/11062 to go through first.

I ordered the dependencies based on the date of the log:

12aa2ac98 - Merge #15323: rpc: Expose g_is_mempool_loaded via getmempoolinfo (2019-05-01 ~ 5 months ago) <MarcoFalke>
3515612e0 - Merge #15473: bench: Benchmark MempoolToJSON (2019-03-06 ~ 7 months ago) <MarcoFalke>
deadalnix added inline comments.
src/rpc/blockchain.h
14 ↗(On Diff #13561)

sort in alphabetic order

Fabien requested changes to this revision.Oct 23 2019, 20:27

One nit, good otherwise.

src/rpc/blockchain.h
14 ↗(On Diff #13671)

Please sort.

This revision now requires changes to proceed.Oct 23 2019, 20:27

fixes forward declarations order.

This revision is now accepted and ready to land.Oct 25 2019, 06:07
This revision was automatically updated to reflect the committed changes.