raft: add (*RawNode).WithProgress by tbg · Pull Request #10309 · etcd-io/etcd

tbg · 2018-12-06T11:17:19Z

Calls to Status can be frequent and currently incur three heap
allocations, but often the caller has no intention to hold on to the
returned status.

Add StatusWithoutProgress and WithProgress to allow avoiding heap
allocations altogether. StatusWithoutProgress does what's on the
tin and additionally returns a value (instead of a pointer) to
avoid the associated heap allocation. By not returning a Progress
map, it avoids all other allocations that Status incurs.

To still introspect the Progress map, add WithProgress, which
uses a simple visitor pattern.

Add benchmarks to verify that this is indeed allocation free.

BenchmarkStatusProgress/members=1/Status-8                  5000000    353 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=1/Status-example-8          5000000    372 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=1/StatusWithoutProgress-8   100000000  17.6 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=1/WithProgress-8            30000000   48.6 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=1/WithProgress-example-8    30000000   42.9 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=3/Status-8                  5000000    395 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=3/Status-example-8          3000000    449 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=3/StatusWithoutProgress-8   100000000  18.7 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=3/WithProgress-8            20000000   78.1 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=3/WithProgress-example-8    20000000   70.7 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=5/Status-8                  3000000    470 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=5/Status-example-8          3000000    544 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=5/StatusWithoutProgress-8   100000000  19.7 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=5/WithProgress-8            20000000   105 ns/op          0 B/op    0 allocs/op
BenchmarkStatusProgress/members=5/WithProgress-example-8    20000000   94.0 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=100/Status-8                100000     11903 ns/op    22663 B/op   12 allocs/op
BenchmarkStatusProgress/members=100/Status-example-8        100000     13330 ns/op    22669 B/op   12 allocs/op
BenchmarkStatusProgress/members=100/StatusWithoutProgress-8 50000000   20.9 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=100/WithProgress-8          1000000    1731 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=100/WithProgress-example-8  1000000    1571 ns/op         0 B/op    0 allocs/op

tbg · 2018-12-06T11:17:39Z

cc @nvanbenschoten

tbg · 2018-12-06T11:48:20Z

CI failed due to what looks to be an unrelated flake:

=== RUN   TestLeasingTxnOwnerIf
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x60 pc=0xafbf8d]
goroutine 19250 [running]:
go.etcd.io/etcd/vendor/google.golang.org/grpc.(*csAttempt).sendMsg(0xc0000ee420, 0x14b4220, 0xc002350470, 0xc0023168db, 0x5, 0x5, 0xc0023168d0, 0xb, 0xb, 0xc0023168d0, ...)
	/go/src/go.etcd.io/etcd/vendor/google.golang.org/grpc/stream.go:713 +0x14d
go.etcd.io/etcd/vendor/google.golang.org/grpc.(*clientStream).SendMsg.func2(0xc0000ee420, 0x48, 0x48)
	/go/src/go.etcd.io/etcd/vendor/google.golang.org/grpc/stream.go:637 +0x186
go.etcd.io/etcd/vendor/google.golang.org/grpc.(*clientStream).withRetry(0xc0000ce6c0, 0xc000031bd0, 0xc001993aa0, 0xc0023168db, 0x5)
	/go/src/go.etcd.io/etcd/vendor/google.golang.org/grpc/stream.go:530 +0x431
go.etcd.io/etcd/vendor/google.golang.org/grpc.(*clientStream).SendMsg(0xc0000ce6c0, 0x14b4220, 0xc002350470, 0x0, 0x0)
	/go/src/go.etcd.io/etcd/vendor/google.golang.org/grpc/stream.go:643 +0x80c
go.etcd.io/etcd/etcdserver/etcdserverpb.(*watchWatchClient).Send(0xc002350460, 0xc002350470, 0x0, 0x206e9b8)
	/go/src/go.etcd.io/etcd/etcdserver/etcdserverpb/rpc.pb.go:3618 +0x6b
go.etcd.io/etcd/clientv3.(*watchGrpcStream).run(0xc0000336c0)
	/go/src/go.etcd.io/etcd/clientv3/watch.go:542 +0xd84
created by go.etcd.io/etcd/clientv3.(*watcher).newWatcherGrpcStream
	/go/src/go.etcd.io/etcd/clientv3/watch.go:277 +0x58c

codecov-io · 2018-12-06T12:30:10Z

Codecov Report

Merging #10309 into master will increase coverage by 0.16%.
The diff coverage is 26.66%.

@@            Coverage Diff             @@
##           master   #10309      +/-   ##
==========================================
+ Coverage   71.54%   71.71%   +0.16%     
==========================================
  Files         391      391              
  Lines       36397    36406       +9     
==========================================
+ Hits        26042    26107      +65     
+ Misses       8538     8475      -63     
- Partials     1817     1824       +7

Impacted Files	Coverage Δ
raft/rawnode.go	`66.93% <0%> (-1.66%)`	⬇️
raft/status.go	`29.72% <33.33%> (+3.92%)`	⬆️
proxy/grpcproxy/register.go	`69.44% <0%> (-11.12%)`	⬇️
pkg/testutil/recorder.go	`77.77% <0%> (-3.71%)`	⬇️
clientv3/concurrency/election.go	`79.68% <0%> (-2.35%)`	⬇️
clientv3/maintenance.go	`69.38% <0%> (-2.05%)`	⬇️
raft/progress.go	`94.17% <0%> (-1.95%)`	⬇️
etcdserver/server.go	`73.45% <0%> (-1.44%)`	⬇️
clientv3/client.go	`78.02% <0%> (-0.85%)`	⬇️
raft/raft.go	`91.49% <0%> (-0.66%)`	⬇️
... and 18 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 510ae3d...e20d3be. Read the comment docs.

bdarnell · 2018-12-06T16:16:55Z

raft/rawnode.go

+// function and passes it a Status with a nil Progress field and pointers to the
+// RawNode's internal Progress for its peers and learners, respectively. These
+// must not be mutated or held on to past the next operation on the RawNode.
+func (rn *RawNode) WithUnsafeStatus(visitor func(s Status, prs, learnerPRs map[uint64]*Progress)) {


I might just call this WithStatus. A With(func) method already implies a warning (at least to me) against using anything outside of its scope.

Exposing the prs and learnerPRs maps separately is unfortunate; this makes it harder for us to change the internal structure of things that are used to derive Status. It might be nice to define a struct (UnsafeStatus, reversing my previous paragraph?) that contains these things so that applications that don't need learners (for example) don't have their builds broken if there is an internal change there.

Nathan suggested splitting the "status" and "progress" portions which I did and I think it turned out much nicer and avoids this problem as well. PTAL.

nvb · 2018-12-06T17:34:39Z

raft/status.go

 	}
-
 	s.HardState = r.hardState()
 	s.SoftState = *r.softState()


I think this is still hitting an allocation. Or maybe escape analysis is smart enough to avoid it?

The benchmarks indicate that escape analysis is smart enough.

Calls to Status can be frequent and currently incur three heap allocations, but often the caller has no intention to hold on to the returned status. Add StatusWithoutProgress and WithProgress to allow avoiding heap allocations altogether. StatusWithoutProgress does what's on the tin and additionally returns a value (instead of a pointer) to avoid the associated heap allocation. By not returning a Progress map, it avoids all other allocations that Status incurs. To still introspect the Progress map, add WithProgress, which uses a simple visitor pattern. Add benchmarks to verify that this is indeed allocation free. ``` BenchmarkStatusProgress/members=1/Status-8 5000000 353 ns/op 784 B/op 3 allocs/op BenchmarkStatusProgress/members=1/Status-example-8 5000000 372 ns/op 784 B/op 3 allocs/op BenchmarkStatusProgress/members=1/StatusWithoutProgress-8 100000000 17.6 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=1/WithProgress-8 30000000 48.6 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=1/WithProgress-example-8 30000000 42.9 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=3/Status-8 5000000 395 ns/op 784 B/op 3 allocs/op BenchmarkStatusProgress/members=3/Status-example-8 3000000 449 ns/op 784 B/op 3 allocs/op BenchmarkStatusProgress/members=3/StatusWithoutProgress-8 100000000 18.7 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=3/WithProgress-8 20000000 78.1 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=3/WithProgress-example-8 20000000 70.7 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=5/Status-8 3000000 470 ns/op 784 B/op 3 allocs/op BenchmarkStatusProgress/members=5/Status-example-8 3000000 544 ns/op 784 B/op 3 allocs/op BenchmarkStatusProgress/members=5/StatusWithoutProgress-8 100000000 19.7 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=5/WithProgress-8 20000000 105 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=5/WithProgress-example-8 20000000 94.0 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=100/Status-8 100000 11903 ns/op 22663 B/op 12 allocs/op BenchmarkStatusProgress/members=100/Status-example-8 100000 13330 ns/op 22669 B/op 12 allocs/op BenchmarkStatusProgress/members=100/StatusWithoutProgress-8 50000000 20.9 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=100/WithProgress-8 1000000 1731 ns/op 0 B/op 0 allocs/op BenchmarkStatusProgress/members=100/WithProgress-example-8 1000000 1571 ns/op 0 B/op 0 allocs/op ```

tbg · 2018-12-06T18:10:25Z

Ready for another look. I also added the benchmark results to the PR message. (TLDR is no allocs and faster).

xiang90 · 2018-12-06T18:56:34Z

lgtm

nvb

LGTM. The ProgressType enum was a good idea.

tbg requested a review from bdarnell December 6, 2018 11:17

bdarnell reviewed Dec 6, 2018

View reviewed changes

nvb reviewed Dec 6, 2018

View reviewed changes

tbg force-pushed the fix/raft-status-allocs branch from e20d3be to bd332b3 Compare December 6, 2018 18:06

tbg changed the title ~~raft: add (*RawNode).WithUnsafeStatus~~ raft: add (*RawNode).WithProgress Dec 6, 2018

bdarnell approved these changes Dec 6, 2018

View reviewed changes

xiang90 merged commit 8a9a2a1 into etcd-io:master Dec 6, 2018

nvb reviewed Dec 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

raft: add (*RawNode).WithProgress#10309

raft: add (*RawNode).WithProgress#10309
xiang90 merged 1 commit intoetcd-io:masterfrom
tbg:fix/raft-status-allocs

tbg commented Dec 6, 2018 •

edited

Loading

Uh oh!

tbg commented Dec 6, 2018

Uh oh!

tbg commented Dec 6, 2018

Uh oh!

codecov-io commented Dec 6, 2018

Uh oh!

bdarnell Dec 6, 2018

Uh oh!

tbg Dec 6, 2018

Uh oh!

nvb Dec 6, 2018

Uh oh!

tbg Dec 6, 2018

Uh oh!

tbg commented Dec 6, 2018

Uh oh!

xiang90 commented Dec 6, 2018

Uh oh!

nvb left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

Conversation

tbg commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbg commented Dec 6, 2018

Uh oh!

tbg commented Dec 6, 2018

Uh oh!

codecov-io commented Dec 6, 2018

Codecov Report

Uh oh!

bdarnell Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

tbg Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

nvb Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

tbg Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

tbg commented Dec 6, 2018

Uh oh!

xiang90 commented Dec 6, 2018

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

tbg commented Dec 6, 2018 •

edited

Loading