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

protobuf: remove gogoproto #5342

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Conversation

jsternberg
Copy link
Collaborator

@jsternberg jsternberg commented Sep 18, 2024

Fixes #3119

Remove gogoproto in favor of the standard protobuf compiler. This
removes any nonstandard extensions that were part of gogoproto such as
the custom types.

Re-spin of #4422

@jsternberg jsternberg force-pushed the gogoproto-remove branch 2 times, most recently from 6b9629f to 4eccb90 Compare September 24, 2024 16:40
@jsternberg
Copy link
Collaborator Author

Separating the vtproto component of this into a separate PR.

@jsternberg
Copy link
Collaborator Author

We did some investigation of memory usage when transferring a large context and found some interesting stuff. The big thing we found is that a certain optimization utilized here does nothing and doesn't help with memory usage. This is because of the default grpc codec and is seen across different implementations.

The default codec will always call Reset on the underlying message. This can be seen here. The Merge is set to false by default so the reset always happens. The implementation of reset in gogo/protobuf removes the saved buffer so we always allocate memory for the data we are transferring.

This behavior is consistent with the standard go implementation which does the same thing, but it seems the standard implementation benefits a little bit from utilizing a v2 version of the codec that incorporates a memory pool. Still, it allocates a lot and that dominates the performance.

vtproto also does the same thing if you do the same thing, but vtproto can generate its own reset function that's useful for memory pools and we can also customize the codec to remove the reset if that makes sense. Either way, I haven't seen a regression in the performance so far but I think we can like do much more with vtproto if we dive into things more thoroughly.

Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

Great. Finally! Looks good to me change-wise. You still need to address the test failures though.

I'll close my PR.

go.mod Outdated
gopkg.in/yaml.v3 v3.0.1 // indirect
kernel.org/pub/linux/libs/security/libcap/psx v1.2.70 // indirect
)

replace github.com/tonistiigi/fsutil => github.com/jsternberg/fsutil v0.0.0-20240925215903-a3400686ae50
Copy link
Member

Choose a reason for hiding this comment

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

this replace can be removed and just need to update github.com/tonistiigi/fsutil now that tonistiigi/fsutil#206 is merged?

@thompson-shaun
Copy link
Collaborator

Thanks for laying all the groundwork for this @kzys -- happy it was finally addressed.

@thompson-shaun thompson-shaun added this to the v0.17.0 milestone Sep 26, 2024
@jsternberg jsternberg force-pushed the gogoproto-remove branch 2 times, most recently from abeb9a9 to 2142c51 Compare September 26, 2024 16:43
cmd/buildctl/debug/logs.go Outdated Show resolved Hide resolved
control/control.go Outdated Show resolved Hide resolved
The benchmarks that are added are related to certain areas that are
considered hotspots for performance where many messages are marshaled.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
Remove gogoproto in favor of the standard protobuf compiler. This
removes any nonstandard extensions that were part of gogoproto such as
the custom types.

Signed-off-by: Jonathan A. Sternberg <[email protected]>
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.

Migrate away from gogo
5 participants