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

mvcc: chunk reads for restoring #7900

Merged
merged 2 commits into from
May 26, 2017

Conversation

heyitsanthony
Copy link
Contributor

Loading all keys at once would cause etcd to use twice as much
memory than it would need to serve the keys, causing RSS to spike on
boot. Instead, load the keys into the mvcc by chunk. Uses pipelining
for some concurrency.

Fixes #7822

tmpfs benchmarks:

Before:

go test -v -run=BenchmarkStoreRestore -bench=BenchmarkStoreRestore
BenchmarkStoreRestoreRevs1-8     1000000      4446 ns/op     814 B/op      7 allocs/op
BenchmarkStoreRestoreRevs10-8     200000     11053 ns/op    5397 B/op     38 allocs/op
BenchmarkStoreRestoreRevs20-8     100000     17458 ns/op   10518 B/op     69 allocs/op

After:

go test -bench=BenchmarkStoreRestore -run=BenchmarkStoreRestore
BenchmarkStoreRestoreRevs1-8    	 1000000	      2464 ns/op	     641 B/op	       6 allocs/op
BenchmarkStoreRestoreRevs10-8   	  200000	      7396 ns/op	    4924 B/op	      37 allocs/op
BenchmarkStoreRestoreRevs20-8   	  100000	     13664 ns/op	    9715 B/op	      68 allocs/op

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm. /cc @xiang90

@heyitsanthony heyitsanthony added this to the v3.3.0 milestone May 9, 2017
@xiang90
Copy link
Contributor

xiang90 commented May 9, 2017

LGTM. Thanks.

Anthony Romano added 2 commits May 9, 2017 20:14
Loading all keys at once would cause etcd to use twice as much
memory than it would need to serve the keys, causing RSS to spike on
boot. Instead, load the keys into the mvcc by chunk. Uses pipelining
for some concurrency.

Fixes etcd-io#7822
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7900   +/-   ##
=========================================
  Coverage          ?   75.82%           
=========================================
  Files             ?      332           
  Lines             ?    26332           
  Branches          ?        0           
=========================================
  Hits              ?    19966           
  Misses            ?     4938           
  Partials          ?     1428
Impacted Files Coverage Δ
mvcc/kvstore.go 87.37% <89.79%> (ø)

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 47f5b7c...1aca63e. Read the comment docs.

@davissp14
Copy link
Contributor

Hey guys, any way we could get this released with 3.2?

@heyitsanthony
Copy link
Contributor Author

@davissp14 can you confirm this fixes the RSS spike?

@heyitsanthony heyitsanthony merged commit 343a018 into etcd-io:master May 26, 2017
@heyitsanthony heyitsanthony deleted the chunk-restore branch May 26, 2017 16:22
@davissp14
Copy link
Contributor

I'll give this a try this afternoon.

@davissp14
Copy link
Contributor

davissp14 commented Jun 1, 2017

Just confirming this did fix the RSS spike. Thanks guys.

Also any thoughts on bringing this into 3.2 release?

@heyitsanthony
Copy link
Contributor Author

OK, I'm fine with moving this up to 3.2.

@gyuho @xiang90 any objections to slipping this into 3.2?

@gyuho
Copy link
Contributor

gyuho commented Jun 1, 2017

No objection from me. @xiang90 ?

@xiang90
Copy link
Contributor

xiang90 commented Jun 1, 2017

No objection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants