Make commitlog msgpack encoder really, really fast. #1160

richardartoul · 2018-11-07T21:41:15Z

The basic gist of this P.R is that the old code had too many function calls. Function call overhead in Golang is small, but when every function does nothing more than write a few bytes to an array in memory, the function call overhead begins to dominate. As a result, what we do in this P.R is create a fast path that manually inlines (read: copy-pastes) a lot of the helper functions. This makes the code more verbose and more difficult to maintain, but we get an over 10x speedup in exchange which is worth it as this is the hottest code path in M3DB and bottlenecks everything else.

Old path log entry

goos: darwin
goarch: amd64
pkg: github.com/m3db/m3/src/dbnode/persist/fs/msgpack
BenchmarkLogEntryDecoder-8   	 3000000	       611 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/m3db/m3/src/dbnode/persist/fs/msgpack	2.423s
Success: Benchmarks passed.

New path log entry

goos: darwin
goarch: amd64
pkg: github.com/m3db/m3/src/dbnode/persist/fs/msgpack
BenchmarkLogEntryEncoderFast-8   	30000000	        43.9 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/m3db/m3/src/dbnode/persist/fs/msgpack	1.381s
Success: Benchmarks passed.

Old path log metadata

goos: darwin
goarch: amd64
pkg: github.com/m3db/m3/src/dbnode/persist/fs/msgpack
BenchmarkLogMetadataEncoder-8   	 5000000	       235 ns/op	     130 B/op	       0 allocs/op
PASS
ok  	github.com/m3db/m3/src/dbnode/persist/fs/msgpack	1.526s
Success: Benchmarks passed.

New path log metadata

goos: darwin
goarch: amd64
pkg: github.com/m3db/m3/src/dbnode/persist/fs/msgpack
BenchmarkLogMetadataEncoderFast-8   	50000000	        31.9 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/m3db/m3/src/dbnode/persist/fs/msgpack	1.648s
Success: Benchmarks passed.

codecov · 2018-11-07T21:54:17Z

Codecov Report

Merging #1160 into master will decrease coverage by <.1%.
The diff coverage is 64%.

@@           Coverage Diff            @@
##           master   #1160     +/-   ##
========================================
- Coverage    71.6%   71.5%   -0.1%     
========================================
  Files         725     726      +1     
  Lines       60644   60839    +195     
========================================
+ Hits        43422   43518     +96     
- Misses      14459   14535     +76     
- Partials     2763    2786     +23

Flag	Coverage Δ
#aggregator	`81.6% <ø> (-0.1%)`	⬇️
#cluster	`85.9% <ø> (ø)`	⬆️
#collector	`78.1% <ø> (ø)`	⬆️
#dbnode	`81% <64%> (-0.3%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`75.2% <ø> (-0.1%)`	⬇️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`18.3% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (-0.3%)`	⬇️
#query	`64.2% <ø> (-0.1%)`	⬇️
#x	`75.2% <ø> (ø)`	⬆️

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 95dbe5a...ab50047. Read the comment docs.

isaachier · 2018-11-07T22:02:36Z

src/dbnode/persist/fs/msgpack/encoder_fast.go

+// any performance gains that can be had in this function are worth it.
+//
+// Before modifying this function, please run the BenchmarkLogEntryEncoderFast benchmark as a small
+// degration in this functions performance can have a substantial impact on M3DB.


degradation typo

robskillington · 2018-11-07T23:31:00Z

src/dbnode/persist/fs/commitlog/writer.go

@@ -129,6 +130,7 @@ func newCommitLogWriter(
 		sizeBuffer:         make([]byte, binary.MaxVarintLen64),
 		seen:               bitset.NewBitSet(defaultBitSetLength),
 		logEncoder:         msgpack.NewEncoder(),
+		logEncoderBuff:     make([]byte, 0, 16384),


Nit: make size a const (I know this was me hah).

Haha will fix

robskillington · 2018-11-07T23:31:37Z

src/dbnode/persist/fs/msgpack/encoder_decoder_bench_test.go

+var benchmarkBuf []byte
+
+func BenchmarkLogEntryEncoderFast(b *testing.B) {
+	var (


robskillington · 2018-11-07T23:42:22Z

src/dbnode/persist/fs/msgpack/encoder_fast.go

+
+// encodeVarUint64 is used in EncodeLogEntryFast which is a very hot path.
+// As a result, many of the function calls in this function have been
+// manually inlined to reduce function call overhead.


So I’ve read this code a few times, the manual inlining makes a lot of sense. Having said that it might be worth using Genny to code gen the code so that if we really need to change parts of the code it affects every party (DRY it up).

Genny should be able to code gen all the inlined parts, it’s more work for sure, so I’m fine if with TODO it but we should definitely do this at some point (otherwise the copy paste manual inlining thing may become a pattern and code gen is definitely better than what manual copy paste to inline things).

Thoughts?

Yeah I thought about doing that too, but I’d prefer to leave as a TODO for now. Mainly because:

Hopefully this doesn’t become a pattern for us

This is really unplanned work and there’s like 5 other things I’m supposed to be doing that are not performance improvements and I have a feeling getting the genny thing right will take a little bit of back and forth

This PR makes sense only if you believe Go won't inline your function in the near future. Try debugging this the explanation here in the Go compiler: https://github.com/golang/go/blob/master/src/cmd/compile/internal/gc/inl.go#L5-L25.

I think the benchmarks demonstrate that this P.R is worth it. Even if Golang inlining gets better in the near-future, this function is so hot and the number of function calls we removed so high (we're talking about literally dozens of function calls per iteration that have been removed) I highly doubt that the Go compiler will be able to achieve a similar effect anytime soon

robskillington · 2018-11-07T23:45:10Z

src/dbnode/persist/fs/msgpack/encoder_fast.go

+
+// encodeBytesLen is not used, it is left here to demonstrate what a manually inlined
+// version of this function should look like.
+func encodeBytesLen(b []byte, l int) []byte {


Is this called? Looks Inlined elsewhere. This is where codgen would help (some being reused while others being used as real calls, everything can just always be inlined potentially).

Nope not called (if it is, it’s because I forgot to in-line it). I just left these in as “documention” per the comment

This reverts commit f92d26b.

This reverts commit a22e1bf.

This reverts commit bb6a808.

robskillington

LGTM

robskillington · 2018-11-08T21:01:20Z

src/dbnode/persist/fs/commitlog/writer.go

+		w.metadataEncoderBuff = w.metadataEncoderBuff[:0]
+		var err error
+
+		w.metadataEncoderBuff, err = msgpack.EncodeLogMetadataFast(w.metadataEncoderBuff, metadata)


p.s. I realized this can be collapsed somewhat to do the [:0] just as you pass it to the EncodeLogMetadataFast method:

var err error w.metadataEncoderBuff, err = msgpack.EncodeLogMetadataFast(w.metadataEncoderBuff[:0], metadata)

robskillington · 2018-11-08T21:06:13Z

src/dbnode/persist/fs/commitlog/writer.go

+	w.logEncoderBuff = w.logEncoderBuff[:0]
+
+	var err error
+	w.logEncoderBuff, err = msgpack.EncodeLogEntryFast(w.logEncoderBuff, logEntry)


Same here, can collapse to w.logEncoderBuff, err = msgpack.EncodeLogEntryFast(w.logEncoderBuff[:0], logEntry)

richardartoul requested review from robskillington, prateek and justinjc November 7, 2018 21:45

richardartoul mentioned this pull request Nov 7, 2018

[WIP] Add fast commit log encoding path #1159

Closed

isaachier reviewed Nov 7, 2018

View reviewed changes

robskillington reviewed Nov 7, 2018

View reviewed changes

Richard Artoul added 15 commits November 8, 2018 13:51

Convert commitlog chan to batches

198417a

refactor

984eb42

Add write batch function

f3a966c

Fast commitlog path

62f263b

Add comments

5d2bdde

Break benchmarks into separate file

40420d6

Move fast encoder to its own file

f4eede3

Revert "Add write batch function"

46e3ef0

This reverts commit f92d26b.

Revert "refactor"

c84e95a

This reverts commit a22e1bf.

Revert "Convert commitlog chan to batches"

6f8527c

This reverts commit bb6a808.

Fix benchmark

c4e93cf

Fix imports

a0db923

Improve comment

a342994

Fix typo

b07b1c9

Optimize encoding log metadata as well

8fc4f8a

richardartoul force-pushed the ra/fast-commitlog branch from 2d095f0 to 8fc4f8a Compare November 8, 2018 18:51

Richard Artoul added 3 commits November 8, 2018 14:01

Use fast path for encoding log metadata in commitlog

39a6dcb

Automatically reset buffers if they get too big and improve comments

9ca2313

Improve comment

f63a8c1

robskillington approved these changes Nov 8, 2018

View reviewed changes

robskillington reviewed Nov 8, 2018

View reviewed changes

Comment out dead code

d0371af

Richard Artoul added 3 commits November 8, 2018 16:36

Remove unnecesary lines

de9ab7e

Fix bug

c9a0525

sort imports

ab50047

richardartoul merged commit a2fab37 into master Nov 12, 2018

justinjc deleted the ra/fast-commitlog branch January 7, 2019 19:31

richardartoul mentioned this pull request Feb 24, 2019

Commit Log Write Performance Improvements #947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make commitlog msgpack encoder really, really fast. #1160

Make commitlog msgpack encoder really, really fast. #1160

richardartoul commented Nov 7, 2018 •

edited

Loading

codecov bot commented Nov 7, 2018 •

edited

Loading

isaachier Nov 7, 2018

robskillington Nov 7, 2018

richardartoul Nov 8, 2018

robskillington Nov 7, 2018

robskillington Nov 7, 2018

richardartoul Nov 8, 2018

isaachier Nov 8, 2018

richardartoul Nov 8, 2018

robskillington Nov 7, 2018

richardartoul Nov 8, 2018

robskillington left a comment

robskillington Nov 8, 2018

robskillington Nov 8, 2018

Make commitlog msgpack encoder really, really fast. #1160

Make commitlog msgpack encoder really, really fast. #1160

Conversation

richardartoul commented Nov 7, 2018 • edited Loading

codecov bot commented Nov 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardartoul commented Nov 7, 2018 •

edited

Loading

codecov bot commented Nov 7, 2018 •

edited

Loading