Page MenuHomePhabricator

bench: Benchmark MempoolToJSON
ClosedPublic

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

Details

Reviewers
Fabien
deadalnix
Group Reviewers
Restricted Project
Commits
rABCe12d2c2449ee: bench: Benchmark MempoolToJSON
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fpelliccioni created this revision.Oct 10 2019, 17:43
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 10 2019, 17:43
fpelliccioni edited the test plan for this revision. (Show Details)Oct 10 2019, 17:44
fpelliccioni updated this revision to Diff 13487.Oct 10 2019, 17:45

fix cmake script style.

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
fpelliccioni updated this revision to Diff 13503.Oct 11 2019, 13:09

bench code fixed.

fpelliccioni updated this revision to Diff 13507.Oct 11 2019, 21:12

fix squashing.

fpelliccioni updated this revision to Diff 13508.Oct 11 2019, 21:14

fix squashing

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
fpelliccioni updated this revision to Diff 13561.Oct 15 2019, 13:47

fix rebase error

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>
fpelliccioni requested review of this revision.Oct 18 2019, 14:26
deadalnix accepted this revision.Wed, Oct 23, 01:41
deadalnix added inline comments.
src/rpc/blockchain.h
14 ↗(On Diff #13561)

sort in alphabetic order

fpelliccioni updated this revision to Diff 13661.Wed, Oct 23, 15:56

rebased from master

fpelliccioni updated this revision to Diff 13662.Wed, Oct 23, 16:19

fixes wrong rebase

fpelliccioni edited the test plan for this revision. (Show Details)Wed, Oct 23, 18:06
Fabien requested changes to this revision.Wed, Oct 23, 20:27

One nit, good otherwise.

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

Please sort.

This revision now requires changes to proceed.Wed, Oct 23, 20:27
fpelliccioni updated this revision to Diff 13690.Thu, Oct 24, 18:21

fixes forward declarations order.

deadalnix accepted this revision.Fri, Oct 25, 00:40
Fabien accepted this revision.Fri, Oct 25, 06:07
This revision is now accepted and ready to land.Fri, Oct 25, 06:07
This revision was automatically updated to reflect the committed changes.