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

pull l0rinc wants to merge 3 commits into bitcoin:master from l0rinc:l0rinc/performance-inefficient-vector-operation changing 7 files +11 −0
  1. l0rinc commented at 1:06 pm on November 17, 2024: contributor

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

    I ran the mentioned clang-tidy command via:

    0cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON
    1
    2run-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.

    0cd src && clang-tidy -list-checks | grep 'vector'
    1    performance-inefficient-vector-operation
    
     0src/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]
     1  433 |     for (int64_t i = 0; i < 100; i++) {
     2  434 |         feerates.emplace_back(1 ,1);
     3      |         ^
     4
     5src/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]
     6  365 |         for (size_t i = 0; i < 3; ++i) {
     7  366 |             tg.emplace_back(
     8      |             ^
     9
    10src/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]
    11  228 |     for (uint32_t x = 0; x < 3; ++x)
    12  229 |         /** Each thread is emplaced with x copy-by-value
    13  230 |         */
    14  231 |         threads.emplace_back([&, x] {
    15      |         ^
    16
    17src/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]
    18  126 |             for (unsigned int i = 0; i < keys.size(); ++i) {
    19  127 |                 pubkeys.push_back(HexToPubKey(keys[i].get_str()));
    20      |                 ^
    

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

  2. DrahtBot commented at 1:06 pm on November 17, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31305.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Refactoring on Nov 17, 2024
  4. hebasto commented at 4:25 pm on November 17, 2024: member
    Also see #31306.
  5. maflcko commented at 8:37 am on November 18, 2024: member
    lgtm ACK 9cb24f82cf88607e289bd53833a9bb0976653b26
  6. hebasto commented at 1:57 pm on November 18, 2024: member

    What about:

     0/ci_container_base/src/bench/prevector.cpp:91: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]
     1   90 |         for (size_t i = 0; i < 260; ++i) {
     2   91 |             vec.emplace_back();
     3      |             ^
     4/ci_container_base/src/bench/prevector.cpp:104: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]
     5  102 |         for (size_t i = 0; i < 260; ++i) {
     6  103 |             // force allocation
     7  104 |             vec.emplace_back(29, T{});
     8      |             ^
     9...
    10/ci_container_base/src/test/fuzz/txorphan.cpp:44: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]
    11   43 |     for (uint8_t i = 0; i < 4; i++) {
    12   44 |         outpoints.emplace_back(Txid::FromUint256(uint256{i}), 0);
    13      |         ^
    

    ?

  7. refactor: Fix remaining clang-tidy performance-inefficient-vector errors
    Before the change:
    > run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,performance-inefficient-vector-operation' | grep -v 'clang-tidy'
    
    ```
    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()));
          |                 ^
    ```
    6c7c38b875
  8. refactor: Preallocate PrevectorFillVector(In)Direct without vector resize
    The prevector benchmarks were likely not trying to measure vector resize performance.
    80699910ed
  9. refactor: Reserve vectors in fuzz tests
    * Since the main LIMITED_WHILE stated `outpoints.size() < 200'000`, I've presized outpoints accordingly.
    * `tx_mut.vin` and `tx_mut.vout` weren't caught by the clang-tidy, but addressed them anyway.
    01e54b7d82
  10. l0rinc force-pushed on Nov 18, 2024
  11. l0rinc commented at 3:29 pm on November 18, 2024: contributor
    Thanks @hebasto, addressed the bench and fuzz ones as well (in separate commits, explaining the decisions)
  12. hebasto commented at 3:53 pm on November 18, 2024: member
    Tested 01e54b7d82a053238b62121eeba2ac2c45b88859 on Ubuntu 24.04. The clang-tidy-19 with the enabled “performance-inefficient-vector-operation” check is happy.
  13. in src/test/fuzz/txorphan.cpp:41 in 01e54b7d82
    37@@ -38,6 +38,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
    38 
    39     TxOrphanage orphanage;
    40     std::vector<COutPoint> outpoints; // Duplicates are tolerated
    41+    outpoints.reserve(200'000);
    


    maflcko commented at 3:58 pm on November 18, 2024:

    This seems backwards? Fuzz engines will generally prefer smaller fuzz inputs over larger ones, given identical coverage metrics. Optimizing for the worst case, which will likely never happen, and possibly pessimising the happy case reads odd.

    Either this has no effect at all on the fuzz target anyway, in which case the code reads odd, or this is actually pessimising.


    l0rinc commented at 5:11 pm on November 18, 2024:
    Are you saying this will affect fuzzing behavior? Does it look under the hood to check how many items to create? I wasn’t aware of that and find it odd. What value do you recommend here instead of the worst case?

    maflcko commented at 9:16 am on November 19, 2024:

    Are you saying this will affect fuzzing behavior?

    No, I was thinking that this may slow down the fuzz target, but I don’t see any difference when running over ./fuzz_corpora/txorphan, so I guess this doesn’t matter.

    (Since you are asking, yes, this may influence the fuzz engine behavior. The reason is that the standard library is also annotated with coverage feedback, so a memory reallocation may count as a coverage increase)

  14. maflcko commented at 9:20 am on November 19, 2024: member
    lgtm ACK 01e54b7d82a053238b62121eeba2ac2c45b88859

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me