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

*: reduce the store clone #1410

Merged
merged 6 commits into from
Jan 30, 2019
Merged

*: reduce the store clone #1410

merged 6 commits into from
Jan 30, 2019

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Jan 14, 2019

What problem does this PR solve?

When there are lots of stores, GetStores will cost a lot of time due to Clone.

What is changed and how it works?

This PR tries to reduce the store clone when using GetStore and GetStores.

Check List

Tests

  • Unit test

@rleungx rleungx requested a review from nolouch January 14, 2019 11:24
nolouch
nolouch reviewed Jan 14, 2019
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

Can you share the benchmark result?

@rleungx rleungx added the type/enhancement The issue or PR belongs to an enhancement. label Jan 16, 2019
disksing
disksing reviewed Jan 17, 2019
View reviewed changes
Copy link
Contributor

@disksing disksing left a comment

Choose a reason for hiding this comment

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

the rest LGTM

server/core/store.go Outdated Show resolved Hide resolved
server/core/store.go Outdated Show resolved Hide resolved
@rleungx rleungx force-pushed the reduce-store-clone branch from 9aa1f73 to ea9a0bb Compare January 17, 2019 06:52
@codecov-io
Copy link

codecov-io commented Jan 17, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@8d6c936). Click here to learn what that means.
The diff coverage is 90.2%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1410   +/-   ##
=========================================
  Coverage          ?   67.81%           
=========================================
  Files             ?      158           
  Lines             ?    14986           
  Branches          ?        0           
=========================================
  Hits              ?    10163           
  Misses            ?     3901           
  Partials          ?      922
Impacted Files Coverage Δ
server/schedulers/balance_leader.go 91.39% <100%> (ø)
server/grpc_service.go 53.65% <100%> (ø)
server/schedule/replica.go 100% <100%> (ø)
server/schedule/range_cluster.go 87.5% <100%> (ø)
server/schedulers/random_merge.go 65.78% <100%> (ø)
server/core/kv.go 76.47% <100%> (ø)
server/api/trend.go 68.75% <100%> (ø)
server/schedule/operator.go 89.12% <100%> (ø)
server/schedulers/label.go 82.6% <100%> (ø)
server/api/store.go 64.9% <100%> (ø)
... and 22 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 8d6c936...1ab3205. Read the comment docs.

@rleungx rleungx force-pushed the reduce-store-clone branch from ea9a0bb to 11bb9b7 Compare January 17, 2019 09:08
@tikv tikv deleted a comment from nolouch Jan 17, 2019
@rleungx rleungx added the DNM label Jan 18, 2019
nolouch
nolouch approved these changes Jan 23, 2019
View reviewed changes
Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

LGTM

@rleungx rleungx force-pushed the reduce-store-clone branch from 3f6e2b2 to 01984c1 Compare January 24, 2019 06:41
@rleungx
Copy link
Member Author

rleungx commented Jan 24, 2019

Here are the benchmark results:

before:
PASS: cluster_test.go:597: testGetStoresSuite.BenchmarkGetStores	    5000	    695042 ns/op
after:
PASS: cluster_test.go:597: testGetStoresSuite.BenchmarkGetStores	  500000	      4624 ns/op

PTAL @disksing @nolouch @siddontang

@huachaohuang
Copy link
Contributor

/rebuild

@rleungx
reduce store clone
26789bf 
Signed-off-by: rleungx <[email protected]>
@rleungx
address comments
97fdc81 
Signed-off-by: rleungx <[email protected]>
@rleungx
tiny fixs
7a08580 
Signed-off-by: rleungx <[email protected]>
@rleungx
add the benchmark
077dfa5 
Signed-off-by: rleungx <[email protected]>
@rleungx
tiny clean up
1ab3205 
Signed-off-by: rleungx <[email protected]>
@rleungx rleungx force-pushed the reduce-store-clone branch from 63fdd4b to 1ab3205 Compare January 28, 2019 03:12
@rleungx rleungx removed the DNM label Jan 28, 2019
@huachaohuang
Copy link
Contributor

The result looks awesome, but please fix the CI.

@rleungx
Copy link
Member Author

rleungx commented Jan 30, 2019

/rebuild

@rleungx
Copy link
Member Author

rleungx commented Jan 30, 2019

The CI problem is not related to this PR. @huachaohuang

@nolouch
Copy link
Contributor

nolouch commented Jan 30, 2019

/rebuild

@huachaohuang huachaohuang merged commit 9db4a56 into tikv:master Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@disksing disksing disksing approved these changes

@nolouch nolouch nolouch approved these changes

Assignees
No one assigned
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rleungx @codecov-io @huachaohuang @nolouch @disksing