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

Jump to bottom

os: make File.WriteString not allocate #42406

Closed
josharian opened this issue Nov 5, 2020 · 5 comments
Closed

os: make File.WriteString not allocate #42406

josharian opened this issue Nov 5, 2020 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Go1.17

Comments

@josharian
Copy link
Contributor

josharian commented Nov 5, 2020 •
edited

This is os.File.WriteString's implementation:

// WriteString is like Write, but writes the contents of string s rather than
// a slice of bytes.
func (f *File) WriteString(s string) (n int, err error) {
	return f.Write([]byte(s))
}

This currently accounts for 10% 35% of allocated space in a service I'm maintaining.

We should be able to do an unsafe cast of s to b here, and avoid the allocation.

Do any kernels modify the bytes during Write? Any other reason why we wouldn't be able to do this?

cc @bradfitz
@josharian josharian added this to the Go1.17 milestone Nov 5, 2020
@josharian
Copy link
Contributor Author

Problem: os can't import reflect, at least according to existing dependency rules. And I don't think #19367 will help here.

There are options: add go:linkname with package runtime support, allow os to import reflect, add StringHeader/SliceHeader to package reflectlite and teach the compiler about that. I imagine the first is the most palatable.

@odeke-em
Copy link
Member

odeke-em commented Nov 6, 2020

@josharian you are Josh and can make it work perhaps in the compiler by special recognition, if the symbol is a (*os.File).WriteString and its on an architecture that we've audited as won't be mutating bytes, rewrite it :)

@ianlancetaylor
Copy link
Contributor

@josharian Permit me to introduce you to the exciting new internal/unsafeheader package.

@josharian
Copy link
Contributor Author

Oooooooooooooooooh!

@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 6, 2020
@gopherbot
Copy link

Change https://golang.org/cl/268020 mentions this issue: os: avoid allocation in File.WriteString

josharian added a commit to tailscale/go that referenced this issue Nov 19, 2020
@josharian
os: avoid allocation in File.WriteString
d625d55 
Instead of alloc+copy to convert the string
to a byte slice, do an unsafe conversion.

Rely on the kernel not to scribble on the
buffer during the write.

Fixes golang#42406

Change-Id: I66f4838b43a11bcc3d67bbfa1706726318d55343
josharian added a commit to tailscale/go that referenced this issue Nov 19, 2020
@josharian
os: avoid allocation in File.WriteString
ba05fe1 
[out for review upstream: https://go-review.googlesource.com/c/go/+/268020]

Instead of alloc+copy to convert the string
to a byte slice, do an unsafe conversion.

Rely on the kernel not to scribble on the
buffer during the write.

Fixes golang#42406

Change-Id: I66f4838b43a11bcc3d67bbfa1706726318d55343
josharian added a commit to tailscale/go that referenced this issue Nov 19, 2020
@josharian
os: avoid allocation in File.WriteString
3bd8919 
[out for review upstream: https://go-review.googlesource.com/c/go/+/268020]

Instead of alloc+copy to convert the string
to a byte slice, do an unsafe conversion.

Rely on the kernel not to scribble on the
buffer during the write.

Fixes golang#42406

Change-Id: I66f4838b43a11bcc3d67bbfa1706726318d55343
bradfitz pushed a commit to tailscale/go that referenced this issue Feb 18, 2021
@josharian @bradfitz
[tailscale1.16] os: avoid allocation in File.WriteString
9e042b1 
[out for review upstream: https://go-review.googlesource.com/c/go/+/268020]

Instead of alloc+copy to convert the string
to a byte slice, do an unsafe conversion.

Rely on the kernel not to scribble on the
buffer during the write.

Fixes golang#42406

Change-Id: I66f4838b43a11bcc3d67bbfa1706726318d55343
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 18, 2021
@adam-azarchs adam-azarchs mentioned this issue Jun 10, 2021
Closed
@golang golang locked and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
6 participants
@toothrot @josharian @dmitshur @ianlancetaylor @odeke-em @gopherbot