Skip to content

encoding/proto: enable use cached size option #8569

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 15, 2025
Merged

encoding/proto: enable use cached size option #8569

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19a8cef
proto: enable use cached size option
rs-unity Sep 10, 2025
188bece
proto: move MarshalOptions closer to size call and add comment clarif…
rs-unity Sep 11, 2025
50dfe64
proto: revert import sort
rs-unity Sep 11, 2025
ec0c47e
proto: improve comment
rs-unity Sep 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions encoding/proto/proto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,33 @@ func (c *codecV2) Marshal(v any) (data mem.BufferSlice, err error) {
return nil, fmt.Errorf("proto: failed to marshal, message is %T, want proto.Message", v)
}

// Important: if we remove this Size call then we cannot use
// UseCachedSize in MarshalOptions below.
size := proto.Size(vv)
Copy link
Contributor

@arjan-bal arjan-bal Sep 11, 2025

Choose a reason for hiding this comment

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

Since this seems to rely on a runtime behaviour of calling proto.Size(vv) before using the cached size (please correct me if I'm wrong), we should call this out in a code comment just above this to serve as a warning for future code authors.

Copy link
Contributor

@arjan-bal arjan-bal Sep 11, 2025

Choose a reason for hiding this comment

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

I think if we don't call proto.Size(vv) here, we could end up using a stale cached size.

Copy link
Contributor Author

@rs-unity rs-unity Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Would this be cleaner? rs-unity@188bece rs-unity@ec0c47e

Copy link
Contributor

@easwars easwars Sep 11, 2025

Choose a reason for hiding this comment

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

The docstring of UseCachedSize does say the following:

	// UseCachedSize indicates that the result of a previous Size call
	// may be reused.****

And that if that condition is not met, bad things might happen.

Copy link
Contributor Author

@rs-unity rs-unity Sep 11, 2025

Choose a reason for hiding this comment

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

I just want to flag an edge case that normally shouldn’t happen: if another part of the stack keeps a reference to the proto message and it’s shared across multiple Go routines, one routine might modify the message while another is marshaling it through this codec. In that situation, enabling UseCachedSize becomes risky.

That said, the issue I’m describing would still cause (other) problems even if UseCachedSize isn’t enabled.

Copy link
Contributor Author

@rs-unity rs-unity Sep 11, 2025

Choose a reason for hiding this comment

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

I've updated the comment: rs-unity@ec0c47e


// MarshalOptions with UseCachedSize allows reusing the result from the
// previous Size call. This is safe here because:
//
// 1. We just computed the size.
// 2. We assume the message is not being mutated concurrently.
//
// Important: If the proto.Size call above is removed, using UseCachedSize
// becomes unsafe and may lead to incorrect marshaling.
//
// For more details, see the doc of UseCachedSize:
// https://pkg.go.dev/google.golang.org/protobuf/proto#MarshalOptions
marshalOptions := proto.MarshalOptions{UseCachedSize: true}

if mem.IsBelowBufferPoolingThreshold(size) {
buf, err := proto.Marshal(vv)
buf, err := marshalOptions.Marshal(vv)
if err != nil {
return nil, err
}
data = append(data, mem.SliceBuffer(buf))
} else {
pool := mem.DefaultBufferPool()
buf := pool.Get(size)
if _, err := (proto.MarshalOptions{}).MarshalAppend((*buf)[:0], vv); err != nil {
if _, err := marshalOptions.MarshalAppend((*buf)[:0], vv); err != nil {
pool.Put(buf)
return nil, err
}
Expand Down