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: using Write to write a []byte causes unnecessary allocation #27757

Open
dominikh opened this Issue Sep 19, 2018 · 7 comments

Comments

Projects
None yet
8 participants
@dominikh
Member

dominikh commented Sep 19, 2018

What version of Go are you using (go version)?

go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

go test -benchmem -bench=. https://play.golang.org/p/mkwTIJMlyJE

What did you expect to see?

I expect binary.Write to write the input []byte directly to the writer, without causing an allocation the size of the input

What did you see instead?

goos: linux
goarch: amd64
BenchmarkWrite-8   	    3000	    351964 ns/op	 2007084 B/op	       3 allocs/op
PASS
ok  	_/tmp	1.122s

Write has a fast path for simple types that it supports directly. At the beginning of the fast path, a buffer the size of the input value is allocated (1 byte for uint8, 2 bytes for uint16, ..., n bytes for []byte). In the specific case of []byte, however, this buffer is unnecessary. We don't even copy the data into it, we just set the variable to the input.

There is no direct need for using binary.Write with a known input of []byte since endianness doesn't matter and one could just say w.Write(b) – however. inputs may be dynamic, and it should probably be binary.Write that handles []byte specially, not every caller. A lot of people also use binary.Write on []byte for the sake of consistency with surrounding code.

/cc @griesemer

@dominikh dominikh added this to the Go1.12 milestone Sep 19, 2018

@bcmills bcmills added the Performance label Sep 19, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Sep 19, 2018

If binary.Write took a ...interface{} then I could see adding a []byte special case. I'm really confused about why you care about a []byte special case for a single parameter though. Just call w.Write(data) instead of binary.Write(w, binary.BigEndian, data). It will not only allocate less, it will run faster since it doesn't have to convert the []byte to an interface{} and then type switch to find it back. I'm very skeptical this needs to be optimized.

But feel free to send a CL if it's important to you.

@agnivade

This comment has been minimized.

Member

agnivade commented Sep 20, 2018

After 2179e49, this has improved slightly.

goos: linux
goarch: amd64
pkg: stdtest
BenchmarkWriteBin-4   	   10000	    234153 ns/op	 2007072 B/op	       2 allocs/op
PASS
@josharian

This comment has been minimized.

Contributor

josharian commented Oct 11, 2018

IIUC, the request is to do this TODO:

2179e49#diff-e41aaf059e0604e922505a771fd5d190R292

This is a relatively easy first contribution.

@josharian josharian added the Suggested label Oct 11, 2018

@gopherbot

This comment has been minimized.

gopherbot commented Oct 17, 2018

Change https://golang.org/cl/142938 mentions this issue: encoding/binary: avoid allocation when writing slice of uint8

@fmstephe

This comment has been minimized.

fmstephe commented Oct 23, 2018

I would like to work on this issue.

If this is the right way of doing it. I have a PR which is almost ready. Should I wait for a reply here before I submit that?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 23, 2018

@fmstephe You can send the change without waiting for a reply. Thanks.

But in the comment stream I see that 6 days ago https://golang.org/cl/142938 was sent, so you should look at that change first and perhaps comment on that one if yours is better.

@fmstephe

This comment has been minimized.

fmstephe commented Oct 23, 2018

I must be blind :) It's right there above my comment.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment