Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#31305: refactor: Fix remaining clang-tidy perfo…
Browse files Browse the repository at this point in the history
…rmance-inefficient-vector errors

11f3bc2 refactor: Reserve vectors in fuzz tests (Lőrinc)
152fefe refactor: Preallocate PrevectorFillVector(In)Direct without vector resize (Lőrinc)
a774c7a refactor: Fix remaining clang-tidy performance-inefficient-vector errors (Lőrinc)

Pull request description:

  PR inspired by bitcoin/bitcoin#29608 (comment) (and bitcoin/bitcoin#29458, bitcoin/bitcoin#29606, bitcoin/bitcoin#29607, bitcoin/bitcoin#30093).

  The `clang-tidy` check can be run via:
  ```bash
  cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)

  run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-inefficient-vector-operation' | grep -v 'clang-tidy'
  ```
  which revealed 3 tests and 1 prod warning (+ fuzz and benching, found by hebasto).
  Even though the tests aren't performance critical, getting rid of these warnings (for which the checks were already enabled via https://github.com/bitcoin/bitcoin/blob/master/src/.clang-tidy#L18, see below), the fix was quite simple.

  <details>
  <summary>clang-tidy -list-checks</summary>

  ```bash
  cd src && clang-tidy -list-checks | grep 'vector'
      performance-inefficient-vector-operation
  ```

  </details>

  <details>
  <summary>Output before the change</summary>

  ```
  src/test/rpc_tests.cpp:434:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
    433 |     for (int64_t i = 0; i < 100; i++) {
    434 |         feerates.emplace_back(1 ,1);
        |         ^

  src/test/checkqueue_tests.cpp:366:13: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
    365 |         for (size_t i = 0; i < 3; ++i) {
    366 |             tg.emplace_back(
        |             ^

  src/test/cuckoocache_tests.cpp:231:9: error: 'emplace_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
    228 |     for (uint32_t x = 0; x < 3; ++x)
    229 |         /** Each thread is emplaced with x copy-by-value
    230 |         */
    231 |         threads.emplace_back([&, x] {
        |         ^

  src/rpc/output_script.cpp:127:17: error: 'push_back' is called inside a loop; consider pre-allocating the container capacity before the loop [performance-inefficient-vector-operation,-warnings-as-errors]
    126 |             for (unsigned int i = 0; i < keys.size(); ++i) {
    127 |                 pubkeys.push_back(HexToPubKey(keys[i].get_str()));
        |                 ^
  ```

  And the fuzz and benchmarks, noticed by hebasto: bitcoin/bitcoin#31305 (comment)

  </details>

ACKs for top commit:
  maflcko:
    review ACK 11f3bc2 🎦
  achow101:
    ACK 11f3bc2
  theuni:
    ACK 11f3bc2
  hebasto:
    ACK 11f3bc2, tested with clang 19.1.5 + clang-tidy.

Tree-SHA512: 41691c19f35c63b922a95407617a54f9bff1af3f95f99d15642064f321df038aeb1ae5f061f854ed913f69036807cc28fa6222b2ff4c24ef43b909027fa0f9b3
  • Loading branch information
achow101 committed Nov 26, 2024
2 parents 28fd0bc + 11f3bc2 commit 5a4bc5c
Show file tree
Hide file tree
Showing 7 changed files with 11 additions and 0 deletions.
2 changes: 2 additions & 0 deletions src/bench/prevector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ static void PrevectorFillVectorDirect(benchmark::Bench& bench)
{
bench.run([&] {
std::vector<prevector<28, T>> vec;
vec.reserve(260);
for (size_t i = 0; i < 260; ++i) {
vec.emplace_back();
}
Expand All @@ -99,6 +100,7 @@ static void PrevectorFillVectorIndirect(benchmark::Bench& bench)
{
bench.run([&] {
std::vector<prevector<28, T>> vec;
vec.reserve(260);
for (size_t i = 0; i < 260; ++i) {
// force allocation
vec.emplace_back(29, T{});
Expand Down
1 change: 1 addition & 0 deletions src/rpc/output_script.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ static RPCHelpMan createmultisig()
// Get the public keys
const UniValue& keys = request.params[1].get_array();
std::vector<CPubKey> pubkeys;
pubkeys.reserve(keys.size());
for (unsigned int i = 0; i < keys.size(); ++i) {
pubkeys.push_back(HexToPubKey(keys[i].get_str()));
}
Expand Down
1 change: 1 addition & 0 deletions src/test/checkqueue_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks)
auto queue = std::make_unique<Standard_Queue>(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS);
{
std::vector<std::thread> tg;
tg.reserve(3);
std::atomic<int> nThreads {0};
std::atomic<int> fails {0};
for (size_t i = 0; i < 3; ++i) {
Expand Down
1 change: 1 addition & 0 deletions src/test/cuckoocache_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ void test_cache_erase_parallel(size_t megabytes)
/** Spin up 3 threads to run contains with erase.
*/
std::vector<std::thread> threads;
threads.reserve(3);
/** Erase the first quarter */
for (uint32_t x = 0; x < 3; ++x)
/** Each thread is emplaced with x copy-by-value
Expand Down
3 changes: 3 additions & 0 deletions src/test/fuzz/txorphan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)

TxOrphanage orphanage;
std::vector<COutPoint> outpoints; // Duplicates are tolerated
outpoints.reserve(200'000);

// initial outpoints used to construct transactions later
for (uint8_t i = 0; i < 4; i++) {
Expand All @@ -55,12 +56,14 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
const auto num_out = fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(1, 256);
// pick outpoints from outpoints as input. We allow input duplicates on purpose, given we are not
// running any transaction validation logic before adding transactions to the orphanage
tx_mut.vin.reserve(num_in);
for (uint32_t i = 0; i < num_in; i++) {
auto& prevout = PickValue(fuzzed_data_provider, outpoints);
// try making transactions unique by setting a random nSequence, but allow duplicate transactions if they happen
tx_mut.vin.emplace_back(prevout, CScript{}, fuzzed_data_provider.ConsumeIntegralInRange<uint32_t>(0, CTxIn::SEQUENCE_FINAL));
}
// output amount will not affect txorphanage
tx_mut.vout.reserve(num_out);
for (uint32_t i = 0; i < num_out; i++) {
tx_mut.vout.emplace_back(CAmount{0}, CScript{});
}
Expand Down
2 changes: 2 additions & 0 deletions src/test/fuzz/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ template<typename B = uint8_t>
{
const size_t n_elements = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, max_vector_size);
std::vector<std::string> r;
r.reserve(n_elements);
for (size_t i = 0; i < n_elements; ++i) {
r.push_back(fuzzed_data_provider.ConsumeRandomLengthString(max_string_length));
}
Expand All @@ -90,6 +91,7 @@ template <typename T>
{
const size_t n_elements = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, max_vector_size);
std::vector<T> r;
r.reserve(n_elements);
for (size_t i = 0; i < n_elements; ++i) {
r.push_back(fuzzed_data_provider.ConsumeIntegral<T>());
}
Expand Down
1 change: 1 addition & 0 deletions src/test/rpc_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ BOOST_AUTO_TEST_CASE(rpc_getblockstats_calculate_percentiles_by_weight)
{
int64_t total_weight = 200;
std::vector<std::pair<CAmount, int64_t>> feerates;
feerates.reserve(200);
CAmount result[NUM_GETBLOCKSTATS_PERCENTILES] = { 0 };

for (int64_t i = 0; i < 100; i++) {
Expand Down

0 comments on commit 5a4bc5c

Please sign in to comment.