Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Tuning GetNeighbors Perf #111

Merged
merged 10 commits into from
Sep 22, 2020
Merged

Tuning GetNeighbors Perf #111

merged 10 commits into from
Sep 22, 2020

Conversation

critical27
Copy link
Contributor

Better to review by each commit, some modification is scattered among lots of files.
Some major improvement:

  1. Only retrieve srcId/type/rank/dstId in collectEdgeProps if needed
  2. Only add FilterNode and AggregateNode if necessary.
  3. Add FLAGS_enable_multi_versions, there is a string copy if we want to filter the edge of different versions.
  4. Allocate RowReader from stack instead of heap, similar to fix VertexHolder::getDefaultProp performance issue. nebula#2249. The difference is that we use RowReaderWrapper to wrap reader of v1 and v2.

The Benchmark will retrieve 1 tag and 1000 edges, almost the same as vesoft-inc/nebula#2305

After these tuning, the benchmark of 2.0 has ~20% improvement than 1.0.

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Aug 17, 2020
@CLAassistant
Copy link

CLAassistant commented Sep 2, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

panda-sheep
panda-sheep previously approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@panda-sheep panda-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pr is same as pr #103, has reviewed in pr #103.

Good job, excellent!

darionyaphet
darionyaphet reviewed Sep 18, 2020
View reviewed changes
src/codec/test/RowReaderV1Test.cpp Outdated
@@ -71,7 +71,7 @@ TEST(RowReaderV1, headerInfo) {

// Empty row, return illegal schema version
SchemaWriter schema5;
auto reader2 = RowReader::getRowReader(&schema5,
auto reader2 = RowReaderWrapper::getRowReader(&schema5,
folly::StringPiece(""));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignment ?

darionyaphet
darionyaphet reviewed Sep 18, 2020
View reviewed changes
src/storage/admin/RebuildEdgeIndexProcessor.cpp Outdated
namespace nebula {
namespace storage {

void RebuildEdgeIndexProcessor::process(const cpp2::RebuildIndexRequest& req) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file have replaced by RebuildEdgeIndexTask?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, add by mistake, fixed

bright-starry-sky
bright-starry-sky approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@bright-starry-sky bright-starry-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

darionyaphet
darionyaphet approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@darionyaphet darionyaphet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Work~

@dutor dutor merged commit d322517 into vesoft-inc:master Sep 22, 2020
@critical27 critical27 deleted the perf branch September 22, 2020 09:56
@bright-starry-sky bright-starry-sky mentioned this pull request Sep 23, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@darionyaphet darionyaphet darionyaphet approved these changes

@panda-sheep panda-sheep panda-sheep approved these changes

@bright-starry-sky bright-starry-sky bright-starry-sky approved these changes

@dangleptr dangleptr Awaiting requested review from dangleptr

@liuyu85cn liuyu85cn Awaiting requested review from liuyu85cn

Assignees
No one assigned
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@critical27 @CLAassistant @darionyaphet @bright-starry-sky @panda-sheep @dutor