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-all-path-go #5528

Merged
merged 5 commits into from
Apr 27, 2023
Merged

optimize-all-path-go #5528

merged 5 commits into from
Apr 27, 2023

Conversation

nevermore3
Copy link
Contributor

@nevermore3 nevermore3 commented Apr 26, 2023

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

close https://github.com/vesoft-inc/nebula-ent/issues/2725

Description:

allPath:
1、In the allpath operator, the intermediate path uses the alternate of vertex and edge (for example [src, [e1, v1,e2,v2,e3], dst]) to represent the path
2、The copy constructor and assignment constructor of the VERTEX and the EDGE  are shallow copies
3、Construct paths base on  adjacency lists using multiple threads

Therefore, when constructing a new path, a large number of copies of vertex and edge are involved. reference count in  VERTEX and EDGE becomes a performance bottleneck

Some improvements blow:
1、Use the new structure NPath to save intermediate paths
    struct NPath {
      NPath* p;
      const Value& vertex;
      const Value& edge;
    }
   This can avoid copying vertex and edge each time a new path is generated
  2、Use threadLocalPtr to save the NPath structure generated inside each thread, reducing the time taken to release memory
BEFOR
(root@nebula) [sf100]>  FIND ALL PATH FROM 1129 TO 30786325832215 OVER KNOWS YIELD path AS p | YIELD count(*);
+----------+
| count(*) |
+----------+
| 112808   |
+----------+
Got 1 rows (time spent 18277713/18271423 us)
NOW
(root@nebula) [sf100]>  FIND ALL PATH FROM 1129 TO 30786325832215 OVER KNOWS YIELD path AS p|yield count(*)
+----------+
| count(*) |
+----------+
| 112808   |
+----------+
Got 1 rows (time spent 2572926/2574018 us)

Go planner

For the scenario of GO n STEPS FROM xxx OVER edge YIELD DISTINCT edge._dst,
use the new execution plan alone, and only obtain the destination point through the getDstbysrc operator

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 .....

src/graph/executor/algo/AllPathsExecutor.cpp Outdated Show resolved Hide resolved
src/graph/executor/algo/AllPathsExecutor.cpp Outdated Show resolved Hide resolved
src/graph/executor/algo/AllPathsExecutor.h Outdated Show resolved Hide resolved
@yixinglu yixinglu added the ready-for-testing PR: ready for the CI test label Apr 27, 2023
// add src;
row.values.emplace_back(list.back());
list.pop_back();
std::reverse(list.begin(), list.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

why need to reverse the list?

Copy link
Contributor Author

@nevermore3 nevermore3 Apr 27, 2023

Choose a reason for hiding this comment

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

when recursively parsing vertex and edge through NPath, the order is reversed compared to the topology of the path

@nevermore3 nevermore3 force-pushed the fix_all_path branch 6 times, most recently from 840a417 to b15aeb0 Compare April 27, 2023 09:19
@nevermore3 nevermore3 changed the title optimize-all-path optimize-all-path-go Apr 27, 2023
@xtcyclist xtcyclist merged commit cd53546 into vesoft-inc:master Apr 27, 2023
Sophie-Xie pushed a commit that referenced this pull request Apr 27, 2023
@nevermore3 nevermore3 deleted the fix_all_path branch April 27, 2023 14:12
Sophie-Xie added a commit that referenced this pull request Apr 27, 2023
* fixed yapf version  (#5513)

* Update requirements.txt

fixed yapf version to the last version supporting tomli 1.x

* Update requirements.txt

fix typo

* update unsupported action syntax (#5527)

update unsupported syntax

* optimize-all-path-go (#5528)

---------

Co-authored-by: George <[email protected]>
Co-authored-by: jimingquan <[email protected]>
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants