From c3d4ffba6559cd80400e888f55b3ffbc7f0b3593 Mon Sep 17 00:00:00 2001 From: edwinwu Date: Mon, 21 Mar 2022 09:36:53 -0700 Subject: [PATCH] [BLE Refactor] Fixed BloomFilterBase base member `_bits` is not initialized when construct from a non-empty ByteArray. PiperOrigin-RevId: 436229874 --- connections/implementation/mediums/ble_v2.cc | 6 +- .../mediums/ble_v2/bloom_filter.cc | 53 +++++---- .../mediums/ble_v2/bloom_filter.h | 104 +++++++++--------- .../mediums/ble_v2/bloom_filter_test.cc | 67 ++++++----- 4 files changed, 127 insertions(+), 103 deletions(-) diff --git a/connections/implementation/mediums/ble_v2.cc b/connections/implementation/mediums/ble_v2.cc index 919543b7..81fcc0f6 100644 --- a/connections/implementation/mediums/ble_v2.cc +++ b/connections/implementation/mediums/ble_v2.cc @@ -390,9 +390,9 @@ ByteArray BleV2::CreateAdvertisementHeader() { Utils::GenerateRandomBytes(kDummyServiceIdLength); std::string dummy_service_id{dummy_service_id_bytes}; - mediums::BloomFilter< - mediums::BleAdvertisementHeader::kServiceIdBloomFilterLength> - bloom_filter; + mediums::BloomFilter bloom_filter( + std::make_unique>()); bloom_filter.Add(dummy_service_id); ByteArray advertisement_hash = diff --git a/connections/implementation/mediums/ble_v2/bloom_filter.cc b/connections/implementation/mediums/ble_v2/bloom_filter.cc index 36bc7c79..23a13ccf 100644 --- a/connections/implementation/mediums/ble_v2/bloom_filter.cc +++ b/connections/implementation/mediums/ble_v2/bloom_filter.cc @@ -16,6 +16,7 @@ #include "absl/numeric/int128.h" #include "absl/strings/numbers.h" +#include "internal/platform/logging.h" #include "src/MurmurHash3.h" namespace location { @@ -23,33 +24,43 @@ namespace nearby { namespace connections { namespace mediums { -BloomFilterBase::BloomFilterBase(const ByteArray& bytes, BitSet* bit_set) - : bits_(bit_set) { +namespace { +constexpr int kHasherNumberOfRepetitions = 5; +} + +BloomFilter::BloomFilter(std::unique_ptr bit_set, + const ByteArray& bytes) + : bit_set_(std::move(bit_set)) { const char* bytes_read_ptr = bytes.data(); + + if (bytes.size() == 0) { + // Ignore it; we don't need to copy the bit for the empty bytes. + return; + } + // If the size is not matched, fall out. + if (bytes.size() * 8 != bit_set_->Size()) { + NEARBY_LOGS(INFO) << "Cannot construct from bytes since the size is not " + "matched. bytes.size(x8) = " + << bytes.size() << ", bit_set.size=" << bit_set_->Size(); + return; + } for (size_t byte_index = 0; byte_index < bytes.size(); byte_index++) { for (size_t bit_index = 0; bit_index < 8; bit_index++) { - bits_->Set((byte_index * 8) + bit_index, - (*bytes_read_ptr >> bit_index) & 0x01); + bit_set_->Set((byte_index * 8) + bit_index, + (*bytes_read_ptr >> bit_index) & 0x01); } bytes_read_ptr++; } } -BloomFilterBase::operator ByteArray() const { - // Gets a binary string representation of the bitset where the leftmost - // character corresponds to bitset position (total size) - 1. - // - // If the bitset's internal representation is: - // [position 0] 0 0 1 1 0 0 0 1 0 1 0 1 [position 11] - // The string representation will be outputted like this: - // "1 0 1 0 1 0 0 0 1 1 0 0" - std::string bitset_binary_string = bits_->ToString(); +BloomFilter::operator ByteArray() const { + std::string bitset_binary_string = bit_set_->ToString(); ByteArray result_bytes(GetMinBytesForBits()); char* result_bytes_write_ptr = result_bytes.data(); // We go through the string backwards because the rightmost character // corresponds to position 0 in the bitset. - for (size_t i = bits_->Size(); i > 0; i -= 8) { + for (size_t i = bit_set_->Size(); i > 0; i -= 8) { std::string byte_binary_string = bitset_binary_string.substr(i - 8, 8); std::uint32_t byte_value; absl::numbers_internal::safe_strtou32_base(byte_binary_string, &byte_value, @@ -60,26 +71,26 @@ BloomFilterBase::operator ByteArray() const { return result_bytes; } -void BloomFilterBase::Add(const std::string& s) { +void BloomFilter::Add(const std::string& s) { std::vector hashes = GetHashes(s); for (int32_t hash : hashes) { - size_t position = static_cast(hash) % bits_->Size(); - bits_->Set(position, true); + size_t position = static_cast(hash) % bit_set_->Size(); + bit_set_->Set(position, true); } } -bool BloomFilterBase::PossiblyContains(const std::string& s) { +bool BloomFilter::PossiblyContains(const std::string& s) { std::vector hashes = GetHashes(s); for (int32_t hash : hashes) { - size_t position = static_cast(hash) % bits_->Size(); - if (!bits_->Test(position)) { + size_t position = static_cast(hash) % bit_set_->Size(); + if (!bit_set_->Test(position)) { return false; } } return true; } -std::vector BloomFilterBase::GetHashes(const std::string& s) { +std::vector BloomFilter::GetHashes(const std::string& s) { std::vector hashes(kHasherNumberOfRepetitions, 0); absl::uint128 hash128; diff --git a/connections/implementation/mediums/ble_v2/bloom_filter.h b/connections/implementation/mediums/ble_v2/bloom_filter.h index 18f69d3e..151c51eb 100644 --- a/connections/implementation/mediums/ble_v2/bloom_filter.h +++ b/connections/implementation/mediums/ble_v2/bloom_filter.h @@ -25,72 +25,72 @@ namespace nearby { namespace connections { namespace mediums { -/** - * A bloom filter that gives access to the underlying BitSet. The implementation - * is copied from our Java version of Bloom filter, which in turn copies from - * Guava's BloomFilter. - * - * BloomFilter is templatized on the size of the byte array and not the size of - * the bit set to ensure the bit set's length is a multiple of 8 (and can - * neatly be returned as a ByteArray). - */ -class BloomFilterBase { +// Interface to set bits of the given bit array, by inserting a user element. +class BitSet { public: + virtual ~BitSet() = default; + + // Gets a binary string representation of the bitset where the leftmost + // character corresponds to bitset position (total size) - 1. + // + // If the bitset's internal representation is: + // [position 0] 0 0 1 1 0 0 0 1 0 1 0 1 [position 11] + // The string representation will be outputted like this: + // "1 0 1 0 1 0 0 0 1 1 0 0" + virtual std::string ToString() const = 0; + virtual void Set(size_t pos, bool value) = 0; + virtual bool Test(size_t pos) const = 0; + virtual size_t Size() const = 0; +}; + +// A bloom filter that gives access to the underlying BitSet. The implementation +// is copied from our Java version of Bloom filter, which in turn copies from +// Guava's BloomFilter. +class BloomFilter { + public: + // Constructs by injecting BitSet implementation. The bit_set will be default + // zero-out. + explicit BloomFilter(std::unique_ptr bit_set) + : BloomFilter(std::move(bit_set), {}) {} + + // Constructs by injecting BitSet implementation with bytes of other + // BloomFilter. The `bit_set` will be filled with the bit_set of other + // BloomFilter. + // + // Note: The capacity size of current bit_set should be the same as the one + // of other BloomFilter, or there is no impact and fallback to the first + // constructor. + BloomFilter(std::unique_ptr bit_set, const ByteArray& bytes); + BloomFilter(BloomFilter&& other) = default; + BloomFilter& operator=(BloomFilter&& other) = default; + explicit operator ByteArray() const; void Add(const std::string& s); bool PossiblyContains(const std::string& s); - protected: - class BitSet { - public: - virtual ~BitSet() = default; - virtual std::string ToString() const = 0; - virtual void Set(size_t pos, bool value) = 0; - virtual bool Test(size_t pos) const = 0; - virtual size_t Size() const = 0; - }; - - BloomFilterBase(const ByteArray& bytes, BitSet* bit_set); - virtual ~BloomFilterBase() = default; - - constexpr static int kHasherNumberOfRepetitions = 5; - std::vector GetHashes(const std::string& s); - private: - int GetMinBytesForBits() const { return (bits_->Size() + 7) >> 3; } + std::vector GetHashes(const std::string& s); + int GetMinBytesForBits() const { return (bit_set_->Size() + 7) >> 3; } - BitSet* bits_; + std::unique_ptr bit_set_; }; +// A default bit set implementation. +// +// It is templatized on the size of the byte array and not the size of +// the bit set to ensure the bit set's length is a multiple of 8 (and can +// neatly be returned as a ByteArray). template -class BloomFilter final : public BloomFilterBase { +class BitSetImpl final : public BitSet { public: - BloomFilter() : BloomFilterBase(ByteArray{}, &bits_) {} - explicit BloomFilter(const ByteArray& bytes) - : BloomFilterBase(bytes, &bits_) {} - BloomFilter(const BloomFilter&) = default; - BloomFilter& operator=(const BloomFilter&) = default; - BloomFilter(BloomFilter&& other) : BloomFilterBase(ByteArray{}, &bits_) { - *this = std::move(other); - } - BloomFilter& operator=(BloomFilter&& other) { - std::swap((*this).bits_, other.bits_); - return *this; - } - ~BloomFilter() override = default; + std::string ToString() const override { return bits_.to_string(); } + void Set(size_t pos, bool value) override { bits_.set(pos, value); } + bool Test(size_t pos) const override { return bits_.test(pos); } + size_t Size() const override { return bits_.size(); } private: - class BitSetImpl final : public BitSet { - public: - std::string ToString() const override { return bits_.to_string(); } - void Set(size_t pos, bool value) override { bits_.set(pos, value); } - bool Test(size_t pos) const override { return bits_.test(pos); } - size_t Size() const override { return bits_.size(); } - - private: - std::bitset bits_; - } bits_; + std::bitset bits_; }; } // namespace mediums diff --git a/connections/implementation/mediums/ble_v2/bloom_filter_test.cc b/connections/implementation/mediums/ble_v2/bloom_filter_test.cc index c2bf7b15..123cae97 100644 --- a/connections/implementation/mediums/ble_v2/bloom_filter_test.cc +++ b/connections/implementation/mediums/ble_v2/bloom_filter_test.cc @@ -27,7 +27,7 @@ namespace { constexpr size_t kByteArrayLength = 100; TEST(BloomFilterTest, EmptyFilterReturnsEmptyArray) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); ByteArray bloom_filter_bytes(bloom_filter); std::string empty_string(kByteArrayLength, '\0'); @@ -36,7 +36,7 @@ TEST(BloomFilterTest, EmptyFilterReturnsEmptyArray) { } TEST(BloomFilterTest, EmptyFilterNeverContains) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_1")); EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_2")); @@ -44,7 +44,7 @@ TEST(BloomFilterTest, EmptyFilterNeverContains) { } TEST(BloomFilterTest, AddSuccess) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_1")); @@ -54,7 +54,7 @@ TEST(BloomFilterTest, AddSuccess) { } TEST(BloomFilterTest, AddOnlyGivenArg) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); bloom_filter.Add("ELEMENT_1"); @@ -64,7 +64,7 @@ TEST(BloomFilterTest, AddOnlyGivenArg) { } TEST(BloomFilterTest, AddMultipleArgs) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); bloom_filter.Add("ELEMENT_1"); bloom_filter.Add("ELEMENT_2"); @@ -75,7 +75,7 @@ TEST(BloomFilterTest, AddMultipleArgs) { } TEST(BloomFilterTest, AddMultipleArgsReturnsNonemptyArray) { - BloomFilter<10> bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); bloom_filter.Add("ELEMENT_1"); bloom_filter.Add("ELEMENT_2"); @@ -87,37 +87,22 @@ TEST(BloomFilterTest, AddMultipleArgsReturnsNonemptyArray) { EXPECT_NE(std::string(bloom_filter_bytes), empty_string); } -TEST(BloomFilterTest, CopyConstructorAndAssignmentSuccess) { - BloomFilter bloom_filter; - - EXPECT_FALSE(bloom_filter.PossiblyContains("ELEMENT_1")); - - bloom_filter.Add("ELEMENT_1"); - - BloomFilter bloom_filter_copy_1{bloom_filter}; - BloomFilter bloom_filter_copy_2 = bloom_filter; - - EXPECT_TRUE(bloom_filter.PossiblyContains("ELEMENT_1")); - EXPECT_TRUE(bloom_filter_copy_1.PossiblyContains("ELEMENT_1")); - EXPECT_TRUE(bloom_filter_copy_2.PossiblyContains("ELEMENT_1")); -} - TEST(BloomFilterTest, MoveConstructorSuccess) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); bloom_filter.Add("ELEMENT_1"); - BloomFilter bloom_filter_move{std::move(bloom_filter)}; + BloomFilter bloom_filter_move{std::move(bloom_filter)}; EXPECT_TRUE(bloom_filter_move.PossiblyContains("ELEMENT_1")); } TEST(BloomFilterTest, MoveAssignmentSuccess) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); bloom_filter.Add("ELEMENT_1"); - BloomFilter bloom_filter_move = std::move(bloom_filter); + BloomFilter bloom_filter_move = std::move(bloom_filter); EXPECT_TRUE(bloom_filter_move.PossiblyContains("ELEMENT_1")); } @@ -134,7 +119,7 @@ TEST(BloomFilterTest, MoveAssignmentSuccess) { * something like [ 0, 1, 0, 0, 1, 1, 0, 0, 0, 1, ..., 1, 0]. */ TEST(BloomFilterTest, RandomnessNoEndBias) { - BloomFilter bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); // Add one element to our BloomFilter. bloom_filter.Add("ELEMENT_1"); @@ -178,7 +163,7 @@ TEST(BloomFilterTest, RandomnessNoEndBias) { } TEST(BloomFilterTest, RandomnessFalsePositiveRate) { - BloomFilter<10> bloom_filter; + BloomFilter bloom_filter(std::make_unique>()); // Add 5 distinct elements to the BloomFilter. bloom_filter.Add("ELEMENT_1"); @@ -200,6 +185,34 @@ TEST(BloomFilterTest, RandomnessFalsePositiveRate) { EXPECT_LE(false_positives, 5); } +TEST(BloomFilterTest, ConstructWithNonEmptyByteArrayWorks) { + BloomFilter bloom_filter(std::make_unique>()); + + bloom_filter.Add("ELEMENT_1"); + ByteArray original_bloom_filter_bytes(bloom_filter); + + BloomFilter bloom_filter_inherited( + std::make_unique>(), + original_bloom_filter_bytes); + + EXPECT_TRUE(bloom_filter_inherited.PossiblyContains("ELEMENT_1")); +} + +TEST(BloomFilterTest, ConstructLongByteArrayFails) { + // Make 1 more byte in original BloomFilter. + BloomFilter bloom_filter( + std::make_unique>()); + + bloom_filter.Add("ELEMENT_1"); + ByteArray original_bloom_filter_bytes(bloom_filter); + + BloomFilter bloom_filter_inherited( + std::make_unique>(), + original_bloom_filter_bytes); + + EXPECT_FALSE(bloom_filter_inherited.PossiblyContains("ELEMENT_1")); +} + } // namespace } // namespace mediums } // namespace connections