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

encoding/binary: add func Append #60023

Closed
lmb opened this issue May 6, 2023 · 15 comments
Closed

encoding/binary: add func Append #60023

lmb opened this issue May 6, 2023 · 15 comments

Comments

@lmb
Copy link
Contributor

lmb commented May 6, 2023

I'd like to propose adding a function with the following signature:

// Append the binary representation of data to buf.
//
// buf may be nil, in which case a new buffer will be allocated. See [Write] on
// which data are acceptable.
//
// Returns the (possibly extended) buffer containing data or an error.
func Append(buf []byte, order AppendByteOrder, data any) ([]byte, error)

This is useful when repeatedly encoding the same kind of value multiple times into a larger buffer and is a natural extension to #50601. A related proposal wants to add similar functions to other packages in encoding: #53693.

Together with #53685 it becomes possible to implement a version of binary.Write that doesn't allocate when using common io.Writer. See my comment for writeBuffer(). Roughly (untested):

func write(w io.Writer, order binary.AppendByteOrder, data any) error {
    buf := writeBuffer(w, binary.Size(data))
    binary.Append(buf[:0], order, data)
    _, err := w.Write(buf)
   return err
}

If the CLs to avoid escaping in reflect APIs lands, Append would allow encoding with zero allocations.

I think it might also allow encoding into stack allocated slices, provided the compiler is (or becomes) smart enough:

buf := binary.Append(make([]byte, 0, 128), binary.LittleEndian, v)
@lmb lmb added the Proposal label May 6, 2023
@gopherbot gopherbot added this to the Proposal milestone May 6, 2023
@ianlancetaylor
Copy link
Contributor

CC @dsnet

@dsnet
Copy link
Member

dsnet commented May 9, 2023

SGTM. Write is the last API in binary that lacks an append-like equivalent.

@lmb
Copy link
Contributor Author

lmb commented May 19, 2023

I've been playing with the implementation a bit, and I'd like to extend my proposal to cover an equivalent function for Read.

// Decode data from buf according to the given byte order.
//
// Returns the number of bytes read from the beginning of buf or io.ErrUnexpectedEOF.
func Decode(buf []byte, order ByteOrder, data any) (int, error)

The rationale is similar to Append: using this new function makes it possible to implement zero-allocation decode for common io.Reader.

I struggled to come up with a good name for the function, what is the inverse of Append? Renaming it to Encode seems clearer to me, and follows existing unexported types. The proposal would become:

func Decode(buf []byte, order ByteOrder, data any) (int, error) // aka Read
func Encode(buf []byte, order AppendByteOrder, data any) ([]byte, error) // aka Write

@rsc rsc changed the title proposal: encoding/binary: add Append to encode values into a provided buffer proposal: encoding/binary: add func Append Apr 4, 2024
@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

We have used Append as the word for all of the other appending encoders, both in this package and others; it is confusing to change to Encode now (for example, why does Encode take an AppendByteOrder and not an EncodeByteOrder? why is it Encode but AppendVarint? and so on). Let's keep using Append.

@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

If we need the decoding version, it could be Parse.

@rsc
Copy link
Contributor

rsc commented Apr 4, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 10, 2024

Looking at unicode/utf8, which has both AppendRune and DecodeRune, it would be fine to have Decode+Append here. But sometimes you are not actually appending, and it seems okay to add Encode too, like utf8 has EncodeRune. But Encode would not be an appender. It would return an error if the buffer was not large enough.

If we did that, we'd have all three:

func Decode(buf []byte, order ByteOrder, data any) (int, error) 
func Encode(buf []byte, order ByteOrder, data any) (int, error)
func Append(buf []byte, order AppendByteOrder, data any) ([]byte, error)

That would match utf8 better.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/579157 mentions this issue: encoding/binary: add Append, Encode and Decode

@lmb
Copy link
Contributor Author

lmb commented Apr 16, 2024

I've dusted off the code I've had lying around and mailed a CL. Encode and Decode were straight forward additions. One interesting wrinkle: I've opted to implement all other primitive functions in terms of Append, which means I need to go from ByteOrder to AppendByteOrder:

func toAppendByteOrder(order ByteOrder) AppendByteOrder {
	switch order := order.(type) {
	case littleEndian:
		return order
	case bigEndian:
		return order
	case nativeEndian:
		return order
	case AppendByteOrder:
		return order
	default:
		return appendableByteOrder{order}
	}
}

toAppendByteOrder can only be implemented this way in encoding/binary because the types are not exported. Doing a type switch vs comparing interface values (which is possible outside of the package) does make a difference in the benchmarks.

P.S.: ToAppendByteOrder is necessary if a dependent package takes ByteOrder as an argument but would like to use Append internally for performance reasons. Without it the choice of encoding primitive „leaks“ into the API and may require breaking changes. I’d like to extend the proposal to include the function (or I can make a separate proposal).

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

Sorry but we're not going to add ToAppendByteOrder. It seems okay to have some duplication of special cases between Encode and Append.

@rsc
Copy link
Contributor

rsc commented Apr 24, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to add:

func Decode(buf []byte, order ByteOrder, data any) (int, error)
func Encode(buf []byte, order ByteOrder, data any) (int, error)
func Append(buf []byte, order AppendByteOrder, data any) ([]byte, error)

@rsc
Copy link
Contributor

rsc commented May 8, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is to add:

func Decode(buf []byte, order ByteOrder, data any) (int, error)
func Encode(buf []byte, order ByteOrder, data any) (int, error)
func Append(buf []byte, order AppendByteOrder, data any) ([]byte, error)

@rsc rsc changed the title proposal: encoding/binary: add func Append encoding/binary: add func Append May 8, 2024
@rsc rsc modified the milestones: Proposal, Backlog May 8, 2024
@aclements
Copy link
Member

aclements commented May 15, 2024

We've been going around and around on the implementation of this and I think I just realized the central issue that's giving us trouble: why does Append take an AppendByteOrder instead of a ByteOrder? In the general case, it already pre-computes the size of the entire encoded value and grows the slice by that much. At that point, there's no need for the Append* methods of AppendByteOrder. In the current CL, it uses the Append methods for the fast path, but I'm not sure there's much value in that over just growing the slice.

@rsc
Copy link
Contributor

rsc commented May 15, 2024

Dropping AppendByteOrder works for me.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587096 mentions this issue: encoding/binary: adjust docs for Append, Encode and Decode

gopherbot pushed a commit that referenced this issue May 22, 2024
Updates #60023

Change-Id: Ida1cc6c4f5537402e11db6b8c411828f2bcc0a5e
Reviewed-on: https://go-review.googlesource.com/c/go/+/587096
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

7 participants