phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-24 18:23:26 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#31305: refactor: Fix remaining clang-tidy performance-inefficient-vector errors

Browse source 
11f3bc229c refactor: Reserve vectors in fuzz tests (Lőrinc)
152fefe7a2 refactor: Preallocate PrevectorFillVector(In)Direct without vector resize (Lőrinc)
a774c7a339 refactor: Fix remaining clang-tidy performance-inefficient-vector errors (Lőrinc)

Pull request description:

  PR inspired by https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307 (and https://github.com/bitcoin/bitcoin/pull/29458, https://github.com/bitcoin/bitcoin/pull/29606, https://github.com/bitcoin/bitcoin/pull/29607, https://github.com/bitcoin/bitcoin/pull/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: https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2483124499

  </details>

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

Tree-SHA512: 41691c19f35c63b922a95407617a54f9bff1af3f95f99d15642064f321df038aeb1ae5f061f854ed913f69036807cc28fa6222b2ff4c24ef43b909027fa0f9b3
This commit is contained in:
Ava Chow 2024-11-26 14:58:44 -05:00
commit 5a4bc5c036
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
7 changed files with 11 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/bench/prevector.cpp
View file

@ -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();
}
@ -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{});

1
src/rpc/output_script.cpp
View file

@ -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()));
}

1
src/test/checkqueue_tests.cpp
View file

@ -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) {

1
src/test/cuckoocache_tests.cpp
View file

@ -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

3
src/test/fuzz/txorphan.cpp
View file

@ -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++) {
@ -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{});
}

2
src/test/fuzz/util.h
View file

@ -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));
}
@ -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>());
}

1
src/test/rpc_tests.cpp
View file

@ -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++) {