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.

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

Jump to bottom

proposal: bytes: add (*bytes.Buffer).SetBytes method #67004

Closed
ajwerner opened this issue Apr 23, 2024 · 11 comments
Closed

proposal: bytes: add (*bytes.Buffer).SetBytes method #67004

ajwerner opened this issue Apr 23, 2024 · 11 comments
Labels
Proposal
Milestone
Proposal

Comments

@ajwerner
Copy link

Proposal Details

Motivation

It should be possible to reset the byte slice underneath a bytes.Buffer without allocating a new object. Today the only method to initialize a bytes.Buffer with an explicit byte slice is bytes.NewBuffer.

The io.Reader interface is widely used and makes decoding/consuming byte streams uniform. In some cases, it is valuable to be able to optimize the special case of *bytes.Buffer as opposed to requiring a larger API surface area (see #63397 (comment)). Processing bytes can at times be the hottest path of a go program. Having a simple method to reset the *bytes.Buffer object with a new byte slice can enable these uniform APIs to be utilized in high performance contexts.

Proposal

Introduce a new method (*bytes.Buffer).SetBytes as defined below:

// SetBytes reinitializes a Buffer using buf as its contents. The Buffer takes
// ownership of buf, and the caller should not use buf after this call. SetBytes
// is intended to prepare a Buffer to read existing data. It can also be used to
// reset the size of the internal buffer for writing. To do that, buf should
// have the desired capacity but a length of zero.
func (b *Buffer) SetBytes(buf []byte) {
    *b = Buffer{buf: buf}
}

This method has symmetry with (*bytes.Buffer).Bytes() which provides access to the underlying unread portion of the buffer.
@gopherbot gopherbot added this to the Proposal milestone Apr 23, 2024
@seankhliao
Copy link
Member

Why not just reset? https://pkg.go.dev/bytes#Buffer.Reset

@randall77
Copy link
Contributor

*b = *bytes.NewBuffer(buf) works. The NewBuffer call is inlined and the allocation inside it happens on the stack.
(Copy a struct of another package wholesale is generally frowned upon, but it works in this case.)

@ajwerner
Copy link
Author

Why not just reset? https://pkg.go.dev/bytes#Buffer.Reset

Reset truncates the existing buffer. The use case here is that I have a byte slice already and I have a bytes.Buffer object and I'd like to create an io.Reader to read those bytes without allocating.

A legitimate claim is that the bytes.Reader API should work, because it indeed allows for resetting with a new slice. Where that turns out to not work is that bytes.Reader doesn't expose the underlying byte slice (it doesn't have a Bytes method).

@ajwerner
Copy link
Author

*b = *bytes.NewBuffer(buf) works. The NewBuffer call is inlined and the allocation inside it happens on the stack.

(Copy a struct of another package wholesale is generally frowned upon, but it works in this case.)

Cool! I'll close this and hope that some future wayward soul may discover this trick.

@dsnet
Copy link
Member

dsnet commented Apr 23, 2024

*b = *bytes.NewBuffer(buf) just blew my mind... I've been programming Go for 10 years and that didn't occur to me.

@dsnet
Copy link
Member

dsnet commented Apr 23, 2024

I still wonder if SetBytes has value as an explicit API?

As prior precedence, we added the AvailableBuffer method to make it more obvious how to correctly append into a bytes.Buffer even though this functionally had always been possible by messing around with the capacity of the buffer returned by the Bytes method.

@ianlancetaylor
Copy link
Contributor

Anybody want to add a test to make sure it keeps working? It's not like it's documented.

@dsnet
Copy link
Member

dsnet commented Apr 23, 2024

I can add a test. I'm about to use this trick.

@gopherbot
Copy link

Change https://go.dev/cl/581297 mentions this issue: bytes: add test to ensure shallow copy of NewBuffer does not allocate

dsnet added a commit to tailscale/tailscale that referenced this issue Apr 23, 2024
@dsnet
logtail: avoid bytes.Buffer allocation
fe73052 
Re-use a pre-allocated bytes.Buffer struct and
shallow the copy the result of bytes.NewBuffer into it
to avoid allocating the struct.

Note that we're only reusing the bytes.Buffer struct itself
and not the underling []byte temporarily stored within it.

Updates #cleanup
Updates tailscale/corp#18514
Updates golang/go#67004

Signed-off-by: Joe Tsai <joetsai@digital-static.net>
@dsnet dsnet mentioned this issue Apr 23, 2024
Open
gopherbot pushed a commit that referenced this issue Apr 24, 2024
@dsnet @gopherbot
bytes: add test to ensure shallow copy of NewBuffer does not allocate
62dfa43 
At present, there is no API to reset the underlying []byte
of an existing Buffer struct, except to shallow copy
the entire Buffer struct.

Updates #67004

Change-Id: I08998f7a95ae5bde0897d86247d47f23cd784583
Reviewed-on: https://go-review.googlesource.com/c/go/+/581297
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Joedian Reid <joedian@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Keith Randall <khr@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@odeke-em
Copy link
Member

@dsnet kindly please see @thanm's notice on the test CL https://go-review.googlesource.com/c/go/+/581297/2#message-b5102998fb0dc108e0ad11e3d249c9017581ab89 about the test failure https://ci.chromium.org/ui/p/golang/builders/ci/gotip-linux-amd64-noopt/b8749654087388264177/test-results on the gotip-linux-amd64-noopt-test_only builder.

@dsnet
Copy link
Member

dsnet commented Apr 30, 2024

I was about to fix this and it appear it's already been fixed: https://go-review.googlesource.com/c/go/+/581915

My apologies for the failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
7 participants
@ajwerner @ianlancetaylor @odeke-em @dsnet @randall77 @gopherbot @seankhliao