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

New gofast protobuf code #236

Merged
merged 4 commits into from
Aug 29, 2017
Merged

New gofast protobuf code #236

merged 4 commits into from
Aug 29, 2017

Conversation

cory-stripe
Copy link
Contributor

@cory-stripe cory-stripe commented Aug 28, 2017

Summary

Uses gogo-protobuf's gofast code generator for faster marshal and unmarshaling.

Motivation

I stumbled into a comment somewhere that gogo-protobuf was way faster than stock protobuf on account of it not using reflection and generating more code for marshaling and unmarshaling. I gave it a whirl and it turns out to be way faster.

Using the same benchmark we made for #234 we get these results with the gogo-protobuf "gogo-fast" compiler:

Unmarshaling (which we do a lot of):

benchmark               old ns/op     new ns/op     delta
BenchmarkParseSSF-8     521           173           -66.79%

benchmark               old allocs     new allocs     delta
BenchmarkParseSSF-8     3              2              -33.33%

benchmark               old bytes     new bytes     delta
BenchmarkParseSSF-8     384           176           -54.17%

So it's 66% faster, and uses 54% less ram with one fewer alloc. Wow! I did not test marshaling since I don't know that we even do that in this code anywhere.

Marshaling (important for client libs):

benchmark                 old ns/op     new ns/op     delta
BenchmarkMarshalSSF-8     294           28.9          -90.17%

benchmark                 old allocs     new allocs     delta
BenchmarkMarshalSSF-8     1              0              -100.00%

benchmark                 old bytes     new bytes     delta
BenchmarkMarshalSSF-8     208           0             -100.00%

Well, that's exciting. 90% reduction and zero allocations. I suspect it's because we're not writing it it to anything in the test. We emit it to a Writer in the code.

Needs

I'm not certain how to add the gogo-protobuf binary to the Dockerfile so that this works with go-generate. I assume a go get added won't DWIW, or will it?

Test plan

gogo-protobuf says it is goprotobuf compatible, so I am relying on existing unit tests + a normal test cycle in QA.

r? @aditya-stripe

@stripe-ci
Copy link

Gerald Rule: Copy Observability on Veneur and Unilog pull requests

cc @stripe/observability
cc @stripe/observability-stripe

@ChimeraCoder
Copy link
Contributor

We do marshal, within the trace package (which other Go services that use Veneur as a client library would use). Let's add a benchmark for that?

@ChimeraCoder
Copy link
Contributor

ChimeraCoder commented Aug 28, 2017

Also, you'll have to update the build process in both Travis and and Jenkins. Updating the latter will fix the build error we're getting (though we have to update the former too, or else we're not properly testing the actual code we run).

Scanning the gogo/protobuf installation instructions, adding a go get should do the right thing, assuming that's what you did locally as well.

@cory-stripe
Copy link
Contributor Author

Ok, ready for re-r @aditya-stripe now that I'm done fucking up the build.

Dockerfile Outdated
@@ -6,7 +6,8 @@ ENV GOPATH=/go
RUN apt-get update
RUN apt-get install -y zip
RUN go get -u -v github.com/ChimeraCoder/gojson/gojson
RUN go get -u -v github.com/golang/protobuf/protoc-gen-go
RUN go get -u github.com/golang/protobuf/protoc-gen-go
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for dropping the -v flag here?

@ChimeraCoder
Copy link
Contributor

Maybe let's rebase this so wtf cory isn't in our git log forever? ;)

lgtm other than that!

@cory-stripe
Copy link
Contributor Author

re-approval for rebase

@cory-stripe cory-stripe merged commit 9bd8217 into master Aug 29, 2017
@cory-stripe cory-stripe deleted the cory-faster-protobuf branch August 29, 2017 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants