Skip to content
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: Reduce memory copying operations in bech32 encoding #29607

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paplorinc
Copy link
Contributor

@paplorinc paplorinc commented Mar 9, 2024

Started optimizing the base conversions in TryParseHex, Base58 and IsSpace - 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`

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 9, 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 josibake, sipa

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

Conflicts

No conflicts as of last run.

@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from 6c6c228 to 4f76265 Compare March 9, 2024 13:16
@paplorinc paplorinc marked this pull request as ready for review March 9, 2024 14:28
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from 4f76265 to db83766 Compare April 21, 2024 10:46
@paplorinc paplorinc marked this pull request as draft April 21, 2024 11:53
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch 10 times, most recently from f8fa74a to b3b84c3 Compare April 22, 2024 10:31
@paplorinc paplorinc marked this pull request as ready for review April 22, 2024 12:57
@josibake
Copy link
Member

Concept ACK

I cherry-picked your last commit into #30047 to get rid of the hardcoded values inside ExpandHRP. Looking at the rest of your commits here, the rest of the changes look great and the benchmark numbers are a nice improvement. Will review more thoroughly this week!

@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch 2 times, most recently from 7dd039a to b856835 Compare May 12, 2024 12:05
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from b856835 to 00dc933 Compare May 13, 2024 09:40
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from 00dc933 to 1048dd6 Compare May 20, 2024 18:25
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch 2 times, most recently from 1d96f43 to aa396df Compare May 29, 2024 09:27
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from aa396df to f45539c Compare June 5, 2024 10:03
@paplorinc paplorinc changed the title refactor: Reduce memory copying operations in bech32 encoding/decoding refactor: Reduce memory copying operations in bech32 encoding Jun 5, 2024
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from f45539c to ec0261d Compare June 5, 2024 10:46
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 5, 2024

🚧 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/25832897694

@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from ec0261d to d9586d6 Compare June 5, 2024 10:46
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch 4 times, most recently from 814c05e to e7c55d4 Compare June 5, 2024 10:59
@paplorinc
Copy link
Contributor Author

@josibake, now that the cherry-pick was merged, I've rebased and re-measured - the important part of this change was already included in the other PR, the remaining optimizations here are smaller, but also a lot simpler.

src/bech32.cpp Outdated Show resolved Hide resolved
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`

Co-authored-by: josibake <josibake@protonmail.com>
@paplorinc paplorinc force-pushed the paplorinc/bech32_optimizations branch from e7c55d4 to 07f6417 Compare June 5, 2024 11:19
@josibake
Copy link
Member

josibake commented Jun 5, 2024

ACK 07f6417

Verified the benchmark locally. Aside from the speed improvements, this also improves the readability of the code.

@DrahtBot DrahtBot removed the CI failed label Jun 5, 2024
@sipa
Copy link
Member

sipa commented Jun 5, 2024

utACK 07f6417

Making the code more readable is a good reason to make this change, but this code was specifically written to prioritize simplicity over performance. If we'd care about performance, it's probably better to use an approach similar to that of the C reference implementation (as it works entirely on the stack, while the current code does several allocations: 1 in ExpandHRP, between 1 and 3 in CreateChecksum, and 1 inevitable one in Encode).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants