From c12990ae33292e879db6dd492d2643dec6297e52 Mon Sep 17 00:00:00 2001
From: florijan <florijan@memgraph.io>
Date: Mon, 12 Jun 2017 12:38:06 +0200
Subject: [PATCH] DynamicBitset - const correctness, tests, docs

Reviewers: dtomicevic, buda

Reviewed By: buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D453
---
 src/data_structures/bitset/dynamic_bitset.hpp | 155 +++++++++++++-----
 tests/unit/dynamic_bitset.cpp                 |  94 +++++++++--
 2 files changed, 195 insertions(+), 54 deletions(-)

diff --git a/src/data_structures/bitset/dynamic_bitset.hpp b/src/data_structures/bitset/dynamic_bitset.hpp
index a4324bbad..1021f4792 100644
--- a/src/data_structures/bitset/dynamic_bitset.hpp
+++ b/src/data_structures/bitset/dynamic_bitset.hpp
@@ -6,36 +6,60 @@
 #include "threading/sync/spinlock.hpp"
 #include "utils/assert.hpp"
 
+/** A sequentially ordered non-unique
+ * lock-free concurrent collection of bits.
+ *
+ * Grows dynamically to accomodate the maximum
+ * set-bit position. Does not dynamically
+ * decrease in size.
+ *
+ * Bits can be set, retrieved and cleared
+ * in groups. Note that all group operations
+ * fail if the group spans multiple basic
+ * storage units. For example, if basic storage
+ * is in 8 bits, calling at(5, 2) is legal,
+ * but calling at(7, 2) is not.
+ *
+ * Organizes bits into chunks. Finding the
+ * right chunk has has O(n) lookup performance,
+ * so use large chunks for better speed in large
+ * bitsets. At the same time larger chunks
+ * mean coarser-grained memory allocation.
+ *
+ * @tparam block_t - Basic storage type. Must have
+ * lock-free atomic. Should probably always be uint_8.
+ * @tparam chunk_size - number of bits in one chunk
+ */
 template <class block_t = uint8_t, size_t chunk_size = 32768>
 class DynamicBitset : Lockable<SpinLock> {
+  // basic storage unit
   struct Block {
     Block() = default;
 
-    Block(Block &) = delete;
-    Block(Block &&) = delete;
-
+    // the number of bits in one Block
     static constexpr size_t size = sizeof(block_t) * 8;
 
+    block_t at(size_t k, size_t n) const {
+      debug_assert(k + n - 1 < size, "Invalid index.");
+      return (block.load() >> k) & bitmask(n);
+    }
+
+    void set(size_t k, size_t n) {
+      debug_assert(k + n - 1 < size, "Invalid index.");
+      block.fetch_or(bitmask(n) << k);
+    }
+
+    void clear(size_t k, size_t n) {
+      debug_assert(k + n - 1 < size, "Invalid index.");
+      block.fetch_and(~(bitmask(n) << k));
+    }
+
+   private:
+    std::atomic<block_t> block{0};
+
     constexpr block_t bitmask(size_t group_size) const {
       return (block_t)(-1) >> (size - group_size);
     }
-
-    block_t at(size_t k, size_t n, std::memory_order order) {
-      debug_assert(k + n - 1 < size, "Invalid index.");
-      return (block.load(order) >> k) & bitmask(n);
-    }
-
-    void set(size_t k, size_t n, std::memory_order order) {
-      debug_assert(k + n - 1 < size, "Invalid index.");
-      block.fetch_or(bitmask(n) << k, order);
-    }
-
-    void clear(size_t k, size_t n, std::memory_order order) {
-      debug_assert(k + n - 1 < size, "Invalid index.");
-      block.fetch_and(~(bitmask(n) << k), order);
-    }
-
-    std::atomic<block_t> block{0};
   };
 
   struct Chunk {
@@ -47,19 +71,21 @@ class DynamicBitset : Lockable<SpinLock> {
     Chunk(Chunk &) = delete;
     Chunk(Chunk &&) = delete;
 
+    // the number of bits in one chunk
     static constexpr size_t size = chunk_size * Block::size;
+    // the number of blocks in one chunk
     static constexpr size_t n_blocks = chunk_size / sizeof(block_t);
 
-    block_t at(size_t k, size_t n, std::memory_order order) {
-      return blocks[k / Block::size].at(k % Block::size, n, order);
+    block_t at(size_t k, size_t n) const {
+      return blocks[k / Block::size].at(k % Block::size, n);
     }
 
-    void set(size_t k, size_t n, std::memory_order order) {
-      blocks[k / Block::size].set(k % Block::size, n, order);
+    void set(size_t k, size_t n) {
+      blocks[k / Block::size].set(k % Block::size, n);
     }
 
-    void clear(size_t k, size_t n, std::memory_order order) {
-      blocks[k / Block::size].clear(k % Block::size, n, order);
+    void clear(size_t k, size_t n) {
+      blocks[k / Block::size].clear(k % Block::size, n);
     }
 
     Block blocks[n_blocks];
@@ -67,10 +93,14 @@ class DynamicBitset : Lockable<SpinLock> {
   };
 
  public:
-  DynamicBitset() : head(new Chunk()) {}
+  DynamicBitset(){};
 
-  DynamicBitset(DynamicBitset &) = delete;
+  // can't move nor copy a DynamicBitset because of atomic
+  // head and locking
+  DynamicBitset(const DynamicBitset &) = delete;
   DynamicBitset(DynamicBitset &&) = delete;
+  DynamicBitset &operator=(const DynamicBitset &) = delete;
+  DynamicBitset &operator=(DynamicBitset &&) = delete;
 
   ~DynamicBitset() {
     auto now = head.load();
@@ -81,28 +111,44 @@ class DynamicBitset : Lockable<SpinLock> {
     }
   }
 
-  block_t at(size_t k, size_t n) {
+  /** Gets the block of bit starting at bit k
+   * and containing the the following n bits.
+   * The bit index with k in this bitset is
+   * zeroth bit in the returned value.
+   */
+  block_t at(size_t k, size_t n) const {
+    if (k >= Chunk::size * chunk_count) return 0;
+
     auto &chunk = find_chunk(k);
-    return chunk.at(k, n, std::memory_order_seq_cst);
+    return chunk.at(k, n);
   }
 
-  bool at(size_t k) {
-    auto &chunk = find_chunk(k);
-    return chunk.at(k, 1, std::memory_order_seq_cst);
-  }
+  /** Returns k-th bit's value. */
+  bool at(size_t k) const { return at(k, 1); }
 
+  /** Set all the bits in the group of size `n`, starting from
+   * bit `k`.
+   */
   void set(size_t k, size_t n = 1) {
-    auto &chunk = find_chunk(k);
-    return chunk.set(k, n, std::memory_order_seq_cst);
+    auto &chunk = find_or_create_chunk(k);
+    return chunk.set(k, n);
   }
 
+  /** Clears all the bits in the group of size `n`, starting
+   * from bit `k`.
+   * */
   void clear(size_t k, size_t n = 1) {
-    auto &chunk = find_chunk(k);
-    return chunk.clear(k, n, std::memory_order_seq_cst);
+    // if desired bit is out of bounds, it's already clear
+    if (k >= Chunk::size * chunk_count) return;
+
+    auto &chunk = find_or_create_chunk(k);
+    return chunk.clear(k, n);
   }
 
  private:
-  Chunk &find_chunk(size_t &k) {
+  // finds the chunk to which k-th bit belong. if k is
+  // out of bounds, the chunk for it is created
+  Chunk &find_or_create_chunk(size_t &k) {
     Chunk *chunk = head.load(), *next = nullptr;
 
     // while i'm not in the right chunk
@@ -128,11 +174,40 @@ class DynamicBitset : Lockable<SpinLock> {
       if (chunk->next.load() != nullptr) continue;
 
       chunk->next.store(new Chunk());
+      chunk_count++;
     }
 
     debug_assert(chunk != nullptr, "Chunk is nullptr.");
     return *chunk;
   }
 
-  std::atomic<Chunk *> head;
+  // finds the chunk to which k-th bit belongs.
+  // fails if k is out of bounds
+  const Chunk &find_chunk(size_t &k) const {
+    debug_assert(k < chunk_size * Chunk::size, "Index out  of bounds");
+    Chunk *chunk = head.load(), *next = nullptr;
+
+    // while i'm not in the right chunk
+    // (my index is bigger than the size of this chunk)
+    while (k >= Chunk::size) {
+      next = chunk->next.load();
+
+      // if a next chunk exists, switch to it and decrement my
+      // pointer by the size of the current chunk
+      if (next != nullptr) {
+        chunk = next;
+        k -= Chunk::size;
+        continue;
+      }
+
+      // the next chunk does not exist, this is illegal state
+      permanent_fail("Out of bounds");
+    }
+
+    debug_assert(chunk != nullptr, "Chunk is nullptr.");
+    return *chunk;
+  }
+
+  std::atomic<Chunk *> head{new Chunk()};
+  std::atomic<int64_t> chunk_count{1};
 };
diff --git a/tests/unit/dynamic_bitset.cpp b/tests/unit/dynamic_bitset.cpp
index 24f3060e0..bf206a487 100644
--- a/tests/unit/dynamic_bitset.cpp
+++ b/tests/unit/dynamic_bitset.cpp
@@ -2,22 +2,88 @@
 
 #include "data_structures/bitset/dynamic_bitset.hpp"
 
-TEST(DynamicBitset, BasicFunctionality) {
+TEST(DynamicBitset, BasicAtAndSet) {
   DynamicBitset<> db;
-  db.set(222555, 1);
-  bool value = db.at(222555, 1);
-  ASSERT_EQ(value, true);
 
-  db.set(32, 1);
-  value = db.at(32, 1);
-  ASSERT_EQ(value, true);
-
-  db.clear(32, 1);
-  value = db.at(32, 1);
-  ASSERT_EQ(value, false);
+  EXPECT_EQ(db.at(17, 1), 0);
+  EXPECT_EQ(db.at(17), false);
+  db.set(17, 1);
+  EXPECT_EQ(db.at(17, 1), 1);
+  EXPECT_EQ(db.at(17), true);
 }
 
-int main(int argc, char **argv) {
-  ::testing::InitGoogleTest(&argc, argv);
-  return RUN_ALL_TESTS();
+TEST(DynamicBitset, GroupAt) {
+  DynamicBitset<> db;
+
+  db.set(0, 1);
+  db.set(1, 1);
+  EXPECT_EQ(db.at(0, 2), 1 | 2);
+  db.set(3, 1);
+  EXPECT_EQ(db.at(0, 2), 1 | 2);
+  EXPECT_EQ(db.at(0, 3), 1 | 2);
+  EXPECT_EQ(db.at(0, 4), 1 | 2 | 8);
+  EXPECT_EQ(db.at(1, 1), 1);
+  EXPECT_EQ(db.at(1, 2), 1);
+  EXPECT_EQ(db.at(1, 3), 1 | 4);
+}
+
+TEST(DynamicBitset, GroupSet) {
+  DynamicBitset<> db;
+  EXPECT_EQ(db.at(0, 3), 0);
+  db.set(1, 2);
+  EXPECT_FALSE(db.at(0));
+  EXPECT_TRUE(db.at(1));
+  EXPECT_TRUE(db.at(2));
+  EXPECT_FALSE(db.at(3));
+}
+
+class Clear : public ::testing::Test {
+ protected:
+  DynamicBitset<> db;
+
+  void SetUp() override {
+    db.set(17, 1);
+    db.set(18, 1);
+    EXPECT_EQ(db.at(17), true);
+    EXPECT_EQ(db.at(18), true);
+  }
+};
+
+TEST_F(Clear, OneElement) {
+  db.clear(17, 1);
+  EXPECT_EQ(db.at(17), false);
+  EXPECT_EQ(db.at(18), true);
+}
+
+TEST_F(Clear, Group) {
+  db.clear(17, 2);
+  EXPECT_EQ(db.at(17), false);
+  EXPECT_EQ(db.at(18), false);
+}
+
+TEST_F(Clear, EmptyGroup) {
+  db.clear(17, 0);
+  EXPECT_EQ(db.at(17), true);
+  EXPECT_EQ(db.at(18), true);
+}
+
+TEST(DynamicBitset, ConstBitset) {
+  auto const_accepting = [](const DynamicBitset<> &cdbs) {
+    EXPECT_FALSE(cdbs.at(16));
+    EXPECT_TRUE(cdbs.at(17));
+    EXPECT_FALSE(cdbs.at(18));
+  };
+
+  DynamicBitset<> dbs;
+  dbs.set(17);
+  const_accepting(dbs);
+}
+
+TEST(DynamicBitset, GroupAcrossBlockFail) {
+  DynamicBitset<uint8_t> db;
+  // groups must be aligned to block_t
+  db.set(8, 1);
+  EXPECT_DEATH(db.at(7, 2), "Invalid index");
+  EXPECT_DEATH(db.set(7, 2), "Invalid index");
+  EXPECT_DEATH(db.clear(7, 2), "Invalid index");
 }