Skip to content

refactor: Reserve memory for ToLower/ToUpper conversions #29606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 13, 2024
Merged

refactor: Reserve memory for ToLower/ToUpper conversions #29606

merged 1 commit into from
Mar 13, 2024

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Mar 8, 2024 •
edited
Loading

Similarly to #29458, we're preallocating the result string based on the input string's length.
The methods were already covered by tests.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 8, 2024 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK tdb3, Empact, maflcko, stickies-v, achow101

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

@tdb3
Copy link
Contributor

tdb3 commented Mar 9, 2024

ACK for 6f2f4a4
Cloned, built, ran all unit and functional tests (all passed).

Empact
Empact approved these changes Mar 9, 2024
View reviewed changes
Copy link
Contributor

@Empact Empact left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK 6f2f4a4

@maflcko
Copy link
Member

maflcko commented Mar 11, 2024

lgtm ACK 6f2f4a4

stickies-v
stickies-v approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6f2f4a4

@achow101
Copy link
Member

ACK 6f2f4a4

@achow101 achow101 merged commit c38157b into bitcoin:master Mar 13, 2024
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 14, 2024
@luke-jr
Reserve memory for ToLower/ToUpper conversions
2435c39 
Github-Pull: bitcoin#29606
Rebased-From: 6f2f4a4
@l0rinc l0rinc deleted the paplorinc/reserve-case-conversion branch May 11, 2024 20:19
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper con…
2421c19 
…versions

6f2f4a4 Reserve memory for ToLower/ToUpper conversions (Lőrinc)

Pull request description:

  Similarly to bitcoin#29458, we're preallocating the result string based on the input string's length.
  The methods were already [covered by tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/util_tests.cpp#L1250-L1276).

ACKs for top commit:
  tdb3:
    ACK for 6f2f4a4
  maflcko:
    lgtm ACK 6f2f4a4
  achow101:
    ACK 6f2f4a4
  Empact:
    Code Review ACK bitcoin@6f2f4a4
  stickies-v:
    ACK 6f2f4a4

Tree-SHA512: e3ba7af77decdc73272d804c94fef0b11028a85f3c0ea1ed6386672611b1c35fce151f02e64f5bb5acb5ba506aaa54577719b07925b9cc745143cf5c7e5eb262
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper con…
fb4cb64 
…versions

6f2f4a4 Reserve memory for ToLower/ToUpper conversions (Lőrinc)

Pull request description:

  Similarly to bitcoin#29458, we're preallocating the result string based on the input string's length.
  The methods were already [covered by tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/util_tests.cpp#L1250-L1276).

ACKs for top commit:
  tdb3:
    ACK for 6f2f4a4
  maflcko:
    lgtm ACK 6f2f4a4
  achow101:
    ACK 6f2f4a4
  Empact:
    Code Review ACK bitcoin@6f2f4a4
  stickies-v:
    ACK 6f2f4a4

Tree-SHA512: e3ba7af77decdc73272d804c94fef0b11028a85f3c0ea1ed6386672611b1c35fce151f02e64f5bb5acb5ba506aaa54577719b07925b9cc745143cf5c7e5eb262
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@achow101 @PastaPastaPasta
Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper con…
d0e15d5 
…versions

6f2f4a4 Reserve memory for ToLower/ToUpper conversions (Lőrinc)

Pull request description:

  Similarly to bitcoin#29458, we're preallocating the result string based on the input string's length.
  The methods were already [covered by tests](https://github.com/bitcoin/bitcoin/blob/master/src/test/util_tests.cpp#L1250-L1276).

ACKs for top commit:
  tdb3:
    ACK for 6f2f4a4
  maflcko:
    lgtm ACK 6f2f4a4
  achow101:
    ACK 6f2f4a4
  Empact:
    Code Review ACK bitcoin@6f2f4a4
  stickies-v:
    ACK 6f2f4a4

Tree-SHA512: e3ba7af77decdc73272d804c94fef0b11028a85f3c0ea1ed6386672611b1c35fce151f02e64f5bb5acb5ba506aaa54577719b07925b9cc745143cf5c7e5eb262
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
@PastaPastaPasta
Merge #6345: backport: trivial 2024 10 23 pr1
aaccc9e 
b70e091 Merge bitcoin#29667: fuzz: actually test garbage >64b in p2p transport test (fanquake)
6d7aa3d Merge bitcoin#29497: test: simplify test_runner.py (fanquake)
d0e15d5 Merge bitcoin#29606: refactor: Reserve memory for ToLower/ToUpper conversions (Ava Chow)
045fa5f Merge bitcoin#29514: tests: Provide more helpful assert_equal errors (Ava Chow)
bd607f0 Merge bitcoin#29393: i2p: log connection was refused due to arbitrary port (Ava Chow)
c961755 Merge bitcoin#29595: doc: Wrap flags with code in developer-notes.md (fanquake)
8d6e5e7 Merge bitcoin#29583: fuzz: Apply fuzz env (suppressions, etc.) when fetching harness list (fanquake)
4dce690 Merge bitcoin#29576: Update functional test runner to return error code when no tests are found to run (fanquake)
910a7d6 Merge bitcoin#29529: fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake)
fdac2b3 Merge bitcoin#29493: subtree: update crc32c subtree (fanquake)
a23b342 Merge bitcoin#29475: doc: Fix Broken Links (fanquake)
92bad90 Merge bitcoin#28178: fuzz: Generate with random libFuzzer settings (fanquake)
9b6a05d Merge bitcoin#29443: depends: fix BDB compilation on OpenBSD (fanquake)
9963e6b Merge bitcoin#29413: fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse` (fanquake)
3914745 Merge bitcoin#29425: test: fix intermittent failure in wallet_reorgrestore.py (fanquake)
b719883 Merge bitcoin#29399: test: Fix utxo set hash serialisation signedness (fanquake)
f096880 Merge bitcoin#29377: test: Add makefile target for running unit tests (Ava Chow)
03e0bd3 Merge bitcoin#27319: addrman, refactor: improve stochastic test in `AddSingle` (Ava Chow)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## What was done?
  See commits

  ## How Has This Been Tested?
  built locally; large combined merge passed tests locally

  ## Breaking Changes
  Should be none

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK b70e091
  knst:
    utACK b70e091

Tree-SHA512: 659a931f812c8a92cf3854abb873d92885219a392d6aa8e49ac4b27fe41e8e163ef9a135050e7c2e1bd33cecd2f7dae215e05a9c29f62e837e0057d3c16746d6
@l0rinc l0rinc mentioned this pull request Nov 17, 2024
Merged
achow101 added a commit that referenced this pull request Nov 26, 2024
@achow101
Merge #31305: refactor: Fix remaining clang-tidy performance-ineffici…
5a4bc5c 
…ent-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 #29608 (comment) (and #29458, #29606, #29607, #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: #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
@bitcoin bitcoin locked and limited conversation to collaborators May 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Empact Empact Empact approved these changes

@stickies-v stickies-v stickies-v approved these changes

@jess2505 jess2505 jess2505 approved these changes

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@l0rinc @DrahtBot @tdb3 @maflcko @achow101 @Empact @stickies-v @jess2505