Expose faster edge_accessor constructor

Summary:
Fix nullptr exceptions

Inline edge_type and from/to in edge accessor

Reviewers: florijan, buda

Reviewed By: florijan

Subscribers: pullbot

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

src/database/graph_db_accessor.cpp

@@ -306,7 +306,7 @@ EdgeAccessor GraphDbAccessor::InsertEdge(
 
   db_.wal_.CreateEdge(transaction_->id_, edge_vlist->id_, from.vlist_->id_,
                       to.vlist_->id_, EdgeTypeName(edge_type));
-  return EdgeAccessor(*edge_vlist, *this);
+  return EdgeAccessor(*edge_vlist, *this, from.vlist_, to.vlist_, edge_type);
 }
 
 int64_t GraphDbAccessor::EdgesCount() const {
@@ -321,7 +321,7 @@ void GraphDbAccessor::RemoveEdge(EdgeAccessor &edge_accessor,
   // due to it getting matched multiple times by some patterns
   // we can only delete it once, so check if it's already deleted
   edge_accessor.SwitchNew();
-  if (edge_accessor.current_->is_expired_by(*transaction_)) return;
+  if (edge_accessor.current().is_expired_by(*transaction_)) return;
   if (remove_from_from)
     edge_accessor.from().update().out_.RemoveEdge(edge_accessor.vlist_);
   if (remove_from_to)

src/durability/snapshooter.cpp

@@ -101,7 +101,7 @@ void RemoveOldWals(const fs::path &wal_dir,
     if (tx_id && tx_id.value() < min_trans_id) fs::remove(wal_file);
   }
 }
-}  // annonnymous namespace
+}  // namespace
 
 fs::path MakeSnapshotPath(const fs::path &durability_dir) {
   std::string date_str =

src/mvcc/record.hpp

@@ -134,7 +134,7 @@ class Record : public Version<T> {
    * True if this record is expired in the current command
    * of the given transaction.
    */
-  bool is_expired_by(const tx::Transaction &t) {
+  bool is_expired_by(const tx::Transaction &t) const {
     return tx_.exp == t.id_ && cmd_.exp == t.cid();
   }
 

src/storage/address.hpp

@@ -81,4 +81,4 @@ class Address {
  private:
   uintptr_t storage_{0};
 };
-}
+}  // namespace storage

src/storage/edge_accessor.cpp

@@ -4,34 +4,49 @@
 #include "storage/vertex_accessor.hpp"
 #include "utils/algorithm.hpp"
 
-GraphDbTypes::EdgeType EdgeAccessor::EdgeType() const {
-  return current().edge_type_;
-}
+GraphDbTypes::EdgeType EdgeAccessor::EdgeType() const { return edge_type_; }
 
 VertexAccessor EdgeAccessor::from() const {
-  return VertexAccessor(*current().from_.local(), db_accessor());
+  return VertexAccessor(*from_.local(), db_accessor());
 }
 
 bool EdgeAccessor::from_is(const VertexAccessor &v) const {
-  return v == current().from_.local();
+  return v == from_.local();
 }
 
 VertexAccessor EdgeAccessor::to() const {
-  return VertexAccessor(*current().to_.local(), db_accessor());
+  return VertexAccessor(*to_.local(), db_accessor());
 }
 
 bool EdgeAccessor::to_is(const VertexAccessor &v) const {
-  return v == current().to_.local();
+  return v == to_.local();
 }
 
-bool EdgeAccessor::is_cycle() const { return current().to_ == current().from_; }
+bool EdgeAccessor::is_cycle() const { return to_ == from_; }
 
 std::ostream &operator<<(std::ostream &os, const EdgeAccessor &ea) {
   os << "E[" << ea.db_accessor().EdgeTypeName(ea.EdgeType());
   os << " {";
-  utils::PrintIterable(os, ea.Properties(), ", ", [&](auto &stream,
-                                                      const auto &pair) {
-    stream << ea.db_accessor().PropertyName(pair.first) << ": " << pair.second;
+  utils::PrintIterable(os, ea.Properties(), ", ",
+                       [&](auto &stream, const auto &pair) {
+                         stream << ea.db_accessor().PropertyName(pair.first)
+                                << ": " << pair.second;
                        });
   return os << "}]";
 }
+
+const Edge &EdgeAccessor::current() {
+  if (current_ == nullptr) RecordAccessor::Reconstruct();
+  return *current_;
+}
+
+const PropertyValueStore<GraphDbTypes::Property> &EdgeAccessor::Properties()
+    const {
+  if (current_ == nullptr) RecordAccessor::Reconstruct();
+  return RecordAccessor::Properties();
+}
+
+const PropertyValue &EdgeAccessor::PropsAt(GraphDbTypes::Property key) const {
+  if (current_ == nullptr) RecordAccessor::Reconstruct();
+  return RecordAccessor::PropsAt(key);
+}

src/storage/edge_accessor.hpp

@@ -15,8 +15,35 @@ class VertexAccessor;
  * takes care of MVCC versioning.
  */
 class EdgeAccessor : public RecordAccessor<Edge> {
+  using VertexAddress = storage::Address<mvcc::VersionList<Vertex>>;
+
  public:
-  using RecordAccessor::RecordAccessor;
+  /**
+   * Create a new EdgeAccessor and reads data from edge mvcc
+   */
+  EdgeAccessor(mvcc::VersionList<Edge> &edge, GraphDbAccessor &db_accessor)
+      : RecordAccessor(edge, db_accessor),
+        from_(nullptr),
+        to_(nullptr),
+        edge_type_() {
+    RecordAccessor::Reconstruct();
+    if (current_ != nullptr) {
+      from_ = current_->from_;
+      to_ = current_->to_;
+      edge_type_ = current_->edge_type_;
+    }
+  }
+
+  /**
+   * Create a new EdgeAccessor without invoking mvcc methods
+   */
+  EdgeAccessor(mvcc::VersionList<Edge> &edge, GraphDbAccessor &db_accessor,
+               VertexAddress from, VertexAddress to,
+               GraphDbTypes::EdgeType edge_type)
+      : RecordAccessor(edge, db_accessor),
+        from_(from),
+        to_(to),
+        edge_type_(edge_type) {}
 
   /**
    * Returns the edge type.
@@ -46,6 +73,24 @@ class EdgeAccessor : public RecordAccessor<Edge> {
   /** Returns true if this edge is a cycle (start and end node are
    * the same. */
   bool is_cycle() const;
+
+  /** Returns current edge
+   */
+  const Edge &current();
+
+  /** Returns edge properties
+   */
+  const PropertyValueStore<GraphDbTypes::Property> &Properties() const;
+
+  /* Returns property at key.
+   * @param key - Property key
+   */
+  const PropertyValue &PropsAt(GraphDbTypes::Property key) const;
+
+ private:
+  VertexAddress from_;
+  VertexAddress to_;
+  GraphDbTypes::EdgeType edge_type_;
 };
 
 std::ostream &operator<<(std::ostream &, const EdgeAccessor &);

src/storage/record_accessor.cpp

@@ -8,9 +8,7 @@
 template <typename TRecord>
 RecordAccessor<TRecord>::RecordAccessor(mvcc::VersionList<TRecord> &vlist,
                                         GraphDbAccessor &db_accessor)
-    : vlist_(&vlist), db_accessor_(&db_accessor) {
-  Reconstruct();
-}
+    : vlist_(&vlist), db_accessor_(&db_accessor) {}
 
 template <typename TRecord>
 const PropertyValue &RecordAccessor<TRecord>::PropsAt(
@@ -107,7 +105,7 @@ RecordAccessor<TRecord> &RecordAccessor<TRecord>::SwitchOld() {
 }
 
 template <typename TRecord>
-bool RecordAccessor<TRecord>::Reconstruct() {
+bool RecordAccessor<TRecord>::Reconstruct() const {
   vlist_->find_set_old_new(db_accessor_->transaction(), old_, new_);
   current_ = old_ ? old_ : new_;
   return old_ != nullptr || new_ != nullptr;
@@ -117,7 +115,13 @@ bool RecordAccessor<TRecord>::Reconstruct() {
 }
 
 template <typename TRecord>
-TRecord &RecordAccessor<TRecord>::update() {
+TRecord &RecordAccessor<TRecord>::update() const {
+  // If the current is not set we probably created the accessor with a lazy
+  // constructor which didn't call Reconstruct on creation
+  if (!current_) {
+    bool reconstructed = Reconstruct();
+    DCHECK(reconstructed) << "Unable to initialize record";
+  }
   auto &t = db_accessor_->transaction();
   // can't update a deleted record if:
   // - we only have old_ and it hasn't been deleted

src/storage/record_accessor.hpp

@@ -145,7 +145,7 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    @return True if this accessor is valid after reconstruction.  This means that
    at least one record pointer was found (either new_ or old_), possibly both.
    */
-  bool Reconstruct();
+  bool Reconstruct() const;
 
  protected:
   /**
@@ -156,7 +156,7 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    * It is not legal to call this function on a Vertex/Edge that has been
    * deleted in the current transaction+command.
    */
-  TRecord &update();
+  TRecord &update() const;
 
   /**
    * Returns the current version (either new_ or old_)
@@ -166,6 +166,16 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    */
   const TRecord &current() const;
 
+  /**
+   * Pointer to the version (either old_ or new_) that READ operations
+   * in the accessor should take data from. Note that WRITE operations
+   * should always use new_.
+   *
+   * This pointer can be null if created by an accessor which lazily reads from
+   * mvcc.
+   */
+  mutable TRecord *current_{nullptr};
+
   // The record (edge or vertex) this accessor provides access to.
   // Immutable, set in the constructor and never changed.
   mvcc::VersionList<TRecord> *vlist_;
@@ -183,7 +193,7 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    * Can be null only when the record itself (the version-list) has
    * been created by the current transaction+command.
    */
-  TRecord *old_{nullptr};
+  mutable TRecord *old_{nullptr};
 
   /**
    * Version that has been modified (created or updated) by the current
@@ -196,14 +206,5 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
    * is null, and if it is it must check with the vlist_ if there is
    * an update.
    */
-  TRecord *new_{nullptr};
-
-  /**
-   * Pointer to the version (either old_ or new_) that READ operations
-   * in the accessor should take data from. Note that WRITE operations
-   * should always use new_.
-   *
-   * This pointer can never ever be null.
-   */
-  TRecord *current_{nullptr};
+  mutable TRecord *new_{nullptr};
 };

src/storage/vertex_accessor.hpp

@@ -20,21 +20,42 @@
  * takes care of MVCC versioning.
  */
 class VertexAccessor : public RecordAccessor<Vertex> {
+  using VertexAddress = storage::Address<mvcc::VersionList<Vertex>>;
   // Helper function for creating an iterator over edges.
+  // @param begin - begin iterator
+  // @param end - end iterator
+  // @param from - if true specifies that the vertex represents `from` part of
+  // the edge, otherwise it specifies `to` part of the edge
+  // @param vertex - one endpoint of every edge
+  // @param db_accessor - database accessor
+  // @return - Iterator over EdgeAccessors
   template <typename TIterator>
   static inline auto MakeAccessorIterator(TIterator &&begin, TIterator &&end,
+                                          bool from, VertexAddress vertex,
                                           GraphDbAccessor &db_accessor) {
     return iter::imap(
-        [&db_accessor](auto &edges_element) {
+        [from, vertex, &db_accessor](auto &edges_element) {
           // Currently only local storage is supported.
-          return EdgeAccessor(*edges_element.edge.local(), db_accessor);
+          if (from) {
+            return EdgeAccessor(*edges_element.edge.local(), db_accessor,
+                                vertex, edges_element.vertex,
+                                edges_element.edge_type);
+          } else {
+            return EdgeAccessor(*edges_element.edge.local(), db_accessor,
+                                edges_element.vertex, vertex,
+                                edges_element.edge_type);
+          }
         },
         utils::Iterable<TIterator>(std::forward<TIterator>(begin),
                                    std::forward<TIterator>(end)));
   }
 
  public:
-  using RecordAccessor::RecordAccessor;
+  VertexAccessor(mvcc::VersionList<Vertex> &vertex,
+                 GraphDbAccessor &db_accessor)
+      : RecordAccessor(vertex, db_accessor) {
+    RecordAccessor::Reconstruct();
+  }
 
   /**
    * Returns the number of outgoing edges.
@@ -81,7 +102,7 @@ class VertexAccessor : public RecordAccessor<Vertex> {
    */
   auto in() const {
     return MakeAccessorIterator(current().in_.begin(), current().in_.end(),
-                                db_accessor());
+                                false, vlist_, db_accessor());
   }
 
   /**
@@ -95,7 +116,8 @@ class VertexAccessor : public RecordAccessor<Vertex> {
       const VertexAccessor &dest,
       const std::vector<GraphDbTypes::EdgeType> *edge_types = nullptr) const {
     return MakeAccessorIterator(current().in_.begin(dest.vlist_, edge_types),
-                                current().in_.end(), db_accessor());
+                                current().in_.end(), false, vlist_,
+                                db_accessor());
   }
 
   /**
@@ -106,7 +128,8 @@ class VertexAccessor : public RecordAccessor<Vertex> {
    */
   auto in(const std::vector<GraphDbTypes::EdgeType> *edge_types) const {
     return MakeAccessorIterator(current().in_.begin(nullptr, edge_types),
-                                current().in_.end(), db_accessor());
+                                current().in_.end(), false, vlist_,
+                                db_accessor());
   }
 
   /**
@@ -114,7 +137,7 @@ class VertexAccessor : public RecordAccessor<Vertex> {
    */
   auto out() const {
     return MakeAccessorIterator(current().out_.begin(), current().out_.end(),
-                                db_accessor());
+                                true, vlist_, db_accessor());
   }
 
   /**
@@ -129,7 +152,8 @@ class VertexAccessor : public RecordAccessor<Vertex> {
       const VertexAccessor &dest,
       const std::vector<GraphDbTypes::EdgeType> *edge_types = nullptr) const {
     return MakeAccessorIterator(current().out_.begin(dest.vlist_, edge_types),
-                                current().out_.end(), db_accessor());
+                                current().out_.end(), true, vlist_,
+                                db_accessor());
   }
 
   /**
@@ -140,7 +164,8 @@ class VertexAccessor : public RecordAccessor<Vertex> {
    */
   auto out(const std::vector<GraphDbTypes::EdgeType> *edge_types) const {
     return MakeAccessorIterator(current().out_.begin(nullptr, edge_types),
-                                current().out_.end(), db_accessor());
+                                current().out_.end(), true, vlist_,
+                                db_accessor());
   }
 };
 
@@ -152,4 +177,4 @@ template <>
 struct hash<VertexAccessor> {
   size_t operator()(const VertexAccessor &v) const { return v.id(); };
 };
-}
+}  // namespace std

src/transactions/transaction.hpp

@@ -92,4 +92,4 @@ class Transaction {
   const std::chrono::time_point<std::chrono::steady_clock> creation_time_{
       std::chrono::steady_clock::now()};
 };
-}
+}  // namespace tx