-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
bytes: add Buffer.Available and Buffer.AvailableBuffer #53685
Comments
I agree that we need a better way to have In #47527, we added // AvailableBuffer returns an empty buffer with non-zero capacity.
// This buffer is intended to be appended to and
// passed to an immediately succeeding Write call.
func (b *Buffer) AvailableBuffer() []byte {
if cap(b.buf) - len(b.buf) {
b.grow(1)
}
return b.buf[len(b.buf):]
} Thus, usage would look like: var bb *bytes.Buffer = ...
b := bb.AvailableBuffer()
b = binary.AppendUvarint(b, ...)
bb.Write(b) Note that this operation is allocation-free and copy-free in the case where the Aside: If we want to be consistent with func (b *Buffer) Available() int {
return cap(b.buf) - len(b.buf)
} † Also, most
If other |
I've often wished for a way to get at the underlying buffer so I support this proposal. It seems like this has fallen through the cracks maybe, since there has been no update since July 6th? @dsnet could you explain why you'd have AvailableBuffer guarantee non-zero capacity? Seems like that is different from |
I can't remember why it guarantees non-zero capacity. It isn't strictly necessary. |
This proposal is in the list of incoming proposals, of which there are many. It hasn't fallen through the cracks. |
I remember now. If the usage pattern is: var bb *bytes.Buffer = ...
b := bb.AvailableBuffer()
b = binary.AppendUvarint(b, ...)
bb.Write(b) we would want the allocation to occur within It's a small optimization to reduce the amount of allocations when an empty |
Ian: I didn't mean this as a criticism, sorry if it came across that way. It's not clear from the outside whether a proposal has been evaluated to go into active or not. To add something to the discussion, I've been playing around with func readBuffer(r io.Reader, n int) ([]byte, error) {
switch r := r.(type) {
case *bufio.Reader:
buf, err := r.Peek(n)
if errors.Is(err, bufio.ErrBufferFull) {
// We've requested a buffer than is larger than the reader. Fall
// back to allocating a new slice.
break
}
if err != nil {
return nil, err
}
// Advance the internal state of r by reading into the already peeked
// buffer. There is no additional copy, since dst == src.
_, err = r.Read(buf)
return buf, err
case *bytes.Buffer:
buf := r.Next(n)
if len(buf) == 0 {
return nil, io.EOF
} else if len(buf) < n {
return nil, io.ErrUnexpectedEOF
}
return buf, nil
}
buf := make([]byte, n)
_, err := io.ReadFull(r, buf)
return buf, err
}
func writeBuffer(w io.Writer, n int) []byte {
switch w := w.(type) {
case *bufio.Writer:
if w.Available() < n {
break
}
return w.AvailableBuffer()
// XXX: Need bytes.Buffer.AvailableBuffer here.
}
return make([]byte, 0, n)
} With these two functions it's possible to wrap append-style encoding APIs into functions that take Some more observations:
P.S.: Should we split |
@lmb:
|
@lmb I didn't take it as criticism, I was just explaining the current status. |
Something like this (if and when bytes.Buffer supports AvailableBuffer())?
|
Change https://go.dev/cl/440135 mentions this issue: |
Given that we added https://pkg.go.dev/bufio#Writer.AvailableBuffer in Go 1.18, it sounds like we should use the same pattern in bytes.Buffer to allow the same idioms? |
This proposal has been added to the active column of the proposals project |
It sounds like people are happy with |
I have a use for Something like: // Always ensure at least 25% capacity remains before the next batch of append operations.
if b.Available() < b.Len()/4 {
b.Grow(b.Available()+1)
} That said, this is no different than: if cap(b.AvailableBuffer()) < b.Len()/4 {
b.Grow(cap(b.AvailableBuffer())+1)
} but isn't as readable. |
Change https://go.dev/cl/469558 mentions this issue: |
Have all concerns about this proposal been addressed? |
This is more of an aside and not a specific concern: I find it surprising that bytes.Buffer has been fine without this method for so long (and it seems to me like a frequently used struct) and so I’m curious if people have an opinion about why this is necessary now/what changed. |
This feature was technically always possible where: b := bb.AvailableBuffer() is semantically identical to: b := bb.Bytes()[bb.Len():] but doing so depended upon undocumented guarantees of Years ago, I tried modifying the documentation on |
Pure anecdata / speculation, I think that people who found performance with Buffer to be lacking switched to |
As part of the effort to rely less on bytes.Buffer, switch most operations to use more natural append-like operations. This makes it easier to swap bytes.Buffer out with a buffer type that only needs to support a minimal subset of operations. As a simplification, we can remove the use of the scratch buffer and use the available capacity of the buffer itself as the scratch. Also, declare an inlineable mayAppendQuote function to conditionally append a double-quote if necessary. Performance: name old time/op new time/op delta CodeEncoder 405µs ± 2% 397µs ± 2% -1.94% (p=0.000 n=20+20) CodeEncoderError 453µs ± 1% 444µs ± 4% -1.83% (p=0.000 n=19+19) CodeMarshal 559µs ± 4% 548µs ± 2% -2.02% (p=0.001 n=19+17) CodeMarshalError 724µs ± 3% 716µs ± 2% -1.13% (p=0.030 n=19+20) EncodeMarshaler 24.9ns ±15% 22.9ns ± 5% ~ (p=0.086 n=20+17) EncoderEncode 14.0ns ±27% 15.0ns ±20% ~ (p=0.365 n=20+20) There is a slight performance gain across the board due to the elimination of the scratch buffer. Appends are done directly into the unused capacity of the underlying buffer, avoiding an additional copy. See #53685 Updates #27735 Change-Id: Icf6d612a7f7a51ecd10097af092762dd1225d49e Reviewed-on: https://go-review.googlesource.com/c/go/+/469558 Reviewed-by: Daniel Martí <[email protected]> Auto-Submit: Joseph Tsai <[email protected]> Reviewed-by: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Joseph Tsai <[email protected]>
Based on the discussion above, this proposal seems like a likely accept. |
No change in consensus, so accepted. 🎉 |
Change https://go.dev/cl/474635 mentions this issue: |
Change https://go.dev/cl/499617 mentions this issue: |
For #53685 Change-Id: I237297d19afeb36ad738074d0c61caa7012f65ac Reviewed-on: https://go-review.googlesource.com/c/go/+/499617 Reviewed-by: Eli Bendersky <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> TryBot-Bypass: Ian Lance Taylor <[email protected]>
Hi!
I propose to introduce a new method to
bytes.Buffer
:Rationale.
Grow
combined withWrite
is nice, but some APIs, binary.PutUvarint for instance, works with slices of bytes. This means memory copying will be needed in order to write uvarint-encoded data. Lots of memory copies in my case where I have manual serialization for some data structures.I guess this method would be handy in cases like mine, where I would:
The text was updated successfully, but these errors were encountered: