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

Feature/arena allocator #4239

Merged
merged 25 commits into from
Jun 7, 2022

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented May 9, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

Sub job of #4122

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

@Shylock-Hg Shylock-Hg added type/enhancement Type: make the code neat or more efficient ready-for-testing PR: ready for the CI test labels May 9, 2022
@Shylock-Hg Shylock-Hg added this to the v3.2.0 milestone May 9, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #4239 (88ec217) into master (326b856) will decrease coverage by 0.02%.
The diff coverage is 91.15%.

@@            Coverage Diff             @@
##           master    #4239      +/-   ##
==========================================
- Coverage   84.99%   84.96%   -0.03%     
==========================================
  Files        1326     1328       +2     
  Lines      131713   131866     +153     
==========================================
+ Hits       111946   112040      +94     
- Misses      19767    19826      +59     
Impacted Files Coverage Δ
src/graph/optimizer/OptGroup.h 100.00% <ø> (ø)
src/graph/planner/match/MatchSolver.cpp 49.46% <0.00%> (ø)
src/storage/exec/IndexVertexScanNode.h 100.00% <ø> (ø)
src/storage/index/LookupProcessor.h 100.00% <ø> (ø)
src/common/expression/TextSearchExpression.h 56.89% <62.50%> (-2.04%) ⬇️
src/graph/planner/plan/Admin.h 66.74% <62.96%> (ø)
src/graph/executor/Executor.cpp 81.23% <80.64%> (ø)
src/storage/index/LookupProcessor.cpp 89.03% <88.88%> (-0.29%) ⬇️
src/graph/planner/plan/Maintain.h 88.54% <90.47%> (ø)
src/common/base/Arena.cpp 95.00% <95.00%> (ø)
... and 104 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 1fd89af...88ec217. Read the comment docs.

nevermore3
nevermore3 previously approved these changes May 11, 2022
Copy link
Contributor

@CPWstatic CPWstatic left a comment

Choose a reason for hiding this comment

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

Do we really need Arena like this for Expression objects?
Emmm, i'm curious about the performance of parser, would you please do some benchmark for it? It might improves it.

@Shylock-Hg
Copy link
Contributor Author

Do we really need Arena like this for Expression objects? Emmm, i'm curious about the performance of parser, would you please do some benchmark for it? It might improves it.

There is benchmark, https://github.com/vesoft-inc/nebula/pull/4239/files#diff-3a4f05d31dbd51dad5408e11f426aea11f5ab1548fb2a65f6143bcda77dee8f3

@Shylock-Hg Shylock-Hg requested a review from CPWstatic May 17, 2022 09:55
Comment on lines +86 to +88
// DefaultAllocator 36.59us 27.33K
// ArenaAllocator 145.89% 25.08us 39.87K
// FollyArenaAllocator 138.96% 26.33us 37.98K
Copy link
Contributor

Choose a reason for hiding this comment

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

Goog job! We should also update the parser benchmarks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Benchmark of origin:

============================================================================
/home/shylock.huang/nebula-tmp2/src/parser/test/ParserBenchmark.cpprelative  time/iter  iters/s
============================================================================
SimpleQuery(1_thread)                                        1.20us  836.34K
SimpleQuery(2_thread)                             96.66%     1.24us  808.44K
SimpleQuery(4_thread)                             94.08%     1.27us  786.79K
SimpleQuery(8_thread)                             94.90%     1.26us  793.64K
SimpleQuery(16_thread)                            95.07%     1.26us  795.14K
SimpleQuery(32_thread)                            66.19%     1.81us  553.59K
SimpleQuery(48_thread)                            47.52%     2.52us  397.44K
----------------------------------------------------------------------------
ComplexQuery(1_thread)                                       8.61us  116.12K
ComplexQuery(2_thread)                            97.47%     8.84us  113.18K
ComplexQuery(4_thread)                            95.44%     9.02us  110.83K
ComplexQuery(8_thread)                            97.86%     8.80us  113.64K
ComplexQuery(16_thread)                           98.14%     8.78us  113.96K
ComplexQuery(32_thread)                           95.71%     9.00us  111.14K
ComplexQuery(48_thread)                           60.94%    14.13us   70.76K
----------------------------------------------------------------------------
MatchConflictQuery(1_thread)                                 3.65us  274.17K
MatchConflictQuery(2_thread)                      96.74%     3.77us  265.23K
MatchConflictQuery(4_thread)                      94.19%     3.87us  258.25K
MatchConflictQuery(8_thread)                      93.17%     3.91us  255.45K
MatchConflictQuery(16_thread)                     94.70%     3.85us  259.64K
MatchConflictQuery(32_thread)                     91.22%     4.00us  250.10K
MatchConflictQuery(48_thread)                     57.08%     6.39us  156.50K
============================================================================

Benchmark of current:

============================================================================
/home/shylock.huang/nebula-tmp/src/parser/test/ParserBenchmark.cpprelative  time/iter  iters/s
============================================================================
SimpleQuery(1_thread)                                        1.07us  931.55K
SimpleQuery(2_thread)                             98.63%     1.09us  918.82K
SimpleQuery(4_thread)                             97.50%     1.10us  908.26K
SimpleQuery(8_thread)                             96.84%     1.11us  902.09K
SimpleQuery(16_thread)                            93.79%     1.14us  873.73K
SimpleQuery(32_thread)                            65.51%     1.64us  610.22K
SimpleQuery(48_thread)                            41.94%     2.56us  390.73K
----------------------------------------------------------------------------
ComplexQuery(1_thread)                                       8.34us  119.88K
ComplexQuery(2_thread)                            98.58%     8.46us  118.18K
ComplexQuery(4_thread)                            96.30%     8.66us  115.45K
ComplexQuery(8_thread)                            92.82%     8.99us  111.27K
ComplexQuery(16_thread)                           93.72%     8.90us  112.36K
ComplexQuery(32_thread)                           94.08%     8.87us  112.78K
ComplexQuery(48_thread)                           59.36%    14.05us   71.16K
----------------------------------------------------------------------------
MatchConflictQuery(1_thread)                                 3.83us  261.02K
MatchConflictQuery(2_thread)                      97.54%     3.93us  254.61K
MatchConflictQuery(4_thread)                      99.23%     3.86us  259.00K
MatchConflictQuery(8_thread)                      96.97%     3.95us  253.10K
MatchConflictQuery(16_thread)                     96.94%     3.95us  253.03K
MatchConflictQuery(32_thread)                     91.72%     4.18us  239.40K
MatchConflictQuery(48_thread)                     57.47%     6.67us  150.01K
============================================================================

@Shylock-Hg Shylock-Hg requested a review from CPWstatic May 30, 2022 05:48
CPWstatic
CPWstatic previously approved these changes Jun 7, 2022
@Sophie-Xie Sophie-Xie merged commit 117e01a into vesoft-inc:master Jun 7, 2022
@Shylock-Hg Shylock-Hg deleted the feature/arena-allocator branch June 7, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test type/enhancement Type: make the code neat or more efficient
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants