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
Avoid unnecessary signing provider copies on descriptor expansion #16116
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
@Empact Can you quantify the improvement via a benchmark? |
d7d31d2
to
7748ecf
Compare
Added bench info to the description. |
Apart from the micro-benchmark testing this specific code: does this make signing (or any other RPC calls) noticeably faster? |
Any suggestion as to how to produce a representative and broad test of this? |
7748ecf
to
70d23eb
Compare
70d23eb
to
c059106
Compare
Concept ACK Edit: There are a few places where |
c059106
to
6c00640
Compare
6c00640
to
5024d30
Compare
Concept ACK |
Currently descriptor expansion unnecessarily copies the signing provider data once per expansion. Avoid this work by making it a member of the class and doing the merge in-place. And add a benchmark for descriptor expansion to characterize the change.
5024d30
to
346c9ee
Compare
Silent merge conflict with master:
This is a fairly simple change, and I'd like to get it in. |
Should this be marked "up for grabs"? |
…pansion 4786959 bench: Add a benchmark for descriptor expansion (Ben Woosley) Pull request description: Taken from bitcoin/bitcoin#16116 , as requested here: bitcoin/bitcoin#25748 (comment) ACKs for top commit: achow101: ACK 4786959 Tree-SHA512: f2efdf8f84e1783c7c298abe65123191d25cab0a9da2d0ff5957a60acc2d10e356151d7ecec0d98d28c456f42ddef50efd70c7edc0c9012df2a977e080515b9d
4786959 bench: Add a benchmark for descriptor expansion (Ben Woosley) Pull request description: Taken from bitcoin#16116 , as requested here: bitcoin#25748 (comment) ACKs for top commit: achow101: ACK 4786959 Tree-SHA512: f2efdf8f84e1783c7c298abe65123191d25cab0a9da2d0ff5957a60acc2d10e356151d7ecec0d98d28c456f42ddef50efd70c7edc0c9012df2a977e080515b9d
Currently descriptor expansion unnecessarily copies the signing provider
data once per expansion. Avoid this work by making it a member of the
class and doing the merge in-place.
Current master Empact@604606d
ExpandDescriptor, 5, 6, 1.22676, 0.0392286, 0.0428134, 0.0403623
This PR 7748ecf
ExpandDescriptor, 5, 6, 1.02993, 0.0333122, 0.0353901, 0.0343695
Note a ranged descriptor is expanded 1000x by default, and the descriptor string I used is from the test suite.
bitcoin/src/test/descriptor_tests.cpp
Line 210 in c7cfd20