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

Improve protobuf serialization #2031

Merged
merged 3 commits into from
May 11, 2020
Merged

Conversation

cyriltovena
Copy link
Contributor

This PR is inspired by Cortex where the use of custom protobuf types allows to improve performance of serialization.(see https://github.com/cortexproject/cortex/blob/8fe97dcc707a3e774229d9fd3b20425214e0ac9b/pkg/ingester/client/timeseries.go#L174)

This contains 2 optimization:

I've run this for a while in a dev environment and everything looks good. This mostly affect everything that serializes Streams.

Benchmark:

❯ go test github.com/grafana/loki/pkg/logproto -bench=.
goos: darwin
goarch: amd64
pkg: github.com/grafana/loki/pkg/logproto
BenchmarkStream-16           	 1000000	      1163 ns/op	    1696 B/op	       4 allocs/op
BenchmarkStreamAdapter-16    	  532299	      2171 ns/op	    4016 B/op	      29 allocs/op
PASS
ok  	github.com/grafana/loki/pkg/logproto	2.371s

On drawback of custom type is gogo/protobuf#509 However I've made sure that a pointer is used where needed.

This is just a step forward I think next step is to pre-allocate data for request/response like Cortex does.

This is the first step to improve protobuf serialization.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Copy link
Contributor Author

This shows the GC rate change during a load tests after deployment:

image

@cyriltovena
Copy link
Contributor Author

The change from []*logproto.Stream to => []logproto.Stream might have an impact on the memory traffic /cc @tomwilkie WDYT ?

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Copy link
Contributor Author

The change from []*logproto.Stream to => []logproto.Stream might have an impact on the memory traffic /cc @tomwilkie WDYT ?

This seems to be just the result of less GC. I don't see anything to worry except less GC and allocs and more memory used.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

AFAICT This should be good unless we increase the size of logproto.Stream significantly.

type Stream struct {
	Labels  string  `protobuf:"bytes,1,opt,name=labels,proto3" json:"labels"`
	Entries []Entry `protobuf:"bytes,2,rep,name=entries,proto3" json:"entries"`
}

By preferring Stream to *Stream, we're allocating the size of string and []Entry on the stack instead of a single pointer referencing the heap. I expect if we keep the struct definition small the net benefit of stack allocation will outweigh heap allocation.

@cyriltovena
Copy link
Contributor Author

Adding limits to ingester made the memory acts normal.

@cyriltovena cyriltovena merged commit 9988ce4 into grafana:master May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants