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

decide for _one_ protobuf compiler, use (and enforce) it everywhere, run it on CI #1976

Closed
marten-seemann opened this issue Jan 4, 2023 · 2 comments · Fixed by #1979
Closed
Labels
effort/hours Estimated to take one or several hours kind/maintenance Work required to avoid breaking changes or harm to project's status quo

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented Jan 4, 2023

We're currently using both gogofast and gogofaster. Both of them are deprecated: https://github.com/gogo/protobuf#deprecated-protocol-buffers-for-go-with-gadgets

p2p/protocol/internal/circuitv1-deprecated/pb/Makefile
7:              protoc --proto_path=$(GOPATH)/src:. --gogofast_out=. $<

p2p/protocol/circuitv2/pb/Makefile
7:              protoc  --gogofast_out=. $<

p2p/protocol/holepunch/pb/Makefile
7:              protoc --proto_path=$(GOPATH)/src:. --gogofast_out=. $<

p2p/protocol/circuitv1/pb/Makefile
7:              protoc --gogofast_out=. $<

p2p/security/noise/pb/Makefile
7:              protoc --proto_path=$(PWD):$(PWD)/../.. --gogofaster_out=. $<

p2p/protocol/identify/pb/Makefile
7:              protoc --proto_path=$(GOPATH)/src:. --gogofast_out=. $<

p2p/host/autonat/pb/Makefile
6:      protoc --gogofast_out=. --proto_path=$(GOPATH)/src:. $<

p2p/host/peerstore/pb/Makefile
7:              protoc --proto_path=$(GOPATH)/src:. --gogofaster_out=. $<

core/peer/pb/Makefile
7:              protoc --proto_path=$(PWD):$(PWD)/../.. --gogofaster_out=. $<

core/record/pb/Makefile
7:              protoc --proto_path=$(PWD):$(PWD)/../.. --gogofaster_out=. $<

core/crypto/pb/Makefile
7:              protoc --proto_path=$(PWD)/../..:. --gogofaster_out=. $<

core/introspection/pb/Makefile
7:      protoc --proto_path=$(PWD):$(PWD)/../..:$(GOPATH)/src --gogofaster_out=Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types:. $<

core/sec/insecure/pb/Makefile
7:              protoc --proto_path=$(GOPATH)/src:../../../crypto/pb:. --gogofaster_out=. $<

We should decide for one compiler to use, to be consistent. We should also run go generate / make on CI, to make sure that our Protobuf code is consistent with the proto definitions.

@marten-seemann marten-seemann added kind/maintenance Work required to avoid breaking changes or harm to project's status quo effort/hours Estimated to take one or several hours labels Jan 4, 2023
@marten-seemann
Copy link
Contributor Author

I added a benchmark test in a branch here: https://github.com/libp2p/go-libp2p/compare/benchmark-identify-protobuf?expand=1. Note that this is benchmarking a proto2.

Not sure if we want to merge this into master, since it's actually just a benchmark of the protobuf library.

Using GoGo protobuf:

goos: darwin
goarch: arm64
pkg: github.com/libp2p/go-libp2p/p2p/protocol/identify
BenchmarkIdentifyProtobufMarshal-10              2770962               404.3 ns/op           896 B/op          1 allocs/op
BenchmarkIdentifyProtobufUnmarshal-10             741806              1473 ns/op            2328 B/op         45 allocs/op

Using Google protobuf:

goos: darwin
goarch: arm64
pkg: github.com/libp2p/go-libp2p/p2p/protocol/identify
BenchmarkIdentifyProtobufMarshal-10              2404834               496.0 ns/op           896 B/op          1 allocs/op
BenchmarkIdentifyProtobufUnmarshal-10             643617              1675 ns/op            2312 B/op         45 allocs/op

This confirms that Google protobuf is slower than GoGo (about 20%). I would have expected the difference to be much bigger. I get similar results for proto3.

@MarcoPolo
Copy link
Collaborator

Given that gogo protobuf is deprecated, and the slow down is small relative to network overhead. I think it makes sense to use google protobuf.

If users need that 20% they can always make sure to use gogo for their own protocols. We don’t send that many protobufs in core. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours kind/maintenance Work required to avoid breaking changes or harm to project's status quo
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants