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

Reduce bytes to strings allocs in decoder #367

Merged
merged 4 commits into from
May 21, 2023

Conversation

marselester
Copy link
Contributor

@marselester marselester commented Apr 23, 2023

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.

$ go test -bench='^BenchmarkUnixFDs$' -benchmem
# new
BenchmarkUnixFDs-2   	    8052	    241806 ns/op	   11181 B/op	     357 allocs/op
# old
BenchmarkUnixFDs-2   	    8899	    265314 ns/op	   11187 B/op	     365 allocs/op

The caveat is that we should bump Go to 1.20, I am not sure if that's something you're comfortable with.

Copy link
Member

@guelfey guelfey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marselester force-pushed the strconv-allocs branch 3 times, most recently from ef5578c to f15ebda Compare April 24, 2023 19:57
@marselester
Copy link
Contributor Author

marselester commented Apr 24, 2023

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.

$ go test .
# github.com/godbus/dbus/v5 [github.com/godbus/dbus/v5.test]
./btos_new.go:10:9: unsafe.String requires go1.20 or later (-lang was set to go1.12; check go.mod)
FAIL	github.com/godbus/dbus/v5 [build failed]
FAIL

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

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.
@guelfey
Copy link
Member

guelfey commented May 21, 2023

Thanks!

@guelfey guelfey merged commit ba32b10 into godbus:master May 21, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants