Use ByteString instead of std::vector in the BloomFilter (#11038)

Author: dconeybe

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

@@ -342,7 +342,7 @@ - (SnapshotVersion)parseVersion:(NSNumber *_Nullable)version {
   NSString *bitmapEncoded = bitsData[@"bitmap"];
   std::string bitmapDecoded;
   absl::Base64Unescape([bitmapEncoded cStringUsingEncoding:NSASCIIStringEncoding], &bitmapDecoded);
-  std::vector<uint8_t> bitmap(bitmapDecoded.begin(), bitmapDecoded.end());
+  ByteString bitmap(bitmapDecoded);
 
   // If not specified in proto, default padding and hashCount to 0.
   int32_t padding = [bitsData[@"padding"] intValue];

Firestore/core/src/remote/bloom_filter.cc

@@ -26,6 +26,7 @@ namespace firebase {
 namespace firestore {
 namespace remote {
 
+using nanopb::ByteString;
 using util::Status;
 using util::StatusOr;
 
@@ -38,9 +39,9 @@ bool HasSameBits(const BloomFilter& lhs, const BloomFilter& rhs) {
     return true;
   }
 
-  const std::vector<uint8_t>& bitmap1 = lhs.bitmap();
-  const std::vector<uint8_t>& bitmap2 = rhs.bitmap();
-  const auto byte_count = static_cast<int32_t>(bitmap1.size());
+  const auto byte_count = static_cast<int32_t>(lhs.bitmap().size());
+  const uint8_t* bitmap1 = lhs.bitmap().data();
+  const uint8_t* bitmap2 = rhs.bitmap().data();
 
   // Compare all bytes from the bitmap, except for the last byte.
   for (int32_t i = 0; i < byte_count - 1; ++i) {
@@ -81,14 +82,12 @@ int32_t BloomFilter::GetBitIndex(const Hash& hash, int32_t hash_index) const {
 }
 
 bool BloomFilter::IsBitSet(int32_t index) const {
-  uint8_t byte_at_index = bitmap_[index / 8];
+  uint8_t byte_at_index = bitmap_.data()[index / 8];
   int32_t offset = index % 8;
   return (byte_at_index & (static_cast<uint8_t>(0x01) << offset)) != 0;
 }
 
-BloomFilter::BloomFilter(std::vector<uint8_t> bitmap,
-                         int32_t padding,
-                         int32_t hash_count)
+BloomFilter::BloomFilter(ByteString bitmap, int32_t padding, int32_t hash_count)
     : bit_count_(static_cast<int32_t>(bitmap.size()) * 8 - padding),
       hash_count_(hash_count),
       bitmap_(std::move(bitmap)) {
@@ -101,7 +100,7 @@ BloomFilter::BloomFilter(std::vector<uint8_t> bitmap,
   HARD_ASSERT(bit_count_ >= 0);
 }
 
-StatusOr<BloomFilter> BloomFilter::Create(std::vector<uint8_t> bitmap,
+StatusOr<BloomFilter> BloomFilter::Create(ByteString bitmap,
                                           int32_t padding,
                                           int32_t hash_count) {
   if (padding < 0 || padding >= 8) {

Firestore/core/src/remote/bloom_filter.h

@@ -18,7 +18,7 @@
 #define FIRESTORE_CORE_SRC_REMOTE_BLOOM_FILTER_H_
 
 #include <string>
-#include <vector>
+#include "Firestore/core/src/nanopb/byte_string.h"
 #include "Firestore/core/src/util/statusor.h"
 #include "absl/strings/string_view.h"
 
@@ -28,7 +28,7 @@ namespace remote {
 
 class BloomFilter final {
  public:
-  BloomFilter(std::vector<uint8_t> bitmap, int32_t padding, int32_t hash_count);
+  BloomFilter(nanopb::ByteString bitmap, int32_t padding, int32_t hash_count);
 
   // Copyable & movable.
   BloomFilter(const BloomFilter&) = default;
@@ -42,7 +42,7 @@ class BloomFilter final {
    * @return a new BloomFilter if the inputs are valid, otherwise returns a not
    * `ok()` status.
    */
-  static util::StatusOr<BloomFilter> Create(std::vector<uint8_t> bitmap,
+  static util::StatusOr<BloomFilter> Create(nanopb::ByteString bitmap,
                                             int32_t padding,
                                             int32_t hash_count);
 
@@ -75,7 +75,7 @@ class BloomFilter final {
   }
 
   /** Bloom filter's bitmap. */
-  const std::vector<uint8_t>& bitmap() const {
+  const nanopb::ByteString& bitmap() const {
     return bitmap_;
   }
 
@@ -108,7 +108,7 @@ class BloomFilter final {
 
   int32_t hash_count_ = 0;
 
-  std::vector<uint8_t> bitmap_;
+  nanopb::ByteString bitmap_;
 };
 
 bool operator==(const BloomFilter& lhs, const BloomFilter& rhs);

Firestore/core/src/remote/existence_filter.h

@@ -18,16 +18,16 @@
 #define FIRESTORE_CORE_SRC_REMOTE_EXISTENCE_FILTER_H_
 
 #include <utility>
-#include <vector>
 
+#include "Firestore/core/src/nanopb/byte_string.h"
 #include "Firestore/core/src/remote/bloom_filter.h"
 
 namespace firebase {
 namespace firestore {
 namespace remote {
 
 struct BloomFilterParameters {
-  std::vector<uint8_t> bitmap;
+  nanopb::ByteString bitmap;
   int32_t padding;
   int32_t hash_count;
 };

Firestore/core/src/remote/serializer.cc

@@ -1456,15 +1456,13 @@ ExistenceFilter Serializer::DecodeExistenceFilter(
 
   int32_t hash_count = filter.unchanged_names.hash_count;
   int32_t padding = 0;
-  std::vector<uint8_t> bitmap;
+  ByteString bitmap;
   if (filter.unchanged_names.has_bits) {
     padding = filter.unchanged_names.bits.padding;
-
-    pb_bytes_array_t* bitmap_ptr = filter.unchanged_names.bits.bitmap;
-    if (bitmap_ptr != nullptr) {
-      bitmap = std::vector<uint8_t>(bitmap_ptr->bytes,
-                                    bitmap_ptr->bytes + bitmap_ptr->size);
-    }
+    // TODO(b/274668697) Steal the bytes using ByteString::Take() instead of
+    //  copying them. To do this, the `filter` argument will need to be
+    //  non-const, which will affect the caller(s), and their caller(s), etc.
+    bitmap = ByteString(filter.unchanged_names.bits.bitmap);
   }
   return {filter.count,
           BloomFilterParameters{std::move(bitmap), padding, hash_count}};

Firestore/core/test/unit/remote/bloom_filter_test.cc

@@ -18,7 +18,6 @@
 
 #include <fstream>
 #include <iostream>
-#include <vector>
 
 #include "Firestore/core/src/util/hard_assert.h"
 #include "Firestore/core/src/util/json_reader.h"
@@ -31,39 +30,40 @@ namespace firestore {
 namespace remote {
 namespace {
 
+using nanopb::ByteString;
 using nlohmann::json;
 using util::JsonReader;
 using util::Path;
 using util::Status;
 using util::StatusOr;
 
 TEST(BloomFilterUnitTest, CanInstantiateEmptyBloomFilter) {
-  BloomFilter bloom_filter(std::vector<uint8_t>{}, 0, 0);
+  BloomFilter bloom_filter(ByteString{}, 0, 0);
   EXPECT_EQ(bloom_filter.bit_count(), 0);
 }
 
 TEST(BloomFilterUnitTest, CanInstantiateNonEmptyBloomFilter) {
   {
-    BloomFilter bloom_filter(std::vector<uint8_t>{1}, 0, 1);
+    BloomFilter bloom_filter(ByteString{1}, 0, 1);
     EXPECT_EQ(bloom_filter.bit_count(), 8);
   }
   {
-    BloomFilter bloom_filter(std::vector<uint8_t>{1}, 7, 1);
+    BloomFilter bloom_filter(ByteString{1}, 7, 1);
     EXPECT_EQ(bloom_filter.bit_count(), 1);
   }
 }
 
 TEST(BloomFilterUnitTest, CreateShouldReturnBloomFilterOnValidInputs) {
   StatusOr<BloomFilter> maybe_bloom_filter =
-      BloomFilter::Create(std::vector<uint8_t>{1}, 1, 1);
+      BloomFilter::Create(ByteString{1}, 1, 1);
   ASSERT_TRUE(maybe_bloom_filter.ok());
   BloomFilter bloom_filter = maybe_bloom_filter.ValueOrDie();
   EXPECT_EQ(bloom_filter.bit_count(), 7);
 }
 
 TEST(BloomFilterUnitTest, CreateShouldBeAbleToCreatEmptyBloomFilter) {
   StatusOr<BloomFilter> maybe_bloom_filter =
-      BloomFilter::Create(std::vector<uint8_t>{}, 0, 0);
+      BloomFilter::Create(ByteString{}, 0, 0);
   ASSERT_TRUE(maybe_bloom_filter.ok());
   BloomFilter bloom_filter = maybe_bloom_filter.ValueOrDie();
   EXPECT_EQ(bloom_filter.bit_count(), 0);
@@ -72,14 +72,14 @@ TEST(BloomFilterUnitTest, CreateShouldBeAbleToCreatEmptyBloomFilter) {
 TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativePadding) {
   {
     StatusOr<BloomFilter> maybe_bloom_filter =
-        BloomFilter::Create(std::vector<uint8_t>{}, -1, 0);
+        BloomFilter::Create(ByteString{}, -1, 0);
     ASSERT_FALSE(maybe_bloom_filter.ok());
     EXPECT_EQ(maybe_bloom_filter.status().error_message(),
               "Invalid padding: -1");
   }
   {
     StatusOr<BloomFilter> maybe_bloom_filter =
-        BloomFilter::Create(std::vector<uint8_t>{1}, -1, 1);
+        BloomFilter::Create(ByteString{1}, -1, 1);
     ASSERT_FALSE(maybe_bloom_filter.ok());
     EXPECT_EQ(maybe_bloom_filter.status().error_message(),
               "Invalid padding: -1");
@@ -89,14 +89,14 @@ TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativePadding) {
 TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativeHashCount) {
   {
     StatusOr<BloomFilter> maybe_bloom_filter =
-        BloomFilter::Create(std::vector<uint8_t>{}, 0, -1);
+        BloomFilter::Create(ByteString{}, 0, -1);
     ASSERT_FALSE(maybe_bloom_filter.ok());
     EXPECT_EQ(maybe_bloom_filter.status().error_message(),
               "Invalid hash count: -1");
   }
   {
     StatusOr<BloomFilter> maybe_bloom_filter =
-        BloomFilter::Create(std::vector<uint8_t>{1}, 1, -1);
+        BloomFilter::Create(ByteString{1}, 1, -1);
     ASSERT_FALSE(maybe_bloom_filter.ok());
     EXPECT_EQ(maybe_bloom_filter.status().error_message(),
               "Invalid hash count: -1");
@@ -105,51 +105,51 @@ TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnNegativeHashCount) {
 
 TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusOnZeroHashCount) {
   StatusOr<BloomFilter> maybe_bloom_filter =
-      BloomFilter::Create(std::vector<uint8_t>{1}, 1, 0);
+      BloomFilter::Create(ByteString{1}, 1, 0);
   ASSERT_FALSE(maybe_bloom_filter.ok());
   EXPECT_EQ(maybe_bloom_filter.status().error_message(),
             "Invalid hash count: 0");
 }
 
 TEST(BloomFilterUnitTest, CreateShouldReturnNotOKStatusIfPaddingIsTooLarge) {
   StatusOr<BloomFilter> maybe_bloom_filter =
-      BloomFilter::Create(std::vector<uint8_t>{1}, 8, 1);
+      BloomFilter::Create(ByteString{1}, 8, 1);
   ASSERT_FALSE(maybe_bloom_filter.ok());
   EXPECT_EQ(maybe_bloom_filter.status().error_message(), "Invalid padding: 8");
 }
 
 TEST(BloomFilterTest, CheckBloomFiltersEqualityWithSameInput) {
-  BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
-  BloomFilter bloom_filter2(std::vector<uint8_t>{1}, 1, 1);
+  BloomFilter bloom_filter1(ByteString{1}, 1, 1);
+  BloomFilter bloom_filter2(ByteString{1}, 1, 1);
   EXPECT_TRUE(bloom_filter1 == bloom_filter2);
   EXPECT_FALSE(bloom_filter1 != bloom_filter2);
 }
 
 TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentBitmap) {
   {
-    BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
-    BloomFilter bloom_filter2(std::vector<uint8_t>{2}, 1, 1);
+    BloomFilter bloom_filter1(ByteString{1}, 1, 1);
+    BloomFilter bloom_filter2(ByteString{2}, 1, 1);
     EXPECT_FALSE(bloom_filter1 == bloom_filter2);
     EXPECT_TRUE(bloom_filter1 != bloom_filter2);
   }
   {
-    BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
-    BloomFilter bloom_filter2(std::vector<uint8_t>{1, 1}, 1, 1);
+    BloomFilter bloom_filter1(ByteString{1}, 1, 1);
+    BloomFilter bloom_filter2(ByteString{1, 1}, 1, 1);
     EXPECT_FALSE(bloom_filter1 == bloom_filter2);
     EXPECT_TRUE(bloom_filter1 != bloom_filter2);
   }
 }
 
 TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentPadding) {
-  BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
-  BloomFilter bloom_filter2(std::vector<uint8_t>{1}, 2, 1);
+  BloomFilter bloom_filter1(ByteString{1}, 1, 1);
+  BloomFilter bloom_filter2(ByteString{1}, 2, 1);
   EXPECT_FALSE(bloom_filter1 == bloom_filter2);
   EXPECT_TRUE(bloom_filter1 != bloom_filter2);
 }
 
 TEST(BloomFilterTest, CheckBloomFiltersEqualityWithDifferentHashCount) {
-  BloomFilter bloom_filter1(std::vector<uint8_t>{1}, 1, 1);
-  BloomFilter bloom_filter2(std::vector<uint8_t>{1}, 1, 2);
+  BloomFilter bloom_filter1(ByteString{1}, 1, 1);
+  BloomFilter bloom_filter2(ByteString{1}, 1, 2);
   EXPECT_FALSE(bloom_filter1 == bloom_filter2);
   EXPECT_TRUE(bloom_filter1 != bloom_filter2);
 }
@@ -159,17 +159,17 @@ TEST(BloomFilterTest,
   // In BloomFilter bitmap, padding is guaranteed to be less than 8, and
   // starts counting from the leftmost indexes of the last byte.
   {
-    BloomFilter bloom_filter1(std::vector<uint8_t>{255, 127}, 1,
+    BloomFilter bloom_filter1(ByteString{255, 127}, 1,
                               1);  // bitmap -> 11111111 01111111
-    BloomFilter bloom_filter2(std::vector<uint8_t>{255, 255}, 1,
+    BloomFilter bloom_filter2(ByteString{255, 255}, 1,
                               1);  // bitmap -> 11111111 11111111
     EXPECT_TRUE(bloom_filter1 == bloom_filter2);
     EXPECT_FALSE(bloom_filter1 != bloom_filter2);
   }
   {
-    BloomFilter bloom_filter1(std::vector<uint8_t>{255, 207}, 4,
+    BloomFilter bloom_filter1(ByteString{255, 207}, 4,
                               1);  // bitmap -> 11111111 11001111
-    BloomFilter bloom_filter2(std::vector<uint8_t>{255, 255}, 4,
+    BloomFilter bloom_filter2(ByteString{255, 255}, 4,
                               1);  // bitmap -> 11111111 11111111
     EXPECT_TRUE(bloom_filter1 == bloom_filter2);
     EXPECT_FALSE(bloom_filter1 != bloom_filter2);
@@ -178,25 +178,25 @@ TEST(BloomFilterTest,
 
 TEST(BloomFilterTest, MightContainCanProcessNonStandardCharacters) {
   // A non-empty BloomFilter object with 1 insertion : "ÀÒ∑"
-  BloomFilter bloom_filter(std::vector<uint8_t>{237, 5}, 5, 8);
+  BloomFilter bloom_filter(ByteString{237, 5}, 5, 8);
   EXPECT_TRUE(bloom_filter.MightContain("ÀÒ∑"));
   EXPECT_FALSE(bloom_filter.MightContain("Ò∑À"));
 }
 
 TEST(BloomFilterUnitTest, MightContainOnEmptyBloomFilterShouldReturnFalse) {
-  BloomFilter bloom_filter(std::vector<uint8_t>{}, 0, 0);
+  BloomFilter bloom_filter(ByteString{}, 0, 0);
   EXPECT_FALSE(bloom_filter.MightContain(""));
   EXPECT_FALSE(bloom_filter.MightContain("a"));
 }
 
 TEST(BloomFilterUnitTest,
      MightContainWithEmptyStringMightReturnFalsePositiveResult) {
   {
-    BloomFilter bloom_filter(std::vector<uint8_t>{1}, 1, 1);
+    BloomFilter bloom_filter(ByteString{1}, 1, 1);
     EXPECT_FALSE(bloom_filter.MightContain(""));
   }
   {
-    BloomFilter bloom_filter(std::vector<uint8_t>{255}, 0, 16);
+    BloomFilter bloom_filter(ByteString{255}, 0, 16);
     EXPECT_TRUE(bloom_filter.MightContain(""));
   }
 }
@@ -241,7 +241,7 @@ class BloomFilterGoldenTest : public ::testing::Test {
     int hash_count = reader.OptionalInt("hashCount", test_file, 0);
     std::string decoded;
     absl::Base64Unescape(bitmap, &decoded);
-    std::vector<uint8_t> decoded_map(decoded.begin(), decoded.end());
+    ByteString decoded_map(decoded);
 
     StatusOr<BloomFilter> maybe_bloom_filter =
         BloomFilter::Create(std::move(decoded_map), padding, hash_count);