Add microbenchmarks to profile more code paths. #8873

ryanofsky · 2016-10-03T19:22:49Z

The new benchmarks exercise script validation, CCoinsDBView caching,
mempool eviction, and wallet coin selection code.

All of the benchmarks added here are extremely simple and don't
necessarily mirror common real world conditions or interesting
performance edge cases. Details about how specific benchmarks can be
improved are noted in comments.

Github-Issue: #7883

fanquake · 2016-10-04T03:20:28Z

Trying to compile this on OS X:

Making all in src
  CXXLD    bench/bench_bitcoin
ld: unknown option: --start-group
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [bench/bench_bitcoin] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1

fanquake · 2016-10-04T03:26:50Z

After removing --start-group I see these numbers for the new benchmarks:

CCoinsCaching,14336,0.000036257319152,0.000077610835433,0.000076690073391
CCoinsCaching,14336,0.000038111116737,0.000081880833022,0.000077795902533

MempoolEviction,3072,0.000186952762306,0.000422742217779,0.000384807276229
MempoolEviction,3072,0.000204847194254,0.000402882695198,0.000387265269334

VerifyScriptBench,3584,0.000147988088429,0.000305769499391,0.000297257809767
VerifyScriptBench,3584,0.000145800411701,0.000301757827401,0.000298310802983

paveljanik · 2016-10-04T05:20:44Z

src/Makefile.bench.include

 bench_bench_bitcoin_LDADD += $(LIBBITCOIN_WALLET)
 endif

 bench_bench_bitcoin_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
-bench_bench_bitcoin_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+bench_bench_bitcoin_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) -Wl,--start-group


Do we use some circular dependencies? Or why adding linker --start-group?

He probably added it because of a weird crypter link issue:

libbitcoin_wallet.a(libbitcoin_wallet_a-crypter.o): In function `CCrypter::Encrypt(std::vector<unsigned char, secure_allocator<unsigned char> > const&, std::vector<unsigned char, std::allocator<unsigned char> >&) const': /.../bitcoin/src/wallet/crypter.cpp:85: undefined reference to `AES256CBCEncrypt::AES256CBCEncrypt(unsigned char const*, unsigned char const*, bool)' /.../bitcoin/src/wallet/crypter.cpp:86: undefined reference to `AES256CBCEncrypt::Encrypt(unsigned char const*, int, unsigned char*) const'

Rather we should structure the libraries sanely than resort to linker groups, though.

Yes, that's the issue I saw. I did try briefly to rearrange the link order and add dup library listings to work around the apparently circular dependencies, but for some reason nothing I tried worked so I resorted to --start-group. I can try again to figure find a working link order that doesn't require the flag, though.

At least one circular dependency exists between the wallet and server libs:

./src/libbitcoin_server.a:libbitcoin_server_a-main.o:000000000000c6c0 T CheckFinalTx(CTransaction const&, int) ./src/libbitcoin_wallet.a:libbitcoin_wallet_a-wallet.o: U CheckFinalTx(CTransaction const&, int) ./src/libbitcoin_server.a:libbitcoin_server_a-init.o: U RegisterWalletRPCCommands(CRPCTable&) ./src/libbitcoin_wallet.a:libbitcoin_wallet_a-rpcwallet.o:0000000000000370 T RegisterWalletRPCCommands(CRPCTable&)

But this problem only manifests if you remove ccoins_caching.cpp, mempool_eviction.cpp, verify_script.cpp from the sources list (which is what I had done during development).

Otherwise, adding the crypo lib after the wallet lib as Wladimir suggested seems to be sufficient to build, so I updated the PR with this change and removed start-group.

laanwj · 2016-10-04T09:12:22Z

Awesome! Thanks for doing this.
Concept ACK.

sipa · 2016-10-04T12:46:57Z

I can't easily look at the source code now. What exactly causes the circular dependency?

laanwj · 2016-10-04T14:59:51Z

I think the problem is not a circular dependency here, but that LIBBITCOIN_WALLET is added after LIBBITCOIN_CRYPTO, whereas wallet needs crypto so it should go before that.

fanquake · 2016-10-09T10:20:03Z

Compiling has been fixed on OS X.

CCoinsCaching,10240,0.000053781084716,0.000108781736344,0.000108645693399
CCoinsCaching,10240,0.000053551048040,0.000109656713903,0.000109049887396
CCoinsCaching,10240,0.000053836032748,0.000108591280878,0.000108507415280

MempoolEviction,1536,0.000360749661922,0.000733753666282,0.000728020289292
MempoolEviction,1536,0.000362284481525,0.000727366656065,0.000728628287713
MempoolEviction,1536,0.000364426523447,0.000764504075050,0.000734255183488

VerifyScriptBench,1536,0.000325001776218,0.000652246177197,0.000652723324796
VerifyScriptBench,1536,0.000325386412442,0.000652894377708,0.000653158252438
VerifyScriptBench,1536,0.000325437635183,0.000652890652418,0.000653182932486

paveljanik · 2016-10-09T10:28:24Z

src/bench/ccoins_caching.cpp

+// (https://github.com/bitcoin/bitcoin/issues/7883#issuecomment-224807484)
+static void CCoinsCaching(benchmark::State& state)
+{
+  CBasicKeyStore keystore;


Please clang-format the new code.

paveljanik · 2016-10-09T10:30:33Z

Concept ACK

The new benchmarks exercise script validation, CCoinsDBView caching, mempool eviction, and wallet coin selection code. All of the benchmarks added here are extremely simple and don't necessarily mirror common real world conditions or interesting performance edge cases. Details about how specific benchmarks can be improved are noted in comments. Github-Issue: bitcoin#7883

laanwj · 2016-10-18T20:02:46Z

Applied clang-format to all the new files.

18dacf9 Add microbenchmarks to profile more code paths. (Russell Yanofsky)

…ple output b21244e Updating benchmarkmarking.md with an updated sample output and help options (Jeff Rade) Pull request description: This PR is just a documentation update for someone (or myself) that looks into finishing up #7883 in the future. Looked through #7883 and appears [ryanofsky's PR](#8873) setup the benchmarks, but there are `FIXME` comments to pull in data from `test/` to get a larger data set (assuming reason why 7883 is still open). Tree-SHA512: d758efc659c75f2b3ceb376f5a466c4234354077e4671ac3eb901c082c4e519ce5ff592cea4742711050b4ce56a1b65ef69433dd74e7db3eb11a8567d517d9e2

…ple output b21244e0be Updating benchmarkmarking.md with an updated sample output and help options (Jeff Rade) Pull request description: This PR is just a documentation update for someone (or myself) that looks into finishing up #7883 in the future. Looked through #7883 and appears [ryanofsky's PR](bitcoin/bitcoin#8873) setup the benchmarks, but there are `FIXME` comments to pull in data from `test/` to get a larger data set (assuming reason why 7883 is still open). Tree-SHA512: d758efc659c75f2b3ceb376f5a466c4234354077e4671ac3eb901c082c4e519ce5ff592cea4742711050b4ce56a1b65ef69433dd74e7db3eb11a8567d517d9e2

18dacf9 Add microbenchmarks to profile more code paths. (Russell Yanofsky)

…ted sample output b21244e Updating benchmarkmarking.md with an updated sample output and help options (Jeff Rade) Pull request description: This PR is just a documentation update for someone (or myself) that looks into finishing up bitcoin#7883 in the future. Looked through bitcoin#7883 and appears [ryanofsky's PR](bitcoin#8873) setup the benchmarks, but there are `FIXME` comments to pull in data from `test/` to get a larger data set (assuming reason why 7883 is still open). Tree-SHA512: d758efc659c75f2b3ceb376f5a466c4234354077e4671ac3eb901c082c4e519ce5ff592cea4742711050b4ce56a1b65ef69433dd74e7db3eb11a8567d517d9e2

…sample output Summary: b21244e Updating benchmarkmarking.md with an updated sample output and help options (Jeff Rade) Pull request description: This PR is just a documentation update for someone (or myself) that looks into finishing up #7883 in the future. Looked through #7883 and appears [ryanofsky's PR](bitcoin/bitcoin#8873) setup the benchmarks, but there are `FIXME` comments to pull in data from `test/` to get a larger data set (assuming reason why 7883 is still open). --- Backport of Core [[bitcoin/bitcoin#12187 | PR12187]] Test Plan: read it Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8205

ryanofsky force-pushed the issue-7883-benchmarks branch from be1bd69 to 4568486 Compare October 3, 2016 20:04

fanquake added the Tests label Oct 3, 2016

paveljanik reviewed Oct 4, 2016

View reviewed changes

ryanofsky force-pushed the issue-7883-benchmarks branch from 4568486 to c9086ed Compare October 4, 2016 20:18

paveljanik reviewed Oct 9, 2016

View reviewed changes

laanwj force-pushed the issue-7883-benchmarks branch from c9086ed to 18dacf9 Compare October 18, 2016 20:00

laanwj merged commit 18dacf9 into bitcoin:master Oct 18, 2016

laanwj added a commit that referenced this pull request Oct 18, 2016

Merge #8873: Add microbenchmarks to profile more code paths.

74dc388

18dacf9 Add microbenchmarks to profile more code paths. (Russell Yanofsky)

This was referenced Oct 18, 2016

Add benchmarks to bench_bitcoin #7883

Closed

Locked memory manager #8753

Merged

ryanofsky mentioned this pull request May 26, 2017

[PoC] Add wallet inspection and modification tool "bitcoin-wallet-tool" #8745

Closed

codablock pushed a commit to codablock/dash that referenced this pull request Jan 12, 2018

Merge bitcoin#8873: Add microbenchmarks to profile more code paths.

2f86773

18dacf9 Add microbenchmarks to profile more code paths. (Russell Yanofsky)

jeffrade mentioned this pull request Jan 15, 2018

[Docs] Updating benchmarkmarking.md with an updated sample output #12187

Merged

dagurval mentioned this pull request Apr 30, 2018

Benchmark cherries bitcoinxt/bitcoinxt#408

Merged

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#8873: Add microbenchmarks to profile more code paths.

9e8dfe7

18dacf9 Add microbenchmarks to profile more code paths. (Russell Yanofsky)

PastaPastaPasta mentioned this pull request Jul 18, 2019

Backports 0.15 pr21 dashpay/dash#3029

Merged

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 18, 2019

Merge bitcoin#8873: Add microbenchmarks to profile more code paths.

69cc5d3

18dacf9 Add microbenchmarks to profile more code paths. (Russell Yanofsky)

sickpig mentioned this pull request Apr 28, 2020

Update benchmark suite BitcoinUnlimited/BitcoinUnlimited#2191

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add microbenchmarks to profile more code paths. #8873

Add microbenchmarks to profile more code paths. #8873

Uh oh!

ryanofsky commented Oct 3, 2016

Uh oh!

fanquake commented Oct 4, 2016

Uh oh!

fanquake commented Oct 4, 2016

Uh oh!

paveljanik Oct 4, 2016

Uh oh!

laanwj Oct 4, 2016 •

edited

Loading

Uh oh!

ryanofsky Oct 4, 2016

Uh oh!

ryanofsky Oct 4, 2016

Uh oh!

laanwj commented Oct 4, 2016

Uh oh!

sipa commented Oct 4, 2016 via email

Uh oh!

laanwj commented Oct 4, 2016

Uh oh!

fanquake commented Oct 9, 2016

Uh oh!

paveljanik Oct 9, 2016

Uh oh!

paveljanik commented Oct 9, 2016

Uh oh!

laanwj commented Oct 18, 2016

Uh oh!

Uh oh!

Add microbenchmarks to profile more code paths. #8873

Add microbenchmarks to profile more code paths. #8873

Uh oh!

Conversation

ryanofsky commented Oct 3, 2016

Uh oh!

fanquake commented Oct 4, 2016

Uh oh!

fanquake commented Oct 4, 2016

Uh oh!

paveljanik Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 4, 2016

Uh oh!

sipa commented Oct 4, 2016 via email

Uh oh!

laanwj commented Oct 4, 2016

Uh oh!

fanquake commented Oct 9, 2016

Uh oh!

paveljanik Oct 9, 2016

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Oct 9, 2016

Uh oh!

laanwj commented Oct 18, 2016

Uh oh!

Uh oh!

laanwj Oct 4, 2016 •

edited

Loading