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

encoding/binary: using Write to write a []byte causes unnecessary allocation #27757

Closed
dominikh opened this issue Sep 19, 2018 · 9 comments
Closed

encoding/binary: using Write to write a []byte causes unnecessary allocation #27757

dominikh opened this issue Sep 19, 2018 · 9 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Unplanned

Comments

@dominikh
Copy link
Member

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 the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 19, 2018
@dominikh dominikh added this to the Go1.12 milestone Sep 19, 2018
@rsc
Copy link
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.

@rsc rsc added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Sep 19, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 19, 2018
@agnivade
Copy link
Contributor

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
Copy link
Contributor

IIUC, the request is to do this TODO:

2179e49#diff-e41aaf059e0604e922505a771fd5d190R292

This is a relatively easy first contribution.

@josharian josharian added the Suggested Issues that may be good for new contributors looking for work to do. label Oct 11, 2018
@gopherbot
Copy link

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

@fmstephe
Copy link

fmstephe commented Oct 23, 2018 •
edited

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
Copy link
Contributor

ianlancetaylor commented Oct 23, 2018 •
edited

@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
Copy link

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

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Unplanned Dec 11, 2018
@agnivade
Copy link
Contributor

@dominikh - The CL sent to fix this was closed because folks didn't like the new make calls. Do you still want to keep this open and look for other ways to fix this ?

@dominikh
Copy link
Member Author

There's not a lot of enthusiasm for fixing this, and maybe it's easier for callers not to call binary.Write after all... I'll close the issue; we can reopen it if more people complain about this.

@golang golang locked and limited conversation to collaborators Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
8 participants
@dominikh @josharian @rsc @fmstephe @agnivade @ianlancetaylor @bcmills @gopherbot