Refactor findpath #4095

nevermore3 · 2022-03-29T11:23:15Z

What type of PR is this?

bug
feature
enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

1、reduce AllPath & multiPairShortestPath 's memory usage
2、Integrate the function of conjunct path into the path operator to reduce data copying

for specific details, please refer to the comment in the file

The blue line is optimized
The green line is not optimized

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

Unit test(positive and negative cases)
Function test
Performance test
N/A

Affects:

Documentation affected (Please add the label if documentation needs to be modified.)
Incompatibility (If it breaks the compatibility, please describe it and add the label.）
If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

ex. Fixed the bug .....

codecov-commenter · 2022-04-11T06:54:49Z

Codecov Report

Merging #4095 (693378b) into master (07cbbac) will decrease coverage by 0.15%.
The diff coverage is 96.61%.

@@            Coverage Diff             @@
##           master    #4095      +/-   ##
==========================================
- Coverage   85.08%   84.92%   -0.16%     
==========================================
  Files        1324     1319       -5     
  Lines      131911   130867    -1044     
==========================================
- Hits       112236   111143    -1093     
- Misses      19675    19724      +49

Impacted Files	Coverage Δ
src/graph/executor/admin/SpaceExecutor.cpp	`76.99% <ø> (-0.22%)`	⬇️
src/graph/executor/algo/BFSShortestPathExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/algo/ProduceAllPathsExecutor.h	`100.00% <ø> (ø)`
src/graph/executor/query/DataCollectExecutor.h	`100.00% <ø> (ø)`
src/graph/planner/ngql/PathPlanner.h	`100.00% <ø> (ø)`
src/graph/planner/plan/Algo.cpp	`43.75% <0.00%> (+2.57%)`	⬆️
src/graph/planner/plan/PlanNode.h	`86.91% <ø> (ø)`
src/graph/service/GraphFlags.cpp	`100.00% <ø> (ø)`
src/graph/validator/test/FindPathValidatorTest.cpp	`100.00% <ø> (ø)`
src/meta/test/BalancerTest.cpp	`94.70% <ø> (-1.41%)`	⬇️
... and 85 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39fbb94...693378b. Read the comment docs.

yixinglu · 2022-04-12T06:17:57Z

src/graph/executor/algo/BFSShortestPathExecutor.cpp

+  batchVids.reserve(batchSize);
+  std::vector<folly::Future<DataSet>> futures;
+  for (size_t i = 0; i < totalSize; ++i) {
+    batchVids.push_back(meetVids[i]);


If you iterate the meetVids vector from end to start, you will need not to copy the contents to a new vector.

yixinglu · 2022-04-12T06:19:11Z

src/graph/executor/algo/BFSShortestPathExecutor.cpp

+}
+
+std::unordered_multimap<Value, Path> BFSShortestPathExecutor::createPath(
+    std::vector<Value> meetVids, bool reverse, bool oddStep) {


need not to copy the vector of values.

yixinglu · 2022-04-12T06:19:34Z

src/graph/executor/algo/BFSShortestPathExecutor.cpp

+  });
+}
+
+DataSet BFSShortestPathExecutor::doConjunct(const std::vector<Value> meetVids, bool oddStep) {


const std::vector<Value> &

yixinglu · 2022-04-12T06:32:28Z

src/graph/executor/algo/MultiShortestPathExecutor.cpp

+
+  std::vector<folly::Future<Status>> futures;
+  auto leftFuture = folly::via(runner(), [this]() { return buildPath(false); });
+  auto rightFuture = folly::via(runner(), [this]() { return buildPath(true); });


I want to remind you that the buildPath will be executed in different threads carefully, don't share the same variables inside the function, especially the fields of the class.

Agree, the previous version used local variables but may cause data copying, so use the member variables

yixinglu · 2022-04-12T06:52:18Z

src/graph/executor/algo/MultiShortestPathExecutor.cpp

+
+DataSet MultiShortestPathExecutor::doConjunct(Interims::iterator startIter,
+                                              Interims::iterator endIter,
+                                              bool oddStep) {


it's better to let it be a const function for thread-safe usage.

this function modifies member variables

* optimize bfs * optimizer allpath * optimizer multi-shortestpath * optimizer multi shortest path * fix validate unit test * add some comment * fix error * fix bfs error * add comment * delete conjunct * add findpath unit test * delete useless file * delete log * remove check * multi thread executor * single shortest multi thread * add some testcases * add gflags * fix bfs error * address comment

* Add SetUsageMessage For daemons (#4134) * fix the wrong regex pattern of scientific notation (#4136) * fix updatePart (#4139) * fix updatePart * fix format check Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * show DATA_BALANCE in job list (#4138) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * fix disk fault recovery (#4131) * fix disk fault recovery * add ut Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * Refactor findpath (#4095) * optimize bfs * optimizer allpath * optimizer multi-shortestpath * optimizer multi shortest path * fix validate unit test * add some comment * fix error * fix bfs error * add comment * delete conjunct * add findpath unit test * delete useless file * delete log * remove check * multi thread executor * single shortest multi thread * add some testcases * add gflags * fix bfs error * address comment * Just report errorof property pruner in debug mode (#4142) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * remove Atomic Edge (#4127) * try to remove Atomic Edge * remove some space * remove zone Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com> * fix show tag/edge index status (#4148) * fix multiple match (#4143) Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> * Refactor the process of symbols in optimizer. (#4146) * Refactor the process of symbols in optimizer. * Fix variable shadowing. * Collect boundary from MatchResult directly. * Check variable in TransformResult don't referenced by outside plan nodes. Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com> Co-authored-by: Alex Xing <90179377+SuperYoko@users.noreply.github.com> Co-authored-by: jie.wang <38901892+jievince@users.noreply.github.com> Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com> Co-authored-by: liwenhui-soul <38217397+liwenhui-soul@users.noreply.github.com> Co-authored-by: panda-sheep <59197347+panda-sheep@users.noreply.github.com> Co-authored-by: jimingquan <mingquan.ji@vesoft.com> Co-authored-by: lionel.liu@vesoft.com <52276794+liuyu85cn@users.noreply.github.com> Co-authored-by: kyle.cao <kyle.cao@vesoft.com> Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com>

nevermore3 added ready-for-testing PR: ready for the CI test ready for review labels Mar 29, 2022

nevermore3 added this to the v3.1.0 milestone Mar 29, 2022

nevermore3 mentioned this pull request Mar 29, 2022

optimize find path #3962

Closed

11 tasks

Sophie-Xie added the cherry-pick-v3.1 PR: need cherry-pick to this version label Apr 1, 2022

nevermore3 force-pushed the refactor-findpath branch 6 times, most recently from 9ef59eb to a3d5543 Compare April 11, 2022 05:59

nevermore3 force-pushed the refactor-findpath branch from a3d5543 to 693378b Compare April 12, 2022 06:45

yixinglu reviewed Apr 12, 2022

View reviewed changes

Sophie-Xie mentioned this pull request Apr 12, 2022

Subgraph, Path: optimize operator #4070

Closed

yixinglu previously approved these changes Apr 12, 2022

View reviewed changes

CPWstatic previously approved these changes Apr 12, 2022

View reviewed changes

nevermore3 added 13 commits April 12, 2022 20:27

optimize bfs

bb8f4b8

optimizer allpath

9539838

optimizer multi-shortestpath

27e152f

optimizer multi shortest path

94c4873

fix validate unit test

020cef1

add some comment

e7f5b3f

fix error

203c27c

fix bfs error

976135e

add comment

b9536d2

delete conjunct

7b74434

add findpath unit test

c64eb8c

delete useless file

d2f68ca

delete log

92ca36c

nevermore3 added 7 commits April 12, 2022 20:27

remove check

adf3b55

multi thread executor

1aae45b

single shortest multi thread

161b779

add some testcases

3a510db

add gflags

0a2be20

fix bfs error

b4123bf

address comment

c03e5c8

nevermore3 dismissed stale reviews from CPWstatic and yixinglu via c03e5c8 April 12, 2022 12:27

nevermore3 force-pushed the refactor-findpath branch from 000e397 to c03e5c8 Compare April 12, 2022 12:27

yixinglu approved these changes Apr 13, 2022

View reviewed changes

CPWstatic approved these changes Apr 13, 2022

View reviewed changes

Merge branch 'master' into refactor-findpath

afd5f20

Sophie-Xie merged commit 072af05 into vesoft-inc:master Apr 13, 2022

nevermore3 deleted the refactor-findpath branch April 13, 2022 05:45

jamieliu1023 mentioned this pull request Apr 16, 2022

Weekly Report 2022-04-15 vesoft-inc/nebula-community#104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor findpath #4095

Refactor findpath #4095

nevermore3 commented Mar 29, 2022 •

edited

Loading

codecov-commenter commented Apr 11, 2022 •

edited

Loading

yixinglu Apr 12, 2022

yixinglu Apr 12, 2022

nevermore3 Apr 12, 2022

yixinglu Apr 12, 2022

nevermore3 Apr 12, 2022

yixinglu Apr 12, 2022

nevermore3 Apr 12, 2022

yixinglu Apr 12, 2022

nevermore3 Apr 12, 2022

Refactor findpath #4095

Refactor findpath #4095

Conversation

nevermore3 commented Mar 29, 2022 • edited Loading

What type of PR is this?

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Release notes:

codecov-commenter commented Apr 11, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevermore3 commented Mar 29, 2022 •

edited

Loading

codecov-commenter commented Apr 11, 2022 •

edited

Loading