-
Notifications
You must be signed in to change notification settings - Fork 37.7k
refactor: Preallocate result in TryParseHex to avoid resizing #29458
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.
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
refactor: Preallocate result in TryParseHex to avoid resizing #29458
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this 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 |
08d7410
to
262a605
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
262a605
to
ad4021b
Compare
ad4021b
to
c68f430
Compare
Your last commit contains changes that should instead should be applied to the first and third commit and then rebased. |
c68f430
to
589d0a4
Compare
4b2e266
to
52140ae
Compare
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
Concept ACK - I think you could do without the 3rd commit. |
Thanks for the review @Empact. |
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:
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. |
There was a problem hiding this 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 |
lgtm ACK 0d127ca |
There was a problem hiding this 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.
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` ```
0d127ca
to
a19235c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a19235c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ACK a19235c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-ACK a19235c
ACK a19235c |
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
…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
…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
…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
…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
…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
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:
After: