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.

Sign up for GitHub

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

Jump to bottom

Using icord to avoid dynamic allocation and speed up write. #2237

Merged
merged 2 commits into from
Jul 30, 2020
Merged

Using icord to avoid dynamic allocation and speed up write. #2237

merged 2 commits into from
Jul 30, 2020

Conversation

Shylock-Hg
Copy link
Contributor

@Shylock-Hg Shylock-Hg commented Jul 20, 2020 •
edited
Loading

What changes were proposed in this pull request?

Why are the changes needed?

Will break the compatibility? How if so?

NO

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Checklist

  • I've run the tests to see all new and existing tests pass
  • If this Pull Request resolves an issue, I linked to the issue in the text above
  • I've informed the technical writer about the documentation change if necessary

@Shylock-Hg Shylock-Hg added the ready-for-testing PR: ready for the CI test label Jul 20, 2020
dangleptr
dangleptr approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

Awesome work!
You are the man!

@dangleptr
Copy link
Contributor

BTW. Do we need to make the buffer to be thread local?

@Shylock-Hg
Copy link
Contributor Author

BTW. Do we need to make the buffer to be thread local?

Why need it?

@dangleptr
Copy link
Contributor

BTW. Do we need to make the buffer to be thread local?

Why need it?

To avoid stack overflow under high write load. Of course, it is not big issue currently.

@Shylock-Hg
Copy link
Contributor Author

BTW. Do we need to make the buffer to be thread local?

Why need it?

To avoid stack overflow under high write load. Of course, it is not big issue currently.

I don't think this will happen, the ICord object will release when write over and it only 1kb in stack.

@dangleptr
Copy link
Contributor

BTW. Do we need to make the buffer to be thread local?

Why need it?

To avoid stack overflow under high write load. Of course, it is not big issue currently.

I don't think this will happen, the ICord object will release when write over and it only 1kb in stack.

Yes, normally it will never happen.

critical27
critical27 approved these changes Jul 30, 2020
View reviewed changes
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Good job

@codecov-commenter
Copy link

Codecov Report

Merging #2237 into master will decrease coverage by 0.08%.
The diff coverage is 89.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2237      +/-   ##
==========================================
- Coverage   86.54%   86.46%   -0.09%     
==========================================
  Files         644      646       +2     
  Lines       63643    64249     +606     
==========================================
+ Hits        55079    55550     +471     
- Misses       8564     8699     +135
Impacted Files Coverage Δ
src/common/filter/Expressions.h 89.82% <0.00%> (ø)
src/dataman/RowSetReader.h 100.00% <ø> (ø)
src/dataman/RowUpdater.h 100.00% <ø> (ø)
src/dataman/RowWriter.h 100.00% <ø> (ø)
src/graph/CloudAuthenticator.cpp 0.00% <0.00%> (ø)
src/graph/FetchVerticesExecutor.h 0.00% <ø> (ø)
src/graph/TraverseExecutor.cpp 76.71% <0.00%> (-3.89%) ⬇️
src/kvstore/raftex/RaftPart.h 98.21% <ø> (ø)
src/meta/SchemaManager.h 100.00% <ø> (ø)
src/meta/ServerBasedSchemaManager.h 100.00% <ø> (ø)
... and 79 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 7149ba2...3e76b6b. Read the comment docs.

@dangleptr dangleptr merged commit 2bf00e7 into vesoft-inc:master Jul 30, 2020
critical27 pushed a commit to critical27/nebula that referenced this pull request Aug 4, 2020
@Shylock-Hg @dangleptr @critical27
Using icord to avoid dynamic allocation and speed up write. (vesoft-i…
5734201 
…nc#2237)

Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com>
xuguruogu pushed a commit to xuguruogu/nebula that referenced this pull request Aug 5, 2020
@Shylock-Hg @dangleptr @xuguruogu
Using icord to avoid dynamic allocation and speed up write. (vesoft-i…
a1186f9 
…nc#2237)

Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com>
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
@Shylock-Hg @dangleptr
Using icord to avoid dynamic allocation and speed up write. (vesoft-i…
76cd36a 
…nc#2237)

Co-authored-by: dangleptr <37216992+dangleptr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@critical27 critical27 critical27 approved these changes

@dangleptr dangleptr dangleptr approved these changes

@CPWstatic CPWstatic Awaiting requested review from CPWstatic

@dutor dutor Awaiting requested review from dutor

@bright-starry-sky bright-starry-sky Awaiting requested review from bright-starry-sky

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.
4 participants
@Shylock-Hg @dangleptr @codecov-commenter @critical27