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).

    The clang-tidy check can be run via:

    0cmake -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)
    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 theuni, maflcko, hebasto, achow101

    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. l0rinc force-pushed on Nov 18, 2024
  8. 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)
  9. 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.
  10. in src/test/fuzz/txorphan.cpp:41 in 01e54b7d82 outdated
    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)

  11. maflcko commented at 9:20 am on November 19, 2024: member
    lgtm ACK 01e54b7d82a053238b62121eeba2ac2c45b88859
  12. l0rinc force-pushed on Nov 25, 2024
  13. l0rinc commented at 12:54 pm on November 25, 2024: contributor

    Rebased on top of #31306 and updated commit messages

    Edit: removed the commits from #31306 (review)

  14. refactor: Fix remaining clang-tidy performance-inefficient-vector errors a774c7a339
  15. refactor: Preallocate PrevectorFillVector(In)Direct without vector resize
    The prevector benchmarks were likely not trying to measure vector resize performance.
    152fefe7a2
  16. 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.
    11f3bc229c
  17. l0rinc force-pushed on Nov 25, 2024
  18. theuni approved
  19. theuni commented at 7:43 pm on November 25, 2024: member
    ACK 11f3bc229ccd4b20191855fb1df882cfa6145264
  20. DrahtBot requested review from maflcko on Nov 25, 2024
  21. l0rinc commented at 7:53 pm on November 25, 2024: contributor
    Note that while the benchmarks indicate a speedup, they’re actually the ones that were changed:
  22. maflcko commented at 10:03 am on November 26, 2024: member

    review ACK 11f3bc229ccd4b20191855fb1df882cfa6145264 🎦

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: review ACK 11f3bc229ccd4b20191855fb1df882cfa6145264 🎦
    3Ert7vHi15ZR6P4PcKl82Cj4z08jBQBblV13as3K6VrO4QZqy91Cixn7/kETwGNxjPbISxlc/mFZ348jSrbOCDA==
    
  23. hebasto approved
  24. hebasto commented at 10:53 am on November 26, 2024: member
    ACK 11f3bc229ccd4b20191855fb1df882cfa6145264, tested with clang 19.1.5 + clang-tidy.
  25. bitcoin deleted a comment on Nov 26, 2024
  26. achow101 commented at 7:51 pm on November 26, 2024: member
    ACK 11f3bc229ccd4b20191855fb1df882cfa6145264
  27. achow101 merged this on Nov 26, 2024
  28. achow101 closed this on Nov 26, 2024


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-12-03 18:12 UTC

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