refactor: Avoid copies in FlatSigningProvider Merge #25748
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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. |
This was referenced Jul 30, 2022
Member
|
Related is #16116. Could you also pull in the benchmark from that? |
Member
|
ACK fa00d8f84dca8ad9c5a3580e0f7a027f472cdc41 |
Member
|
Silent merge conflict with master |
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Aug 12, 2022
…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
Member
Author
|
Rebased and used @Empact's idea to transform this into a member function from https://github.com/bitcoin/bitcoin/pull/16116/files (thanks), which is a lot faster than an elided copy and std::move(). I've kept the argument as r-ref to allow stealing the pointers, which again should be faster than copying the entries individually. |
furszy
approved these changes
Aug 12, 2022
Member
|
ACK fa3f15f |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 12, 2022
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
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 18, 2022
fa3f15f refactor: Avoid copies in FlatSigningProvider Merge (MacroFake) Pull request description: `Merge` will create several copies unconditionally: * To initialize the args `a`, and `b` * `ret`, which is the merge of the two args So change the code to let the caller decide how many copies they need/want: * `a`, and `b` must be explicitly moved or copied by the caller * `ret` is no longer needed, as `a` can be used for it in place "for free" ACKs for top commit: achow101: ACK fa3f15f furszy: looks good, ACK fa3f15f ryanofsky: Code review ACK fa3f15f. Confirmed that all the places `std::move` was added the argument actually did seem safe to move from. Compiler enforces that temporary copies are explicitly created in non-move cases. Tree-SHA512: 7c027ccdea1549cd9f37403344ecbb76e008adf545f6ce52996bf95e89eb7dc89af6cb31435a9289d6f2eea1c416961b2fb96348bc8a211d550728f1d99ac49c
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mergewill create several copies unconditionally:a, andbret, which is the merge of the two argsSo change the code to let the caller decide how many copies they need/want:
a, andbmust be explicitly moved or copied by the callerretis no longer needed, asacan be used for it in place "for free"