bloom: use Span instead of std::vector for insert and contains #23115

1. 
   fanquake commented at 7:39 am on September 28, 2021: member

   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.

2. fanquake added the label Refactoring on Sep 28, 2021
3. MarcoFalke renamed this:
   bloom: use Span instead of std::vector for `insert` and `contains` (#18985)
   bloom: use Span instead of std::vector for `insert` and `contains`
   on Sep 28, 2021
4. in src/bloom.h:71 in 69a20e8c6e outdated

   65@@ -66,11 +66,11 @@ class CBloomFilter
   66 
   67     SERIALIZE_METHODS(CBloomFilter, obj) { READWRITE(obj.vData, obj.nHashFuncs, obj.nTweak, obj.nFlags); }
   68 
   69-    void insert(const std::vector<unsigned char>& vKey);
   70+    void insert(Span<const unsigned char> vKey);
   71     void insert(const COutPoint& outpoint);
   72     void insert(const uint256& hash);
   

   ---

   ---

   

   martinus commented at 8:42 am on September 28, 2021:

   You could completely remove the void insert(const uint256& hash); and bool contains(const uint256& hash) const; methods in both CRollingBloomFilter and CBloomFilter, uint256 can be converted to Span automatically

   ---

   

   MarcoFalke commented at 10:39 am on September 28, 2021:

   Indeed: #18985 (review)

   ---

   

   fanquake commented at 1:56 am on September 29, 2021:

   Thought I’d done this in my branch already. Fixed.
5. 
   jonatack commented at 10:49 am on September 28, 2021: member

   Concept ACK
6. 
   laanwj commented at 1:14 pm on September 28, 2021: member

   Thanks for picking this up. Concept ACK.
7. bloom: use Span instead of std::vector for `insert` and `contains`

   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>

   2ba4ddf31d
8. bloom: use constexpr where appropriate f1ed1d3194
9. bloom: cleanup includes a11da75411
10. fanquake force-pushed on Sep 29, 2021
11. 
    fanquake commented at 1:57 am on September 29, 2021: member

    Fixed up the change that I should have already made, and added minor bloom cleanup commits.
12. 
    sipa commented at 2:48 am on September 29, 2021: member

    Code review ACK a11da7541148b5bb8e293c0ee49b2856a6628099
13. 
    laanwj commented at 12:30 pm on September 29, 2021: member

    Code review ACK a11da7541148b5bb8e293c0ee49b2856a6628099
14. MarcoFalke referenced this in commit 829c441af2 on Sep 29, 2021
15. sidhujag referenced this in commit a939fc6f46 on Sep 29, 2021
16. 
    DrahtBot commented at 3:06 pm on September 29, 2021: member

    🐙 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”.

17. DrahtBot added the label Needs rebase on Sep 29, 2021
18. MarcoFalke closed this on Sep 29, 2021

    ---

19. fanquake deleted the branch on Sep 29, 2021
20. DrahtBot locked this on Oct 30, 2022

Contributors
fanquake martinus MarcoFalke jonatack laanwj sipa DrahtBot

Labels
Refactoring Needs rebase