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
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
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
41 | bench_bench_bitcoin_LDADD += $(LIBBITCOIN_WALLET) 42 | endif 43 | 44 | bench_bench_bitcoin_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS) 45 | -bench_bench_bitcoin_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) 46 | +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.
Awesome! Thanks for doing this. Concept ACK.
I can't easily look at the source code now. What exactly causes the circular dependency?
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.
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
54 | +// characteristics than e.g. reindex timings. But that's not a requirement of 55 | +// every benchmark." 56 | +// (https://github.com/bitcoin/bitcoin/issues/7883#issuecomment-224807484) 57 | +static void CCoinsCaching(benchmark::State& state) 58 | +{ 59 | + CBasicKeyStore keystore;
Please clang-format the new code.
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: #7883
Applied clang-format to all the new files.