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

remove read lock in meta client #3256

Merged
merged 14 commits into from
Nov 12, 2021
Merged

Conversation

jackwener
Copy link
Contributor

What type of PR is this?

  • feature

Which issue(s) this PR fixes:

close #3129

What this PR does / why we need it?

reduce the side effect of read-write locks

Checklist:

  • Performance regression: Consumes more Memory `

@jackwener jackwener added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Nov 2, 2021
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. After I review the code, some places you need to pay attention:

  1. You only handle the structure in SpaceInfoCache, I think some function is missing, for example getTagSchemaFromCache, getLatestTagVersionFromCache and so on. In fact, almost all function with folly::RWSpinLock::ReadHolder should be taken care.
  2. Just delete the commented code, don't keep them in the old place

You could do some simple performance test using k6 or something else. This PR is very sensitive to performance.

@jackwener jackwener added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Nov 9, 2021
@jackwener jackwener changed the title [DNM] remove rwlock in meta client remove read lock in meta client Nov 10, 2021
@jackwener
Copy link
Contributor Author

jackwener commented Nov 11, 2021

There are 5 remain read lock in 667 2319 2360 2383 2395 line.
leader is in line 667. leader is updated in updateLeader() outside loaddata().
leadersInfo_ is in line 2319 and 2360. leadersInfo_.leaderMap_ and leadersInfo_.pickedIndex_ is update outside loaddata().
authCheckFromCache() is in 2383 and checkShadowAccountFromCache() is in 2395, they must be const because const client in PasswordAuthenticator() call it.

@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2021

Codecov Report

Merging #3256 (7b158da) into master (8e0028f) will decrease coverage by 0.02%.
The diff coverage is 81.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3256      +/-   ##
==========================================
- Coverage   85.29%   85.27%   -0.03%     
==========================================
  Files        1305     1305              
  Lines      120383   120443      +60     
==========================================
+ Hits       102683   102710      +27     
- Misses      17700    17733      +33     
Impacted Files Coverage Δ
src/graph/service/CloudAuthenticator.cpp 0.00% <0.00%> (ø)
...torage/transaction/ChainAddEdgesProcessorLocal.cpp 58.82% <50.00%> (+0.01%) ⬆️
src/clients/meta/MetaClient.cpp 75.34% <82.12%> (+0.37%) ⬆️
src/clients/meta/MetaClient.h 95.65% <100.00%> (+2.31%) ⬆️
src/graph/service/PasswordAuthenticator.cpp 100.00% <100.00%> (ø)
src/graph/service/QueryEngine.h 100.00% <100.00%> (ø)
src/graph/context/Result.cpp 85.00% <0.00%> (-9.45%) ⬇️
src/graph/executor/StorageAccessExecutor.h 66.66% <0.00%> (-8.98%) ⬇️
...eta/processors/session/SessionManagerProcessor.cpp 74.19% <0.00%> (-4.52%) ⬇️
src/common/thread/GenericWorker.h 92.30% <0.00%> (-3.85%) ⬇️
... and 37 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 2c1d720...7b158da. Read the comment docs.

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.

Good job!

return true;
}

static Indexes __buildIndexes(std::vector<cpp2::IndexItem> indexItemVec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pay attention to the name style, we don't use __ as prefix.

// edgeSchemas[edgeIt.get_edge_type()].resize(schema->getVersion() + 1);
// lastEdgeType = edgeIt.get_edge_type();
// }
// edgeSchemas[edgeIt.get_edge_type()][schema->getVersion()] = std::move(schema);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just delete them, if you are sure that you are doing the right thing.

@critical27
Copy link
Contributor

There are 5 remain read lock in 667 2319 2360 2383 2395 line. leader is in line 667. leader is updated in updateLeader() outside loaddata(). leadersInfo_ is in line 2319 and 2360. leadersInfo_.leaderMap_ and leadersInfo_.pickedIndex_ is update outside loaddata(). authCheckFromCache() is in 2383 and checkShadowAccountFromCache() is in 2395, they must be const because const client in PasswordAuthenticator() call it.

I see, my bad, Github has fold some function name...

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.

Well done, LGTM

@CPWstatic CPWstatic merged commit 28776ae into vesoft-inc:master Nov 12, 2021
jackwener added a commit to jackwener/nebula that referenced this pull request Nov 15, 2021
* add one

* test

* test

* test

* add more debug Info

* Revert "add more debug Info"

This reverts commit 90ef647.

* remove

* wip

* finish

* remove redundant variable

* remove annotation

* remove more rlock

Co-authored-by: cpw <13495049+CPWstatic@users.noreply.github.com>
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.

Remove all read-write locks in meta client
4 participants