Lower DeferredDeleter memory allocations

Reviewers: teon.banek, msantl

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1688

src/storage/distributed/deferred_deleter.hpp

@@ -16,17 +16,13 @@
  */
 template <typename T>
 class DeferredDeleter {
- public:
-  /**
-   * @brief - keep track of what object was deleted at which time.
-   */
+ private:
   struct DeletedObject {
-    const T *object;
-    const tx::TransactionId deleted_at;
-    DeletedObject(const T *object, tx::TransactionId deleted_at)
-        : object(object), deleted_at(deleted_at) {}
+    T *object;
+    tx::TransactionId deleted_at;
   };
 
+ public:
   /**
    * @brief - check if everything is freed
    */
@@ -36,22 +32,15 @@ class DeferredDeleter {
   }
 
   /**
-   * @brief - Add objects to this deleter. This method assumes that it will
+   * @brief - Add object to this deleter. This method assumes that it will
    * always be called with a non-decreasing sequence of `deleted_at`.
-   * @param objects - vector of objects to add
-   * @param last_transaction - nothing newer or equal to it can see these
-   * objects
+   * @param object - object to add
+   * @param deleted_at - when was the object deleted
    */
-  void AddObjects(const std::vector<DeletedObject> &objects) {
-    auto previous_tx_id = objects_.empty()
-                              ? std::numeric_limits<tx::TransactionId>::min()
-                              : objects_.back().deleted_at;
-    for (auto object : objects) {
-      CHECK(previous_tx_id <= object.deleted_at)
-          << "deleted_at must be non-decreasing";
-      previous_tx_id = object.deleted_at;
-      objects_.push_back(object);
-    }
+  void AddObject(T *object, tx::TransactionId deleted_at) {
+    CHECK(previous_tx_id_ <= deleted_at) << "deleted_at must be non-decreasing";
+    previous_tx_id_ = deleted_at;
+    objects_.push_back(DeletedObject{object, deleted_at});
   }
 
   /**
@@ -75,4 +64,6 @@ class DeferredDeleter {
  private:
   // Ascendingly sorted list of deleted objects by `deleted_at`.
   std::list<DeletedObject> objects_;
+  // Last transaction ID that had deleted objects
+  tx::TransactionId previous_tx_id_{0};
 };

src/storage/distributed/garbage_collector.hpp

@@ -34,33 +34,22 @@ class GarbageCollector {
   void Run(const tx::Snapshot &snapshot, const tx::Engine &engine) {
     auto collection_accessor = collection_.access();
     uint64_t count = 0;
-    std::vector<typename DeferredDeleter<TRecord>::DeletedObject>
-        deleted_records;
-    std::vector<
-        typename DeferredDeleter<mvcc::VersionList<TRecord>>::DeletedObject>
-        deleted_version_lists;
     for (auto id_vlist : collection_accessor) {
       mvcc::VersionList<TRecord> *vlist = id_vlist.second;
       // If the version_list is empty, i.e. there is nothing else to be read
       // from it we can delete it.
       auto ret = vlist->GcDeleted(snapshot, engine);
       if (ret.first) {
-        deleted_version_lists.emplace_back(vlist, engine.LocalLast());
+        version_list_deleter_.AddObject(vlist, engine.LocalLast());
         count += collection_accessor.remove(id_vlist.first);
       }
-      if (ret.second != nullptr)
-        deleted_records.emplace_back(ret.second, engine.LocalLast());
+      if (ret.second != nullptr) {
+        record_deleter_.AddObject(ret.second, engine.LocalLast());
+      }
     }
     DLOG_IF(INFO, count > 0)
         << "GC started cleaning with snapshot: " << snapshot;
     DLOG_IF(INFO, count > 0) << "Destroyed: " << count;
-
-    // Add records to deleter, with the id larger or equal than the last active
-    // transaction.
-    record_deleter_.AddObjects(deleted_records);
-    // Add version_lists to deleter, with the id larger or equal than the last
-    // active transaction.
-    version_list_deleter_.AddObjects(deleted_version_lists);
   }
 
  private:

src/storage/single_node/deferred_deleter.hpp

@@ -16,17 +16,13 @@
  */
 template <typename T>
 class DeferredDeleter {
- public:
-  /**
-   * @brief - keep track of what object was deleted at which time.
-   */
+ private:
   struct DeletedObject {
-    const T *object;
-    const tx::TransactionId deleted_at;
-    DeletedObject(const T *object, tx::TransactionId deleted_at)
-        : object(object), deleted_at(deleted_at) {}
+    T *object;
+    tx::TransactionId deleted_at;
   };
 
+ public:
   /**
    * @brief - check if everything is freed
    */
@@ -36,22 +32,15 @@ class DeferredDeleter {
   }
 
   /**
-   * @brief - Add objects to this deleter. This method assumes that it will
+   * @brief - Add object to this deleter. This method assumes that it will
    * always be called with a non-decreasing sequence of `deleted_at`.
-   * @param objects - vector of objects to add
-   * @param last_transaction - nothing newer or equal to it can see these
-   * objects
+   * @param object - object to add
+   * @param deleted_at - when was the object deleted
    */
-  void AddObjects(const std::vector<DeletedObject> &objects) {
-    auto previous_tx_id = objects_.empty()
-                              ? std::numeric_limits<tx::TransactionId>::min()
-                              : objects_.back().deleted_at;
-    for (auto object : objects) {
-      CHECK(previous_tx_id <= object.deleted_at)
-          << "deleted_at must be non-decreasing";
-      previous_tx_id = object.deleted_at;
-      objects_.push_back(object);
-    }
+  void AddObject(T *object, tx::TransactionId deleted_at) {
+    CHECK(previous_tx_id_ <= deleted_at) << "deleted_at must be non-decreasing";
+    previous_tx_id_ = deleted_at;
+    objects_.push_back(DeletedObject{object, deleted_at});
   }
 
   /**
@@ -75,4 +64,6 @@ class DeferredDeleter {
  private:
   // Ascendingly sorted list of deleted objects by `deleted_at`.
   std::list<DeletedObject> objects_;
+  // Last transaction ID that had deleted objects
+  tx::TransactionId previous_tx_id_{0};
 };

src/storage/single_node/garbage_collector.hpp

@@ -34,33 +34,22 @@ class GarbageCollector {
   void Run(const tx::Snapshot &snapshot, const tx::Engine &engine) {
     auto collection_accessor = collection_.access();
     uint64_t count = 0;
-    std::vector<typename DeferredDeleter<TRecord>::DeletedObject>
-        deleted_records;
-    std::vector<
-        typename DeferredDeleter<mvcc::VersionList<TRecord>>::DeletedObject>
-        deleted_version_lists;
     for (auto id_vlist : collection_accessor) {
       mvcc::VersionList<TRecord> *vlist = id_vlist.second;
       // If the version_list is empty, i.e. there is nothing else to be read
       // from it we can delete it.
       auto ret = vlist->GcDeleted(snapshot, engine);
       if (ret.first) {
-        deleted_version_lists.emplace_back(vlist, engine.LocalLast());
+        version_list_deleter_.AddObject(vlist, engine.LocalLast());
         count += collection_accessor.remove(id_vlist.first);
       }
-      if (ret.second != nullptr)
-        deleted_records.emplace_back(ret.second, engine.LocalLast());
+      if (ret.second != nullptr) {
+        record_deleter_.AddObject(ret.second, engine.LocalLast());
+      }
     }
     DLOG_IF(INFO, count > 0)
         << "GC started cleaning with snapshot: " << snapshot;
     DLOG_IF(INFO, count > 0) << "Destroyed: " << count;
-
-    // Add records to deleter, with the id larger or equal than the last active
-    // transaction.
-    record_deleter_.AddObjects(deleted_records);
-    // Add version_lists to deleter, with the id larger or equal than the last
-    // active transaction.
-    version_list_deleter_.AddObjects(deleted_version_lists);
   }
 
  private:

tests/unit/deferred_deleter.cpp

@@ -10,10 +10,8 @@
 TEST(DeferredDeleter, AddObjects) {
   DeferredDeleter<Vertex> deleter;
   for (int i = 0; i < 10; ++i) {
-    std::vector<DeferredDeleter<Vertex>::DeletedObject> v;
-    v.emplace_back(new Vertex, 5);
-    v.emplace_back(new Vertex, 5);
-    deleter.AddObjects(v);
+    deleter.AddObject(new Vertex, 5);
+    deleter.AddObject(new Vertex, 5);
     EXPECT_EQ(deleter.Count(), (i + 1) * 2);
   }
   deleter.FreeExpiredObjects(tx::Transaction::MaxId());
@@ -24,10 +22,8 @@ TEST(DeferredDeleter, Destructor) {
   std::atomic<int> count{0};
   DeferredDeleter<DestrCountRec> *deleter = new DeferredDeleter<DestrCountRec>;
   for (int i = 0; i < 10; ++i) {
-    std::vector<DeferredDeleter<DestrCountRec>::DeletedObject> v;
-    v.emplace_back(new DestrCountRec(count), 5);
-    v.emplace_back(new DestrCountRec(count), 5);
-    deleter->AddObjects(v);
+    deleter->AddObject(new DestrCountRec(count), 5);
+    deleter->AddObject(new DestrCountRec(count), 5);
     EXPECT_EQ(deleter->Count(), (i + 1) * 2);
   }
   EXPECT_EQ(0, count);
@@ -40,11 +36,9 @@ TEST(DeferredDeleter, Destructor) {
 // Check if deleter frees objects.
 TEST(DeferredDeleter, FreeExpiredObjects) {
   DeferredDeleter<DestrCountRec> deleter;
-  std::vector<DeferredDeleter<DestrCountRec>::DeletedObject> v;
   std::atomic<int> count{0};
-  v.emplace_back(new DestrCountRec(count), 5);
-  v.emplace_back(new DestrCountRec(count), 5);
-  deleter.AddObjects(v);
+  deleter.AddObject(new DestrCountRec(count), 5);
+  deleter.AddObject(new DestrCountRec(count), 5);
 
   deleter.FreeExpiredObjects(5);
   EXPECT_EQ(deleter.Count(), 2);