Reduce bytes to strings allocs in decoder#367
Merged
guelfey merged 4 commits intogodbus:masterfrom May 21, 2023
Merged
Conversation
marselester
force-pushed
the
strconv-allocs
branch
from
3418e0c to
f33f35b
Compare
guelfey
reviewed
Apr 24, 2023
Member
guelfey
left a comment
guelfey
left a comment
There was a problem hiding this comment.
Thanks. Generally I'm fine with this, though I wouldn't want to bump the minimum version to Go 1.20 just for this. But using build tags it should be possible to wrap this functionality such that it's only used when using Go 1.20 - just put the actual stringConverter in a file with //+build go1.20 and a stub implementation that just uses a normal string conversion in a file with //+build !go1.20.
marselester
force-pushed
the
strconv-allocs
branch
3 times, most recently
from
ef5578c to
f15ebda
Compare
Contributor
Author
|
That's a good idea to leverage build tags. Maybe I am doing something wrong, but for some reason go1.20 tag clashes with go 1.12 version defined in the go.mod. |
Contributor
Author
|
We can use the old way of converting strings in all the Go versions instead of the build tags. func toString(b []byte) string {
var s string
h := (*reflect.StringHeader)(unsafe.Pointer(&s))
h.Data = uintptr(unsafe.Pointer(&b[0]))
h.Len = len(b)
return s
} |
marselester
force-pushed
the
strconv-allocs
branch
from
646ef96 to
b4ff52c
Compare
Instead of allocating when converting small byte slices to strings in the decoder, we can write to a 4kb buffer and use unsafe.String which refers to that buffer. This approach was successfully tested in https://github.com/marselester/systemd which is used by https://github.com/parca-dev/parca-agent. Note, BenchmarkUnixFDs showed 8 allocs reduction. With larger dbus messages, the savings should be significant.
marselester
force-pushed
the
strconv-allocs
branch
from
b4ff52c to
ab0b98d
Compare
Member
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of allocating when converting small byte slices to strings in the decoder, we can write to a 4kb buffer and use
unsafe.Stringwhich refers to that buffer. This approach was successfully tested in https://github.com/marselester/systemd which is used by https://github.com/parca-dev/parca-agent.Note,
BenchmarkUnixFDsshowed 8 allocs reduction. With larger dbus messages, the savings should be significant.The caveat is that we should bump Go to 1.20, I am not sure if that's something you're comfortable with.