Skip to content

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Feb 20, 2024

This pull request introduces optimizations to the TryParseHex function, focusing primarily on the ideal case (valid hexadecimal input without spaces).
A new benchmark, HexParse was introduced in a separate commit.

The main optimization preallocates the result vector based on the input string's length. This aims to completely avoid costly dynamic reallocations when no spaces are present.


Before:

|           ns/base16 |            base16/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                1.60 |      623,238,893.11 |    0.3% |      0.01 | `HexParse`
|                1.65 |      606,747,566.34 |    0.6% |      0.01 | `HexParse`
|                1.60 |      626,149,544.07 |    0.3% |      0.01 | `HexParse`

After:

|           ns/base16 |            base16/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                0.68 |    1,465,555,976.27 |    0.8% |      0.01 | `HexParse`
|                0.68 |    1,472,962,920.18 |    0.3% |      0.01 | `HexParse`
|                0.68 |    1,476,159,423.00 |    0.3% |      0.01 | `HexParse`

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 20, 2024

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 hebasto, andrewtoth, Empact, achow101
Stale ACK maflcko

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

@l0rinc l0rinc changed the title benchmark/speedup: optimize TryParseHex calls considerably optimization: Speed up TryParseHex by 300% Feb 20, 2024
@l0rinc l0rinc marked this pull request as ready for review February 20, 2024 16:49
Copy link
Contributor

@andrewtoth andrewtoth left a comment

Choose a reason for hiding this comment

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

ACK a37a5b1

Bench before:

ns/base16 base16/s err% ins/base16 cyc/base16 IPC bra/base16 miss% total benchmark
1.88 532,130,184.93 1.4% 29.43 4.13 7.118 6.40 0.0% 0.53 TryParseHexBenchmark

After:

ns/base16 base16/s err% ins/base16 cyc/base16 IPC bra/base16 miss% total benchmark
0.82 1,226,337,041.45 2.5% 14.24 1.80 7.931 2.87 0.0% 0.54 TryParseHexBenchmark

@l0rinc l0rinc force-pushed the paplorinc/optimize-hex-parsing branch 2 times, most recently from 08d7410 to 262a605 Compare February 23, 2024 19:21
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/21921664428

@l0rinc l0rinc force-pushed the paplorinc/optimize-hex-parsing branch from 262a605 to ad4021b Compare February 23, 2024 19:27
@andrewtoth
Copy link
Contributor

Your last commit contains changes that should instead should be applied to the first and third commit and then rebased.

@l0rinc l0rinc force-pushed the paplorinc/optimize-hex-parsing branch from c68f430 to 589d0a4 Compare February 25, 2024 16:36
@l0rinc l0rinc changed the title optimization: Speed up TryParseHex by 300% optimization: Speed up TryParseHex by 200% Feb 25, 2024
@l0rinc l0rinc force-pushed the paplorinc/optimize-hex-parsing branch 2 times, most recently from 4b2e266 to 52140ae Compare February 25, 2024 16:59
fanquake added a commit that referenced this pull request Feb 26, 2024
b03b206 Fix CI-detected codespell warnings (Lőrinc)

Pull request description:

  Split out the typo fixes encountered in #29458 to a separate PR.

ACKs for top commit:
  maflcko:
    ACK b03b206

Tree-SHA512: 99b6fac01ba2ae6e6de9c50d2b481387899844a4b3a77d544c7b8afe7cfd25251a982329688d4739cde8b98ad35afcfd49be7c7cc3dad9bdff1d5915861a206d
@Empact
Copy link
Contributor

Empact commented Feb 27, 2024

Concept ACK - I think you could do without the 3rd commit.

@l0rinc
Copy link
Contributor Author

l0rinc commented Feb 27, 2024

Thanks for the review @Empact.
We could do without, that's why it's in a separate commit. It's very low risk however and makes the code more readable. If you think maintainability is risky, I can add more tests - what do you think?

@maflcko
Copy link
Member

maflcko commented Feb 27, 2024

It's very low risk however and makes the code more readable.

Why is that? According to https://corecheck.dev/bitcoin/bitcoin/pulls/29458 this commit is adding a bunch of sonarcloud warnings. I am not saying that those are accurate, but that "readability" is subjective. The general guideline on style changes applies:

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

  • Time spent on review
  • Accidental introduction of bugs
  • (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more information about refactoring changes and stylistic cleanup, see

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

Leave a comment below, if you have any questions.

@DrahtBot DrahtBot requested a review from andrewtoth February 27, 2024 17:41
Copy link
Contributor

@andrewtoth andrewtoth left a comment

Choose a reason for hiding this comment

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

ACK 0d127ca

Before:

ns/base16 base16/s err% ins/base16 cyc/base16 IPC bra/base16 miss% total benchmark
1.76 567,923,390.61 0.6% 29.43 3.88 7.577 6.40 0.0% 10.95 HexParse

After:

ns/base16 base16/s err% ins/base16 cyc/base16 IPC bra/base16 miss% total benchmark
0.81 1,234,838,738.87 0.5% 15.21 1.78 8.534 3.37 0.0% 10.84 HexParse

@maflcko
Copy link
Member

maflcko commented Feb 27, 2024

lgtm ACK 0d127ca

@DrahtBot DrahtBot removed the request for review from maflcko February 27, 2024 19:29
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK 0d127ca.

Observing ~x2 speed up on my machine.

Lőrinc added 2 commits February 28, 2024 17:23
Running `make && ./src/bench/bench_bitcoin -filter=HexParse` a few times results in:
```
|           ns/base16 |            base16/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                1.60 |      623,238,893.11 |    0.3% |      0.01 | `HexParse`
|                1.65 |      606,747,566.34 |    0.6% |      0.01 | `HexParse`
|                1.60 |      626,149,544.07 |    0.3% |      0.01 | `HexParse`
```
Running `make && ./src/bench/bench_bitcoin -filter=HexParse` a few times results in:
```
|           ns/base16 |            base16/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|                0.68 |    1,465,555,976.27 |    0.8% |      0.01 | `HexParse`
|                0.68 |    1,472,962,920.18 |    0.3% |      0.01 | `HexParse`
|                0.68 |    1,476,159,423.00 |    0.3% |      0.01 | `HexParse`
```
@l0rinc l0rinc force-pushed the paplorinc/optimize-hex-parsing branch from 0d127ca to a19235c Compare February 28, 2024 17:24
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK a19235c.

Copy link
Contributor

@andrewtoth andrewtoth left a comment

Choose a reason for hiding this comment

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

Re-ACK a19235c

@DrahtBot DrahtBot removed the request for review from Empact February 29, 2024 21:19
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.

Re-ACK a19235c

@achow101
Copy link
Member

ACK a19235c

@achow101 achow101 merged commit 6dda050 into bitcoin:master Mar 11, 2024
@l0rinc l0rinc deleted the paplorinc/optimize-hex-parsing branch March 11, 2024 14:18
achow101 added a commit that referenced this pull request Mar 13, 2024
6f2f4a4 Reserve memory for ToLower/ToUpper conversions (Lőrinc)

Pull request description:

  Similarly to #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 6f2f4a4
  stickies-v:
    ACK 6f2f4a4

Tree-SHA512: e3ba7af77decdc73272d804c94fef0b11028a85f3c0ea1ed6386672611b1c35fce151f02e64f5bb5acb5ba506aaa54577719b07925b9cc745143cf5c7e5eb262
achow101 added a commit that referenced this pull request Jun 13, 2024
…coding

07f6417 Reduce memory copying operations in bech32 encode (Lőrinc)
d5ece3c Reserve hrp memory in Decode and LocateErrors (Lőrinc)

Pull request description:

  Started optimizing the base conversions in [TryParseHex](#29458), [Base58](#29473) and [IsSpace](#29602) - this is the next step.

  Part of this change was already merged in #30047, which made decoding `~26%` faster.

  Here I've reduced the memory reallocations and copying operations in bech32 encode, making it `~15%` faster.

  >  make && ./src/bench/bench_bitcoin --filter='Bech32Encode' --min-time=1000

  Before:
  ```
  |             ns/byte |              byte/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |               19.97 |       50,074,562.72 |    0.1% |      1.06 | `Bech32Encode`
  ```

  After:
  ```
  |             ns/byte |              byte/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |               17.33 |       57,687,668.20 |    0.1% |      1.10 | `Bech32Encode`
  ```

ACKs for top commit:
  josibake:
    ACK 07f6417
  sipa:
    utACK 07f6417
  achow101:
    ACK 07f6417

Tree-SHA512: 511885217d044ad7ef2bdf9203b8e0b94eec8b279bc193bb7e63e29ab868df6d21e9e4c7a24390358e1f9c131447ee42039df72edcf1e2b11e1856eb2b3e10dd
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
…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
…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
…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
achow101 added a commit that referenced this pull request Nov 26, 2024
…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 Mar 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants