Fix expansion benchmark

Reviewers: florijan, mferencevic, teon.banek, buda

Reviewed By: florijan

Subscribers: pullbot

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

src/data_structures/concurrent/skiplist_gc.hpp

@@ -49,6 +49,12 @@ class SkipListGC {
    * @brief - Returns instance of executioner shared between all SkipLists.
    */
   auto &GetExecutioner() {
+    // TODO: Even though executioner is static, there are multiple instance:
+    // one for each TNode param type. We probably don't want that kind of
+    // behavior. Static variables with nontrivial destructor create subtle bugs
+    // because of their order of destruction. For example of one bug take a look
+    // at documentation in database/dbms.hpp. Rethink ownership and lifetime of
+    // executioner.
     static Executioner executioner(
         (std::chrono::seconds(FLAGS_skiplist_gc_interval)));
 

src/database/dbms.hpp

@@ -15,6 +15,27 @@ DECLARE_string(snapshot_directory);
 DECLARE_bool(recover_on_startup);
 
 namespace fs = std::experimental::filesystem;
+
+// Always be sure that Dbms object is destructed before main exits, i. e. Dbms
+// object shouldn't be part of global/static variable, except if its destructor
+// is explicitly called before main exits.
+// Consider code:
+//
+// Dbms dbms;  // KeyIndex is created as a part of dbms.
+// int main() {
+//   auto dba = dbms.active();
+//   auto v = dba->InsertVertex();
+//   v.add_label(dba->Label(
+//       "Start"));  // New SkipList is created in KeyIndex for LabelIndex.
+//                   // That SkipList creates SkipListGc which
+//                   // initialises static Executioner object.
+//   return 0;
+// }
+//
+// After main exits: 1. Executioner is destructed, 2. KeyIndex is destructed.
+// Destructor of KeyIndex calls delete on created SkipLists which destroy
+// SkipListGc that tries to use Excutioner object that doesn't exist anymore.
+// -> CRASH
 class Dbms {
  public:
   Dbms() {

tests/benchmark/expansion.cpp

@@ -9,12 +9,13 @@
 
 class ExpansionBenchFixture : public benchmark::Fixture {
  protected:
+  // Dbms shouldn't be global constructed/destructed. See documentation in
+  // database/dbms.hpp for details.
   std::experimental::optional<Dbms> dbms_;
   query::Interpreter interpeter_;
 
   void SetUp(const benchmark::State &state) override {
-    if (!dbms_)
-      dbms_.emplace();
+    dbms_.emplace();
     auto dba = dbms_->active();
     for (int i = 0; i < state.range(0); i++) dba->InsertVertex();
 
@@ -33,22 +34,23 @@ class ExpansionBenchFixture : public benchmark::Fixture {
     auto dba = dbms_->active();
     for (auto vertex : dba->Vertices(false)) dba->DetachRemoveVertex(vertex);
     dba->Commit();
+    dbms_ = std::experimental::nullopt;
   }
 };
 
-// BENCHMARK_DEFINE_F(ExpansionBenchFixture, Match)(benchmark::State &state) {
-//   auto query = "MATCH (s:Start) return s";
-//   auto dba = dbms_->active();
-//   while (state.KeepRunning()) {
-//     ResultStreamFaker results;
-//     interpeter_.Interpret(query, *dba, results, {});
-//   }
-// }
-// 
-// BENCHMARK_REGISTER_F(ExpansionBenchFixture, Match)
-//     ->RangeMultiplier(1024)
-//     ->Range(1, 1 << 20)
-//     ->Unit(benchmark::kMillisecond);
+BENCHMARK_DEFINE_F(ExpansionBenchFixture, Match)(benchmark::State &state) {
+  auto query = "MATCH (s:Start) return s";
+  auto dba = dbms_->active();
+  while (state.KeepRunning()) {
+    ResultStreamFaker results;
+    interpeter_.Interpret(query, *dba, results, {});
+  }
+}
+
+BENCHMARK_REGISTER_F(ExpansionBenchFixture, Match)
+    ->RangeMultiplier(1024)
+    ->Range(1, 1 << 20)
+    ->Unit(benchmark::kMillisecond);
 
 BENCHMARK_DEFINE_F(ExpansionBenchFixture, Expand)(benchmark::State &state) {
   auto query = "MATCH (s:Start) WITH s MATCH (s)--(d) RETURN count(d)";