fix VertexHolder::getDefaultProp performance issue. #2249

xuguruogu · 2020-07-25T07:36:50Z

VertexHolder::getDefaultProp has huge performace problem, if get some prop not exist.

before:

after:

xuguruogu · 2020-07-25T10:18:45Z

Anyway VertexHolder::get also has performance problem, due to call malloc for every vertex prop fetch.

Lines 1352 to 1356 in a2d51c8

    
           } 
        
           auto reader = RowReader::getRowReader(std::get<1>(iter2->second), std::get<0>(iter2->second)); 
        
           auto res = RowReader::getPropByName(reader.get(), prop);

xuguruogu · 2020-07-25T10:29:28Z

rewrite VertexHolder maybe required.

dangleptr · 2020-07-27T02:59:10Z

src/graph/GoExecutor.cpp

-        if (it2 != it->second.cend()) {
-            return RowReader::getDefaultProp(std::get<0>(it2->second).get(), prop);
-        }
+OptVariantType GoExecutor::VertexHolder::getDefaultProp(


Actually, we don't need "getDefaultProp" and "getDefaultPropType" inside VertexHolder,

You could use RowReader::getDefaultProp directly.

FYI. You could pass schemaManager into VertexHolder directly.

dangleptr · 2020-07-27T03:01:10Z

Thanks for your contribution. It is really a stupid mistake.

dangleptr · 2020-07-27T03:17:52Z

Anyway VertexHolder::get also has performance problem, due to call malloc for every vertex prop fetch.

nebula/src/graph/GoExecutor.cpp

Lines 1352 to 1356 in a2d51c8

}

auto reader = RowReader::getRowReader(std::get<1>(iter2->second), std::get<0>(iter2->second));

auto res = RowReader::getPropByName(reader.get(), prop);

I found the problem too. Not only in graphd, but also inside storaged. We have fixed the issue inside 2.0
If you want to fix it in 1.0, what you need to do is to implement reset method inside RowReader, for each edge/tag,
just reset the value and schema. Use only one RowReader for the whole request.

dangleptr · 2020-07-28T02:02:41Z

Totally the PR looks good to me. Do you have any plan to fix the RowReader problem in this pr? @xuguruogu

xuguruogu · 2020-07-28T04:59:39Z

i prefer to fix rowreader later

xuguruogu · 2020-07-29T04:30:22Z

Totally the PR looks good to me. Do you have any plan to fix the RowReader problem in this pr? @xuguruogu

wait for a while. Writing codes.

xuguruogu · 2020-07-29T07:38:30Z

Anyway VertexHolder::get also has performance problem, due to call malloc for every vertex prop fetch.

nebula/src/graph/GoExecutor.cpp

Lines 1352 to 1356 in a2d51c8

}

auto reader = RowReader::getRowReader(std::get<1>(iter2->second), std::get<0>(iter2->second));

auto res = RowReader::getPropByName(reader.get(), prop);

I found the problem too. Not only in graphd, but also inside storaged. We have fixed the issue inside 2.0
If you want to fix it in 1.0, what you need to do is to implement reset method inside RowReader, for each edge/tag,
just reset the value and schema. Use only one RowReader for the whole request.

expose RowReader with the interface of std::unique_ptr may be a better choice. Without changing large amount of code, all functions using RowReader can benefit from malloc free codes.

dangleptr · 2020-07-29T07:57:35Z

src/storage/mutate/AddEdgesProcessor.cpp

@@ -87,7 +87,7 @@ std::string AddEdgesProcessor::addEdges(int64_t version, PartitionID partId,
    });
    for (auto& e : newEdges) {
        std::string val;
-        std::unique_ptr<RowReader> nReader;
+        RowReader nReader = RowReader::getEmptyRowReader();


You'd better use pointer of RowReader this place. Because there are lots of check null for rowReader in the code.

nebula/src/dataman/RowReader.h

Lines 299 to 313 in e5cefb2

bool operator==(nullptr_t) const noexcept {

return !data_.data();

}

bool operator==(const RowReader& x) const noexcept {

return data_ == x.data_;

}

bool operator!=(nullptr_t) const noexcept {

return (bool)data_.data();

}

bool operator!=(const RowReader& x) const noexcept {

return data_ != x.data_;

}

Solve it with CPP magic. This can really help to improve performance, the total batch computing costs reduces from 13min to 5min, for about 2.6X improvement.

Access data set from the stack is much faster than from heap, for CPU hardware cache optimization.

dangleptr · 2020-07-29T07:58:41Z

src/storage/query/QueryBoundProcessor.cpp

@@ -88,7 +87,7 @@ kvstore::ResultCode QueryBoundProcessor::processEdgeSampling(const PartitionID p
    using Sample = std::tuple<
        EdgeType, /* type */
        std::string, /* key */
-        std::unique_ptr<RowReader>, /* val */
+        RowReader, /* val */


RowReader size is large. Pointer is a better choice.

dangleptr

Awesome work!! The pr looks good to me.

Besides the RowReader, there is another point we have optimized in 2.0. You could take it as your reference.

That is MetaClient::getEdgeSchemaFromCache
For each row, inside the RowReader, it will call this method to get the related schema.

It is costly, because there are two hashMap inside it.
So in 2.0, we copy the related edge schema before scanning each edgeType. And use a vector to store the multi versions schema for the edgeType.

So two hash lookup will be replaced by one random array access.

dangleptr · 2020-07-29T09:15:22Z

src/dataman/RowReader.h

+        return *get();
+    }
+
+    void reset() noexcept {


It seems we dont need reset any more. But never mind, leave it here.

dangleptr · 2020-07-29T09:35:21Z

src/storage/query/QueryBoundProcessor.cpp

-                                        currEdgeSchema, props));
+                        std::make_tuple(
+                            edgeType, k.str(),
+                            std::make_unique<RowReader>(std::move(reader)),


HaHa. Very tricky one~

dangleptr · 2020-07-29T09:36:35Z

Please check the code style.

critical27

Impressive!

critical27

Good job!

xuguruogu · 2020-08-01T12:25:01Z

Awesome work!! The pr looks good to me.

Besides the RowReader, there is another point we have optimized in 2.0. You could take it as your reference.

That is MetaClient::getEdgeSchemaFromCache
For each row, inside the RowReader, it will call this method to get the related schema.

It is costly, because there are two hashMap inside it.
So in 2.0, we copy the related edge schema before scanning each edgeType. And use a vector to store the multi versions schema for the edgeType.

So two hash lookup will be replaced by one random array access.

emm... It may not give the expected performance improvement. I am familiar with commonly used optimization method. Maybe we can talk about it later in a specified scenario.

dangleptr · 2020-08-03T07:00:57Z

emm... It may not give the expected performance improvement. I am familiar with commonly used optimization method. Maybe we can talk about it later in a specified scenario.

For RowReader, it has about 1.3x improvement.

* fix VertexHolder::getDefaultProp performance issue. * rewrite row reader to avoid malloc Co-authored-by: trippli <trippli@tencent.com> Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com>

* add allpath test * add shortest path test case * add subgraph test case * add go test case * add go test case Co-authored-by: jimingquan <mingquan.ji@vesoft.com>

dangleptr reviewed Jul 27, 2020

View reviewed changes

dangleptr reviewed Jul 29, 2020

View reviewed changes

xuguruogu force-pushed the fix-VertexHolder-getDefaultProp branch from e5cefb2 to bf264be Compare July 29, 2020 09:04

dangleptr reviewed Jul 29, 2020

View reviewed changes

dangleptr previously approved these changes Jul 29, 2020

View reviewed changes

dangleptr requested a review from critical27 July 29, 2020 09:32

dangleptr added the ready-for-testing PR: ready for the CI test label Jul 29, 2020

dangleptr reviewed Jul 29, 2020

View reviewed changes

critical27 reviewed Jul 29, 2020

View reviewed changes

fix VertexHolder::getDefaultProp performance issue.

777c7e3

xuguruogu dismissed dangleptr’s stale review via 8ef5730 July 29, 2020 10:12

xuguruogu force-pushed the fix-VertexHolder-getDefaultProp branch from c7ee3b4 to 8ef5730 Compare July 29, 2020 10:12

rewrite row reader to avoid malloc

ce66684

xuguruogu force-pushed the fix-VertexHolder-getDefaultProp branch from 8ef5730 to ce66684 Compare July 29, 2020 11:17

critical27 approved these changes Jul 29, 2020

View reviewed changes

Merge branch 'master' into fix-VertexHolder-getDefaultProp

bef371f

dangleptr approved these changes Jul 30, 2020

View reviewed changes

dangleptr merged commit c95e85e into vesoft-inc:master Jul 30, 2020

xuguruogu deleted the fix-VertexHolder-getDefaultProp branch July 31, 2020 03:08

xuguruogu mentioned this pull request Aug 1, 2020

Let us talk about replace the LRU cache with cuckoo hash cache. #2264

Closed

critical27 mentioned this pull request Aug 5, 2020

Tuning GetNeighbors perf vesoft-inc/nebula-storage#103

Closed

critical27 mentioned this pull request Aug 17, 2020

Tuning GetNeighbors Perf vesoft-inc/nebula-storage#111

Merged

yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Jan 31, 2023

Add tck test (vesoft-inc#2249)

9e9a000

* add allpath test * add shortest path test case * add subgraph test case * add go test case * add go test case Co-authored-by: jimingquan <mingquan.ji@vesoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix VertexHolder::getDefaultProp performance issue. #2249

fix VertexHolder::getDefaultProp performance issue. #2249

xuguruogu commented Jul 25, 2020 •

edited

Loading

xuguruogu commented Jul 25, 2020

xuguruogu commented Jul 25, 2020

dangleptr Jul 27, 2020 •

edited

Loading

dangleptr commented Jul 27, 2020

dangleptr commented Jul 27, 2020 •

edited

Loading

dangleptr commented Jul 28, 2020

xuguruogu commented Jul 28, 2020

xuguruogu commented Jul 29, 2020

xuguruogu commented Jul 29, 2020

dangleptr Jul 29, 2020

xuguruogu Jul 29, 2020 •

edited

Loading

dangleptr Jul 29, 2020

xuguruogu Jul 29, 2020

dangleptr left a comment

dangleptr Jul 29, 2020

dangleptr Jul 29, 2020

dangleptr commented Jul 29, 2020

critical27 left a comment

critical27 left a comment

xuguruogu commented Aug 1, 2020

dangleptr commented Aug 3, 2020

	bool operator==(nullptr_t) const noexcept {
	return !data_.data();
	}

	bool operator==(const RowReader& x) const noexcept {
	return data_ == x.data_;
	}

	bool operator!=(nullptr_t) const noexcept {
	return (bool)data_.data();
	}

	bool operator!=(const RowReader& x) const noexcept {
	return data_ != x.data_;
	}

fix VertexHolder::getDefaultProp performance issue. #2249

fix VertexHolder::getDefaultProp performance issue. #2249

Conversation

xuguruogu commented Jul 25, 2020 • edited Loading

xuguruogu commented Jul 25, 2020

xuguruogu commented Jul 25, 2020

dangleptr Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

dangleptr commented Jul 27, 2020

dangleptr commented Jul 27, 2020 • edited Loading

dangleptr commented Jul 28, 2020

xuguruogu commented Jul 28, 2020

xuguruogu commented Jul 29, 2020

xuguruogu commented Jul 29, 2020

dangleptr Jul 29, 2020

Choose a reason for hiding this comment

xuguruogu Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

dangleptr Jul 29, 2020

Choose a reason for hiding this comment

xuguruogu Jul 29, 2020

Choose a reason for hiding this comment

dangleptr left a comment

Choose a reason for hiding this comment

dangleptr Jul 29, 2020

Choose a reason for hiding this comment

dangleptr Jul 29, 2020

Choose a reason for hiding this comment

dangleptr commented Jul 29, 2020

critical27 left a comment

Choose a reason for hiding this comment

critical27 left a comment

Choose a reason for hiding this comment

xuguruogu commented Aug 1, 2020

dangleptr commented Aug 3, 2020

xuguruogu commented Jul 25, 2020 •

edited

Loading

dangleptr Jul 27, 2020 •

edited

Loading

dangleptr commented Jul 27, 2020 •

edited

Loading

xuguruogu Jul 29, 2020 •

edited

Loading