bloom: use Span instead of std::vector for insert and contains
#23115
+28
−54
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
insert and contains (#18985)insert and contains
martinus
reviewed
Sep 28, 2021
|
Concept ACK |
|
Thanks for picking this up. Concept ACK. |
We can avoid many unnecessary std::vector allocations by changing CBloomFilter to take Spans instead of std::vector's for the `insert` and `contains` operations. CBloomFilter currently converts types such as CDataStream and uint256 to std::vector on `insert` and `contains`. This is unnecessary because CDataStreams and uint256 are already std::vectors internally. We just need a way to point to the right data within those types. Span gives us this ability. Signed-off-by: William Casarin <jb55@jb55.com>
69a20e8 to
a11da75
Compare
|
Fixed up the change that I should have already made, and added minor bloom cleanup commits. |
|
Code review ACK a11da75 |
1 similar comment
|
Code review ACK a11da75 |
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Sep 29, 2021
…or `insert` and `contains` a11da75 bloom: cleanup includes (fanquake) f1ed1d3 bloom: use constexpr where appropriate (fanquake) 2ba4ddf bloom: use Span instead of std::vector for `insert` and `contains` (William Casarin) Pull request description: This is #18985 rebased, with the most recent comments addressed. > We can avoid many unnecessary std::vector allocations by changing CBloomFilter to take Spans instead of std::vector's for the `insert` and `contains` operations. > CBloomFilter currently converts types such as CDataStream and uint256 to std::vector on `insert` and `contains`. This is unnecessary because CDataStreams and uint256 are already std::vectors internally. We just need a way to point to the right data within those types. Span gives us this ability. ACKs for top commit: sipa: Code review ACK a11da75 laanwj: Code review ACK a11da75 Tree-SHA512: ee9ba02c9588daa1ff51782d1953fd060839dd15aa85861b2633b6ff2398320188ddd00f01d0c99442224485364ede9f8322366de4239fc7831ebfa06bd34659
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Sep 29, 2021
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This is #18985 rebased, with the most recent comments addressed.