raft: don't allocate slice and sort on every commit #8689

nvanbenschoten · 2017-10-12T19:16:41Z

No description provided.

xiang90 · 2017-10-12T19:20:05Z

will this bring in an observable perf improvement for cockroachdb?

nvanbenschoten · 2017-10-13T00:06:27Z

I doubt it will be observable, but saving 2 allocations (the slice and the sort.reverse) and often avoiding the need to Sort for each committed entry seems like a win.

nvanbenschoten · 2017-10-19T21:01:39Z

Here's a benchstat diff of this change running on Proposal3Nodes:

name              old time/op    new time/op    delta
Proposal3Nodes-4    5.04µs ±26%    4.62µs ±32%   -8.35%  (p=0.033 n=20+20)

name              old alloc/op   new alloc/op   delta
Proposal3Nodes-4    4.58kB ±42%    4.34kB ±69%     ~     (p=0.192 n=20+20)

name              old allocs/op  new allocs/op  delta
Proposal3Nodes-4      11.5 ±39%       9.7 ±34%  -15.65%  (p=0.013 n=20+20)

The change is also addressing a TODO, so we should remove that TODO if we don't want to optimize this.

xiang90 · 2018-01-03T00:43:28Z

@nvanbenschoten can you rebase?

xiang90 · 2018-01-03T00:43:51Z

@siddontang can you take a look?

nvanbenschoten · 2018-01-03T01:34:51Z

@xiang90 I just rebased this off master.

siddontang · 2018-01-03T03:32:14Z

@xiang90 ok

siddontang · 2018-01-03T03:38:27Z

I suggest using a new PR for the change of Raft learner.

Rest LGTM

codecov-io · 2018-01-03T03:50:27Z

Codecov Report

Merging #8689 into master will increase coverage by 6.77%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master    #8689      +/-   ##
==========================================
+ Coverage   69.17%   75.94%   +6.77%     
==========================================
  Files         386      359      -27     
  Lines       35891    29961    -5930     
==========================================
- Hits        24827    22755    -2072     
+ Misses       9264     5625    -3639     
+ Partials     1800     1581     -219

Impacted Files	Coverage Δ
raft/raft.go	`91.64% <92.3%> (-0.28%)`	⬇️
main.go	`0% <0%> (-100%)`	⬇️
wal/file_pipeline.go	`75% <0%> (-20.46%)`	⬇️
pkg/fileutil/preallocate_unix.go	`42.85% <0%> (-14.29%)`	⬇️
etcdserver/api/v3rpc/grpc.go	`85.71% <0%> (-14.29%)`	⬇️
proxy/grpcproxy/register.go	`69.44% <0%> (-11.12%)`	⬇️
pkg/adt/interval_tree.go	`81.38% <0%> (-9.31%)`	⬇️
etcdctl/ctlv3/command/snapshot_command.go	`64.91% <0%> (-7.31%)`	⬇️
pkg/fileutil/lock_linux.go	`76.66% <0%> (-6.67%)`	⬇️
proxy/grpcproxy/auth.go	`94% <0%> (-6%)`	⬇️
... and 275 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 c5bef4f...0a415cf. Read the comment docs.

nvanbenschoten · 2018-01-03T13:18:19Z

@siddontang TFTR. I pulled the second commit out into #9088.

xiang90 · 2018-01-04T23:07:07Z

raft/raft.go

@@ -547,13 +548,23 @@ func (r *raft) bcastHeartbeatWithCtx(ctx []byte) {
 // the commit index changed (in which case the caller should call
 // r.bcastAppend).
 func (r *raft) maybeCommit() bool {
-	// TODO(bmizerany): optimize.. Currently naive
-	mis := make(uint64Slice, 0, len(r.prs))
+	if cap(r.matchBuf) < len(r.prs) {


probably document this optimization for avoiding allocations on slice creating and sorting.

xiang90 · 2018-01-04T23:08:17Z

raft/raft.go

+	}
+	mis := r.matchBuf[:len(r.prs)]
+	idx := 0
+	sorted := true
 	for _, p := range r.prs {


it seems that it is low chance that prs happens to be sorted? not sure if this optimization makes sense.

In TiKV, we still sort it every time. maybe we can write a benchmark for this.

Yeah the change that this is sorted is low - 1/(n!) if we assume that map iteration is always random. Probably not worth the extra complexity to this function. Removed.

nvanbenschoten · 2018-07-26T03:43:25Z

Reviving this after seeing this allocation account for .92% of allocations in a production CockroachDB cluster.

xiang90 · 2018-07-26T04:17:41Z

lgtm. defer to @siddontang or @gyuho

gyuho · 2018-07-26T05:32:31Z

I am on vacation till Friday. Will review by Monday. Thanks!

gyuho

lgtm. Thanks for optimization @nvanbenschoten!

This picks up etcd-io/etcd#8689. Release note: None

27998: Pick up etcd-io/etcd#8689 r=nvanbenschoten a=nvanbenschoten This picks up etcd-io/etcd#8689. Release note: None 28002: storage: avoid double lock in tryGetOrCreateReplica r=nvanbenschoten a=nvanbenschoten Before this change, `tryGetOrCreateReplica` was RLocking, checking an uncommon condition, then Locking. This was unnecessary and made the code look strange. It also resulted in an extra RLock in the exceptionally common case where the Replica was not destroyed. This change fixes this by collapsing the destroyStatus check into the main critical section. There's no real expected performance improvement here, this is mostly just for cleaner code. Release note: None Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

xiang90 added the area/raft label Oct 12, 2017

siddontang mentioned this pull request Oct 13, 2017

raft: don't allocate slice and sort on every commit tikv/tikv#2379

Closed

nvanbenschoten force-pushed the nvanbenschoten/commit branch from b5b9d61 to 1f08a33 Compare January 3, 2018 01:34

nvanbenschoten force-pushed the nvanbenschoten/commit branch from 1f08a33 to ee5a1ae Compare January 3, 2018 13:18

xiang90 reviewed Jan 4, 2018

View reviewed changes

raft: dont allocate slice and sort on every commit

0a415cf

nvanbenschoten force-pushed the nvanbenschoten/commit branch from ee5a1ae to 0a415cf Compare July 26, 2018 03:42

gyuho approved these changes Jul 27, 2018

View reviewed changes

gyuho merged commit d16c8b3 into etcd-io:master Jul 27, 2018

nvanbenschoten added a commit to cockroachdb/vendored that referenced this pull request Jul 28, 2018

Pick up etcd-io/etcd#8689

c93f18b

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Jul 28, 2018

Pick up etcd-io/etcd#8689

0946414

This picks up etcd-io/etcd#8689. Release note: None

nvanbenschoten mentioned this pull request Jul 28, 2018

Pick up coreos/etcd#8689 cockroachdb/cockroach#27998

Merged

absolute8511 mentioned this pull request Mar 13, 2019

Consider merge some optimize from etcd-raft youzan/ZanRedisDB#23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raft: don't allocate slice and sort on every commit #8689

raft: don't allocate slice and sort on every commit #8689

nvanbenschoten commented Oct 12, 2017

xiang90 commented Oct 12, 2017

nvanbenschoten commented Oct 13, 2017

nvanbenschoten commented Oct 19, 2017

xiang90 commented Jan 3, 2018

xiang90 commented Jan 3, 2018

nvanbenschoten commented Jan 3, 2018

siddontang commented Jan 3, 2018

siddontang commented Jan 3, 2018

codecov-io commented Jan 3, 2018 •

edited

Loading

nvanbenschoten commented Jan 3, 2018

xiang90 Jan 4, 2018

nvanbenschoten Jul 26, 2018

xiang90 Jan 4, 2018

siddontang Jan 5, 2018

nvanbenschoten Jul 26, 2018

nvanbenschoten commented Jul 26, 2018

xiang90 commented Jul 26, 2018

gyuho commented Jul 26, 2018

gyuho left a comment

raft: don't allocate slice and sort on every commit #8689

raft: don't allocate slice and sort on every commit #8689

Conversation

nvanbenschoten commented Oct 12, 2017

xiang90 commented Oct 12, 2017

nvanbenschoten commented Oct 13, 2017

nvanbenschoten commented Oct 19, 2017

xiang90 commented Jan 3, 2018

xiang90 commented Jan 3, 2018

nvanbenschoten commented Jan 3, 2018

siddontang commented Jan 3, 2018

siddontang commented Jan 3, 2018

codecov-io commented Jan 3, 2018 • edited Loading

Codecov Report

nvanbenschoten commented Jan 3, 2018

xiang90 Jan 4, 2018

Choose a reason for hiding this comment

nvanbenschoten Jul 26, 2018

Choose a reason for hiding this comment

xiang90 Jan 4, 2018

Choose a reason for hiding this comment

siddontang Jan 5, 2018

Choose a reason for hiding this comment

nvanbenschoten Jul 26, 2018

Choose a reason for hiding this comment

nvanbenschoten commented Jul 26, 2018

xiang90 commented Jul 26, 2018

gyuho commented Jul 26, 2018

gyuho left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 3, 2018 •

edited

Loading