From a0ec95e67637df182977c0cd389bba364b96facb Mon Sep 17 00:00:00 2001 From: jimingquan Date: Tue, 8 Nov 2022 17:31:16 +0800 Subject: [PATCH] shallow copy vertex & edgee (#1565) * shallow copy vertex & edgee * add delete * fix error * delete error code * optimize getVertex * fix memory leak * delete useless code * address comment * fix multiple threads double free pointer problem --- src/common/datatypes/Edge.h | 12 +++++ src/common/datatypes/Path.h | 2 +- src/common/datatypes/Value.cpp | 82 ++++++++++++++++++----------- src/common/datatypes/Value.h | 14 +++-- src/common/datatypes/ValueOps-inl.h | 16 +++--- src/common/datatypes/Vertex.h | 12 +++++ src/graph/context/Iterator.cpp | 12 +++-- src/graph/context/Iterator.h | 9 ++-- 8 files changed, 104 insertions(+), 55 deletions(-) diff --git a/src/common/datatypes/Edge.h b/src/common/datatypes/Edge.h index 4f9973538bd..e873c62689a 100644 --- a/src/common/datatypes/Edge.h +++ b/src/common/datatypes/Edge.h @@ -20,6 +20,7 @@ struct Edge { std::string name; EdgeRanking ranking; std::unordered_map props; + std::atomic refcnt{1}; Edge() {} Edge(Edge&& v) noexcept @@ -44,6 +45,13 @@ struct Edge { ranking(std::move(r)), props(std::move(p)) {} + size_t ref() { + return ++refcnt; + } + size_t unref() { + return --refcnt; + } + void clear(); void __clear() { @@ -57,6 +65,10 @@ struct Edge { bool operator==(const Edge& rhs) const; + bool operator!=(const Edge& rhs) const { + return !(*this == rhs); + } + void format() { if (type < 0) { reverse(); diff --git a/src/common/datatypes/Path.h b/src/common/datatypes/Path.h index f0ac650b2f4..5dbedb3b9d0 100644 --- a/src/common/datatypes/Path.h +++ b/src/common/datatypes/Path.h @@ -91,7 +91,7 @@ struct Step { if (dst != rhs.dst) { return dst < rhs.dst; } - if (type != rhs.dst) { + if (type != rhs.type) { return type < rhs.type; } if (ranking != rhs.ranking) { diff --git a/src/common/datatypes/Value.cpp b/src/common/datatypes/Value.cpp index c49adf90290..324756777f1 100644 --- a/src/common/datatypes/Value.cpp +++ b/src/common/datatypes/Value.cpp @@ -463,9 +463,9 @@ void Value::setVertex(Vertex&& v) { setV(std::move(v)); } -void Value::setVertex(std::unique_ptr&& v) { +void Value::setVertex(Vertex* v) { clear(); - setV(std::move(v)); + setV(v); } void Value::setEdge(const Edge& v) { @@ -478,9 +478,9 @@ void Value::setEdge(Edge&& v) { setE(std::move(v)); } -void Value::setEdge(std::unique_ptr&& v) { +void Value::setEdge(Edge* v) { clear(); - setE(std::move(v)); + setE(v); } void Value::setPath(const Path& v) { @@ -620,7 +620,7 @@ const Vertex& Value::getVertex() const { const Vertex* Value::getVertexPtr() const { CHECK_EQ(type_, Type::VERTEX); - return value_.vVal.get(); + return value_.vVal; } const Edge& Value::getEdge() const { @@ -630,7 +630,7 @@ const Edge& Value::getEdge() const { const Edge* Value::getEdgePtr() const { CHECK_EQ(type_, Type::EDGE); - return value_.eVal.get(); + return value_.eVal; } const Path& Value::getPath() const { @@ -945,11 +945,23 @@ void Value::clearSlow() { break; } case Type::VERTEX: { - destruct(value_.vVal); + if (value_.vVal) { + if (value_.vVal->unref() == 0) { + delete value_.vVal; + } + value_.vVal = nullptr; + type_ = Type::__EMPTY__; + } break; } case Type::EDGE: { - destruct(value_.eVal); + if (value_.eVal) { + if (value_.eVal->unref() == 0) { + delete value_.eVal; + } + value_.eVal = nullptr; + type_ = Type::__EMPTY__; + } break; } case Type::PATH: { @@ -1026,12 +1038,18 @@ Value& Value::operator=(Value&& rhs) noexcept { break; } case Type::VERTEX: { - setV(std::move(rhs.value_.vVal)); - break; + value_.vVal = rhs.value_.vVal; + type_ = Type::VERTEX; + rhs.value_.vVal = nullptr; + rhs.type_ = Type::__EMPTY__; + return *this; } case Type::EDGE: { - setE(std::move(rhs.value_.eVal)); - break; + value_.eVal = rhs.value_.eVal; + type_ = Type::EDGE; + rhs.value_.eVal = nullptr; + rhs.type_ = Type::__EMPTY__; + return *this; } case Type::PATH: { setP(std::move(rhs.value_.pVal)); @@ -1246,44 +1264,36 @@ void Value::setDT(DateTime&& v) { new (std::addressof(value_.dtVal)) DateTime(std::move(v)); } -void Value::setV(const std::unique_ptr& v) { - type_ = Type::VERTEX; - new (std::addressof(value_.vVal)) std::unique_ptr(new Vertex(*v)); -} - -void Value::setV(std::unique_ptr&& v) { +void Value::setV(Vertex* v) { type_ = Type::VERTEX; - new (std::addressof(value_.vVal)) std::unique_ptr(std::move(v)); + value_.vVal = v; + value_.vVal->ref(); } void Value::setV(const Vertex& v) { type_ = Type::VERTEX; - new (std::addressof(value_.vVal)) std::unique_ptr(new Vertex(v)); + new (std::addressof(value_.vVal)) Vertex*(new Vertex(v)); } void Value::setV(Vertex&& v) { type_ = Type::VERTEX; - new (std::addressof(value_.vVal)) std::unique_ptr(new Vertex(std::move(v))); + new (std::addressof(value_.vVal)) Vertex*(new Vertex(std::move(v))); } -void Value::setE(const std::unique_ptr& v) { +void Value::setE(Edge* v) { type_ = Type::EDGE; - new (std::addressof(value_.eVal)) std::unique_ptr(new Edge(*v)); -} - -void Value::setE(std::unique_ptr&& v) { - type_ = Type::EDGE; - new (std::addressof(value_.eVal)) std::unique_ptr(std::move(v)); + value_.eVal = v; + value_.eVal->ref(); } void Value::setE(const Edge& v) { type_ = Type::EDGE; - new (std::addressof(value_.eVal)) std::unique_ptr(new Edge(v)); + new (std::addressof(value_.eVal)) Edge*(new Edge(v)); } void Value::setE(Edge&& v) { type_ = Type::EDGE; - new (std::addressof(value_.eVal)) std::unique_ptr(new Edge(std::move(v))); + new (std::addressof(value_.eVal)) Edge*(new Edge(std::move(v))); } void Value::setP(const std::unique_ptr& v) { @@ -1902,9 +1912,15 @@ Value Value::equal(const Value& v) const { return getDateTime() == v.getDateTime(); } case Value::Type::VERTEX: { + if (value_.vVal == v.value_.vVal) { + return true; + } return getVertex() == v.getVertex(); } case Value::Type::EDGE: { + if (value_.eVal == v.value_.eVal) { + return true; + } return getEdge() == v.getEdge(); } case Value::Type::PATH: { @@ -2737,9 +2753,15 @@ bool Value::equals(const Value& rhs) const { return getDateTime() == rhs.getDateTime(); } case Value::Type::VERTEX: { + if (value_.vVal == rhs.value_.vVal) { + return true; + } return getVertex() == rhs.getVertex(); } case Value::Type::EDGE: { + if (value_.eVal == rhs.value_.eVal) { + return true; + } return getEdge() == rhs.getEdge(); } case Value::Type::PATH: { diff --git a/src/common/datatypes/Value.h b/src/common/datatypes/Value.h index b55397a1566..936a4cf4a3b 100644 --- a/src/common/datatypes/Value.h +++ b/src/common/datatypes/Value.h @@ -256,10 +256,10 @@ struct Value { void setDateTime(DateTime&& v); void setVertex(const Vertex& v); void setVertex(Vertex&& v); - void setVertex(std::unique_ptr&& v); + void setVertex(Vertex* v); void setEdge(const Edge& v); void setEdge(Edge&& v); - void setEdge(std::unique_ptr&& v); + void setEdge(Edge* v); void setPath(const Path& v); void setPath(Path&& v); void setPath(std::unique_ptr&& v); @@ -391,8 +391,8 @@ struct Value { Date dVal; Time tVal; DateTime dtVal; - std::unique_ptr vVal; - std::unique_ptr eVal; + Vertex* vVal; + Edge* eVal; std::unique_ptr pVal; std::unique_ptr lVal; std::unique_ptr mVal; @@ -437,13 +437,11 @@ struct Value { void setDT(const DateTime& v); void setDT(DateTime&& v); // Vertex value - void setV(const std::unique_ptr& v); - void setV(std::unique_ptr&& v); + void setV(Vertex* v); void setV(const Vertex& v); void setV(Vertex&& v); // Edge value - void setE(const std::unique_ptr& v); - void setE(std::unique_ptr&& v); + void setE(Edge* v); void setE(const Edge& v); void setE(Edge&& v); // Path value diff --git a/src/common/datatypes/ValueOps-inl.h b/src/common/datatypes/ValueOps-inl.h index 53db26fa22c..d9ab04320f4 100644 --- a/src/common/datatypes/ValueOps-inl.h +++ b/src/common/datatypes/ValueOps-inl.h @@ -368,10 +368,10 @@ void Cpp2Ops::read(Protocol* proto, nebula::Value* obj) { } case 9: { if (readState.fieldType == apache::thrift::protocol::T_STRUCT) { - obj->setVertex(nebula::Vertex()); - auto ptr = std::make_unique(); - Cpp2Ops::read(proto, ptr.get()); - obj->setVertex(std::move(ptr)); + auto* ptr = new nebula::Vertex(); + Cpp2Ops::read(proto, ptr); + obj->setVertex(ptr); + ptr->unref(); } else { proto->skip(readState.fieldType); } @@ -379,10 +379,10 @@ void Cpp2Ops::read(Protocol* proto, nebula::Value* obj) { } case 10: { if (readState.fieldType == apache::thrift::protocol::T_STRUCT) { - obj->setEdge(nebula::Edge()); - auto ptr = std::make_unique(); - Cpp2Ops::read(proto, ptr.get()); - obj->setEdge(std::move(ptr)); + auto* ptr = new nebula::Edge(); + Cpp2Ops::read(proto, ptr); + obj->setEdge(ptr); + ptr->unref(); } else { proto->skip(readState.fieldType); } diff --git a/src/common/datatypes/Vertex.h b/src/common/datatypes/Vertex.h index 1239121b7bb..41f4cfab554 100644 --- a/src/common/datatypes/Vertex.h +++ b/src/common/datatypes/Vertex.h @@ -61,12 +61,20 @@ struct Tag { struct Vertex { Value vid; std::vector tags; + std::atomic refcnt{1}; Vertex() = default; Vertex(const Vertex& v) : vid(v.vid), tags(v.tags) {} Vertex(Vertex&& v) noexcept : vid(std::move(v.vid)), tags(std::move(v.tags)) {} Vertex(Value id, std::vector t) : vid(std::move(id)), tags(std::move(t)) {} + size_t ref() { + return ++refcnt; + } + size_t unref() { + return --refcnt; + } + void clear() { vid.clear(); tags.clear(); @@ -89,6 +97,10 @@ struct Vertex { return vid == rhs.vid && tags == rhs.tags; } + bool operator!=(const Vertex& rhs) const { + return vid != rhs.vid || tags != rhs.tags; + } + bool operator<(const Vertex& rhs) const; bool contains(const Value& key) const; diff --git a/src/graph/context/Iterator.cpp b/src/graph/context/Iterator.cpp index 6d4afad6838..4079f190144 100644 --- a/src/graph/context/Iterator.cpp +++ b/src/graph/context/Iterator.cpp @@ -439,12 +439,15 @@ const Value& GetNeighborsIter::getEdgeProp(const std::string& edge, const std::s return currentEdge_->values[propIndex->second]; } -Value GetNeighborsIter::getVertex(const std::string& name) const { +Value GetNeighborsIter::getVertex(const std::string& name) { UNUSED(name); if (!valid()) { return Value::kNullValue; } auto vidVal = getColumn(0); + if (!prevVertex_.empty() && prevVertex_.getVertex().vid == vidVal) { + return prevVertex_; + } Vertex vertex; vertex.vid = vidVal; @@ -470,7 +473,8 @@ Value GetNeighborsIter::getVertex(const std::string& name) const { } vertex.tags.emplace_back(std::move(tag)); } - return Value(std::move(vertex)); + prevVertex_ = Value(std::move(vertex)); + return prevVertex_; } std::vector GetNeighborsIter::vids() { @@ -735,7 +739,7 @@ StatusOr SequentialIter::getColumnIndex(const std::string& col) con return index->second; } -Value SequentialIter::getVertex(const std::string& name) const { +Value SequentialIter::getVertex(const std::string& name) { return getColumn(name); } @@ -852,7 +856,7 @@ const Value& PropIter::getProp(const std::string& name, const std::string& prop) } } -Value PropIter::getVertex(const std::string& name) const { +Value PropIter::getVertex(const std::string& name) { UNUSED(name); if (!valid()) { return Value::kNullValue; diff --git a/src/graph/context/Iterator.h b/src/graph/context/Iterator.h index 9727680336c..1b4119f0a32 100644 --- a/src/graph/context/Iterator.h +++ b/src/graph/context/Iterator.h @@ -145,7 +145,7 @@ class Iterator { return Value::kEmpty; } - virtual Value getVertex(const std::string& name = "") const { + virtual Value getVertex(const std::string& name = "") { UNUSED(name); return Value(); } @@ -319,7 +319,7 @@ class GetNeighborsIter final : public Iterator { const Value& getEdgeProp(const std::string& edge, const std::string& prop) const override; - Value getVertex(const std::string& name = "") const override; + Value getVertex(const std::string& name = "") override; Value getEdge() const override; @@ -420,6 +420,7 @@ class GetNeighborsIter final : public Iterator { boost::dynamic_bitset<> bitset_; int64_t bitIdx_{-1}; + Value prevVertex_; }; class SequentialIter : public Iterator { @@ -497,7 +498,7 @@ class SequentialIter : public Iterator { StatusOr getColumnIndex(const std::string& col) const override; - Value getVertex(const std::string& name = "") const override; + Value getVertex(const std::string& name = "") override; Value getEdge() const override; @@ -548,7 +549,7 @@ class PropIter final : public SequentialIter { StatusOr getColumnIndex(const std::string& col) const override; - Value getVertex(const std::string& name = "") const override; + Value getVertex(const std::string& name = "") override; Value getEdge() const override;