Page MenuHomePhabricator
Differential D4234

bench: Benchmark MempoolToJSON
ClosedPublic
Actions

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

Details

Reviewers
Fabien
deadalnix
Group Reviewers
Restricted Project
Commits
rSTAGINGe12d2c2449ee: bench: Benchmark MempoolToJSON
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
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

fpelliccioni created this revision.Oct 10 2019, 17:43
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 10 2019, 17:43
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Oct 10 2019, 17:43

Build Bitcoin-ABC / Diffs / Diff Testing started.

fpelliccioni edited the test plan for this revision. (Show Details)Oct 10 2019, 17:44
teamcity added a comment.Oct 10 2019, 17:45

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Harbormaster failed remote builds in B7736: Diff 13486!Oct 10 2019, 17:45
fpelliccioni updated this revision to Diff 13487.Oct 10 2019, 17:45

fix cmake script style.

teamcity added a comment.Oct 10 2019, 17:45

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster failed remote builds in B7737: Diff 13487!Oct 10 2019, 17:46
teamcity added a comment.Oct 10 2019, 17:46

Build Bitcoin-ABC / Diffs / Diff Testing failed.

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.

teamcity added a comment.Oct 11 2019, 13:09

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 11 2019, 13:22

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7746: Diff 13503.Oct 11 2019, 13:22
fpelliccioni added a child revision: D4243: rpc: Expose g_is_mempool_loaded via getmempoolinfo.Oct 11 2019, 21:09
fpelliccioni updated this revision to Diff 13507.Oct 11 2019, 21:12

fix squashing.

teamcity added a comment.Oct 11 2019, 21:12

Build Bitcoin-ABC / Diffs / Diff Testing started.

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

fix squashing

teamcity added a comment.Oct 11 2019, 21:14

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 11 2019, 21:26

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7748: Diff 13507.Oct 11 2019, 21:26
teamcity added a comment.Oct 11 2019, 21:28

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7749: Diff 13508.Oct 11 2019, 21:28
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

teamcity added a comment.Oct 15 2019, 13:47

Build Bitcoin-ABC / Diffs / Diff Testing started.

fpelliccioni added a comment.Oct 15 2019, 13:58
In D4234#102134, @deadalnix wrote:

It looks like you have dependencies to backport first.

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

teamcity added a comment.Oct 15 2019, 14:00

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7776: Diff 13561.Oct 15 2019, 14:00
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
fpelliccioni added a comment.Oct 17 2019, 17:46
In D4234#102664, @deadalnix wrote:

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.Oct 23 2019, 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.Oct 23 2019, 15:56

rebased from master

teamcity added a comment.Oct 23 2019, 15:56

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 23 2019, 16:10

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7845: Diff 13661.Oct 23 2019, 16:10
fpelliccioni updated this revision to Diff 13662.Oct 23 2019, 16:19

fixes wrong rebase

teamcity added a comment.Oct 23 2019, 16:19

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 23 2019, 16:32

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7846: Diff 13662.Oct 23 2019, 16:32
fpelliccioni edited the test plan for this revision. (Show Details)Oct 23 2019, 18:06
fpelliccioni updated this revision to Diff 13671.Oct 23 2019, 18:33

rebased

teamcity added a comment.Oct 23 2019, 18:33

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7854: Diff 13671.Oct 23 2019, 18:43
teamcity added a comment.Oct 23 2019, 18:43

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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
fpelliccioni updated this revision to Diff 13690.Oct 24 2019, 18:21

fixes forward declarations order.

teamcity added a comment.Oct 24 2019, 18:21

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7862: Diff 13690.Oct 24 2019, 18:34
teamcity added a comment.Oct 24 2019, 18:34

Build Bitcoin-ABC / Diffs / Diff Testing passed.

fpelliccioni mentioned this in D4243: rpc: Expose g_is_mempool_loaded via getmempoolinfo.Oct 24 2019, 18:39
deadalnix accepted this revision.Oct 25 2019, 00:40
Fabien accepted this revision.Oct 25 2019, 06:07
This revision is now accepted and ready to land.Oct 25 2019, 06:07
Closed by commit rABCe12d2c2449ee: bench: Benchmark MempoolToJSON (authored by MarcoFalke <falke.marco@gmail.com>, committed by fpelliccioni). · Explain WhyOct 25 2019, 13:56
This revision was automatically updated to reflect the committed changes.
fpelliccioni added a commit: rABCe12d2c2449ee: bench: Benchmark MempoolToJSON.
fpelliccioni added a commit: rSTAGINGe12d2c2449ee: bench: Benchmark MempoolToJSON.Apr 7 2020, 21:30

Revision Contents
Changeset List

PathSize
src/
5 lines
bench/
1 line
45 lines
4 lines
rpc/
5 lines
52 lines
4 lines
2 lines

Diff 13690

View Options

src/Makefile.bench.include

Loading...
View Options

src/bench/CMakeLists.txt

Loading...
View Options

src/bench/rpc_mempool.cpp

Loading...
View Options

src/rest.cpp

Loading...
View Options

src/rpc/blockchain.h

Loading...
View Options

src/rpc/blockchain.cpp

Loading...
View Options

src/txmempool.h

Loading...
View Options

src/txmempool.cpp

Loading...