Expose vtprotobuf generation #824
Comments
|
Did a few discussions with folks about various risks and tradeoffs The biggest concerns are around a gogo 2.0 that can't be ripped out easily. From the internal protobuf side, I am not sure how much of the details I can discuss publicly, but there was not concerns in adoption of vtprotobuf. |
Envoy tracking issue: envoyproxy/go-control-plane#824 This will be a NOP until go-control-plane completes ^. Once that happens, we will get the optimizations for free
Envoy tracking issue: envoyproxy/go-control-plane#824 This will be a NOP until go-control-plane completes ^. Once that happens, we will get the optimizations for free
Envoy tracking issue: envoyproxy/go-control-plane#824 This will be a NOP until go-control-plane completes ^. Once that happens, we will get the optimizations for free
That's what calling code should look like. And what happens when an important project writes the equivalent of: func (vtprotoCodec) Marshal(v any) ([]byte, error) {
return v.(vtprotoMessage).MarshalVT() // panics if the proto doesn't support vtprotoMessage
}Removing vtproto would be a breaking change. |
You can go down this rabbit hole with any API really. Someone can always use If someone goes out of there way to shoot themselves in the foot, I don't see any issue with breaking them -- it would be far less of a breaking change than go-control-plane has made numerous times over the years as well (not reason to do it again, but still). Also see build tag discussion in envoyproxy/envoy#31172 (comment) |
|
These aren't private fields, and my example does not use reflection or unsafe. These are exported methods, and removing them is going to be seen as less "okay". The issue with "just break people who do bad things" is: when you work for a big company (e.g. Google) and someone imports the project doing bad things into your codebase, you might not be able to upgrade your thing without fixing the people that did the bad things first. If this can go in with a build flag that is opt-in instead of opt-out, that would probably remove all of my concerns. |
This is my plan, need to merge planetscale/vtprotobuf#122 first though |
|
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions. |
See envoyproxy/go-control-plane#824 for more information Signed-off-by: John Howard <howardjohn@google.com>
See envoyproxy/go-control-plane#824 for more information This PR adds the vtprotobuf protoc plugin for Go. This works on top of the existing protoc-gen-go, to add optimized marshal functions that callers can opt in to using. This is not like gogo, which was a very invasive change -- everything is layered and opt-in. See issue for benchmarks, etc. Additionally, to avoid possible binary size increase, the entire new code is protected under a go build tag. Users will need to opt-in at build time (-tags=vtprotobuf). By default, there is no impact for users at all. Risk Level: Low - only additional opt-in code Testing: Manually tested in Istio codebase Signed-off-by: John Howard <howardjohn@google.com>
See envoyproxy/go-control-plane#824 for more information This PR adds the vtprotobuf protoc plugin for Go. This works on top of the existing protoc-gen-go, to add optimized marshal functions that callers can opt in to using. This is not like gogo, which was a very invasive change -- everything is layered and opt-in. See issue for benchmarks, etc. Additionally, to avoid possible binary size increase, the entire new code is protected under a go build tag. Users will need to opt-in at build time (-tags=vtprotobuf). By default, there is no impact for users at all. Risk Level: Low - only additional opt-in code Testing: Manually tested in Istio codebase Signed-off-by: John Howard <howardjohn@google.com> Mirrored from https://github.com/envoyproxy/envoy @ 21b52ba73d8ebbb51834d529a68f55ea2ec5e614
|
This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions. |
vtprotobuf is a protoc plugin that enables faster proto operations. See blog for introduction.
The tl;dr is that it is
gogoprotobuf, but built on top of protobuf v2, instead of replacing it. vtprotobuf works by creating strictly new files/methods, and enhancing existing types (similar to protoc-gen-validate). Users who wish to utilize these fast functions can call them, or chose not to. Generally, the presence of vtprotobuf would not even be strictly required, and the same binary would compile fine with and without (albeit slower in the latter case), as calling pattern is typically like https://github.com/vitessio/vitess/blob/40f314c1d052db3fa5a52e5fb2ddd2bddaefde44/go/vt/servenv/grpc_codec.go#L40-L49. Additionally, its actively supported by Vitess (CNCF graduation project).There are a few risks:
Mitigation, if needed: expose this only under a build tag, so a use can opt in (or opt out)
Mitigation: dropping vtprotobuf would be compatible, as noted above (just have a large performance impact)
However, the gains are tremendous. Below shows benchmarks from our control plan. Note these are not really synthetic microbenchmarks, but rather exercise our entire application; the CPU profiles of the benchmarks very closely match production profiles:
Overall we see a 40% improvement across the board, which is too good to ignore IMO
A branch of go-control-plane with these changes can be found here: https://github.com/howardjohn/go-control-plane/tree/exp/vt-2
One alternative idea I had was to simply not use go-control-plane and build our own codegen. This is not viable, however. Because go-control-plane has creeped into other core projects, such as grpc and prometheus, its ~impossible to import those packages and our own alternative generated protos. Additionally, we cannot just store the vtproto parts out of band, as methods cannot be attached to structs in other packages. As a result, our only option (AFAIK) is to include them in this repo directly.
The text was updated successfully, but these errors were encountered: