Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize go N steps over edge #1471

Merged
merged 3 commits into from
Jan 21, 2020
Merged

Optimize go N steps over edge #1471

merged 3 commits into from
Jan 21, 2020

Conversation

dangleptr
Copy link
Contributor

@dangleptr dangleptr commented Dec 18, 2019

Optimize go executor.
The pr improve the perf for "go 2 steps over edge" about 20x with about 1M vertex ids returned.
Of course, it will improve the perf for other queries, but i have't test it yet.

Main changes:

  1. Modify the structure storaged returned, separate vertexId from props to avoid "getPropByName" called when getting dstId. (This is a normal request for go)

  2. Return directly if current executor is the rightest. It avoid one extra encode/decode procedure which cost lots of time if we have many rows returned.

  3. Pre calculate the yield columns types to avoid calculate them for every edge.

@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing passed.

@nebula-community-bot
Copy link
Member

Unit testing failed.

@dangleptr dangleptr removed the ready-for-testing PR: ready for the CI test label Dec 20, 2019
@dangleptr dangleptr added the ready-for-testing PR: ready for the CI test label Dec 20, 2019
@nebula-community-bot
Copy link
Member

Unit testing passed.

src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
@nebula-community-bot
Copy link
Member

Unit testing passed.

1 similar comment
@nebula-community-bot
Copy link
Member

Unit testing passed.

src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
int64_t totalRows = 0;
for (auto& resp : rpcResp.responses()) {
if (resp.get_total_edges() != nullptr) {
totalRows += *resp.get_total_edges();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe *(resp.get_total_edges()) more readable

src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
if (!edgeSchema.empty()) {
auto it = edgeSchema.find(edgeType);
DCHECK(it != edgeSchema.end());
reader = RowReader::getRowReader(edge.props, it->second);
Copy link
Contributor

Choose a reason for hiding this comment

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

because the reader depend edgeSchema, if the edgeSchema is empty, the reader is nullptr, then CHECK(reader != nullptr); will crash , so if can we return directly when edgeSchema is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If getAliasProp invoked, it means the reader should not be NULL. Otherwise it is a bug.

src/graph/FindPathExecutor.cpp Outdated Show resolved Hide resolved
src/graph/FindPathExecutor.cpp Show resolved Hide resolved
src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
src/graph/GoExecutor.cpp Show resolved Hide resolved
src/graph/GoExecutor.cpp Outdated Show resolved Hide resolved
src/storage/query/QueryBaseProcessor.inl Outdated Show resolved Hide resolved
src/interface/storage.thrift Outdated Show resolved Hide resolved
@jude-zhu
Copy link
Contributor

jude-zhu commented Jan 8, 2020

close #1604

src/graph/GoExecutor.cpp Show resolved Hide resolved
src/graph/FindPathExecutor.cpp Show resolved Hide resolved
src/graph/GoExecutor.cpp Show resolved Hide resolved
CPWstatic
CPWstatic previously approved these changes Jan 13, 2020
@@ -1408,5 +1468,73 @@ SupportedType GoExecutor::getPropTypeFromInterim(const std::string &prop) const
return index_->getColumnType(prop);
}

nebula::cpp2::SupportedType GoExecutor::calculateExprType(Expression* exp) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the schema cache of graphd and the storaged are different, maybe has problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we use the schema in graphd.
And the problem also existed in current master branch, because different storaged maybe have different versions schema when updating it.

<< time::WallClock::fastNowInMicroSec() - start << "us";
}
if (!ret.ok()) {
LOG(ERROR) << "Get rows failed: " << ret.status();
Copy link
Contributor

Choose a reason for hiding this comment

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

add doError()

@dutor dutor merged commit 19c5f85 into vesoft-inc:master Jan 21, 2020
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
Optimize go executor.
The pr improve the perf for "go 2 steps over edge" about 20x with about 1M vertex ids returned.
Of course,  it will improve the perf for other queries, but i have't test it yet. 

Main changes:
1.  Modify the structure storaged returned,  separate vertexId from props to avoid "getPropByName" called when getting dstId.  (This is a normal request for go)

2.  Return directly if current executor is the rightest.  It avoid one extra encode/decode procedure which cost lots of time if we have many rows returned.

3. Pre calculate the yield columns types to avoid calculate them for every edge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants