search

golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.64k stars 1.58k forks source link

protodelim.UnmarshalOptions should support specifying a reusable buffer #1540

Closed vanja-pejovic closed 1 year ago

vanja-pejovic commented 1 year ago

Is your feature request related to a problem? Please describe. protodelim.UnmarshalOptions.UnmarshalFrom creates a new buffer for every message that it parses. When unmarshalling many messages, this allocates a bunch of memory and costs CPU time.

Describe the solution you'd like Allow the user to specify a reusable buffer in UnmarshalOptions. Specifically, add a Buffer field to UnmarshalOptions. If it is non-nil, attempt to use that buffer instead of creating a new one. If it's too small, allocate a bigger buffer and place it into Buffer.

Describe alternatives you've considered protodelim.UnmarshalOptions could have an un-exported buffer field, which it always tried to reuse. This would have similar performance benefits, but it wouldn't allow the user to presize the buffer to handle their data size.

UnmarshalOptions.UnmarshalFrom could reuse the buffer even when the user didn't specify one. This would be a regression for users who care more about peak memory than CPU usage and have many live UnmarshalOptions.

Additional context I have code which currently handrolls a version of protodelim with a reusable buffer. I would like to simplify our code by switching to using protodelim, but as is, our benchmarks are 5% slower because of allocating a new buffer for every message. Implementing the changes I suggested above makes protodelim perform as well as our existing code.

vanja-pejovic commented 1 year ago

FYI: I have this implemented, and will send it for review soon.

vanja-pejovic commented 1 year ago

I've created https://go-review.git.corp.google.com/c/protobuf/+/491596, but given that this is my first time using Git and Gerrit, I'm not sure if I've done everything right.

vanja-pejovic commented 1 year ago

This was fixed by https://go-review.git.corp.google.com/c/protobuf/+/491596. However, instead of allowing the user to directly specify a buffer, if they pass in a bufio.Reader, we try to use the buffer from the reader.