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

Avoid too many copies in string encoder/decoder #33

Merged
merged 9 commits into from
Nov 9, 2022

Conversation

evt
Copy link
Contributor

@evt evt commented Sep 15, 2022

Closes #12

@evt evt self-assigned this Sep 15, 2022
@fasmat fasmat force-pushed the avoid-to-many-copies-in-string-enc-dec branch from 21e8ebe to d1122a4 Compare November 9, 2022 14:01
@fasmat fasmat marked this pull request as ready for review November 9, 2022 14:01
@fasmat fasmat requested a review from dshulyak November 9, 2022 14:48
@fasmat
Copy link
Member

fasmat commented Nov 9, 2022

@dshulyak I got the following results running the benchmarks in unsafe_test.go:

go test -v -benchmem -bench="^Benchmark_" -run="^$" -gcflags=-N .

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-scale
Benchmark_UnsafeBytesToString
Benchmark_UnsafeBytesToString-6         530076894                2.265 ns/op           0 B/op          0 allocs/op
Benchmark_SafeBytesToString
Benchmark_SafeBytesToString-6           417205314                2.864 ns/op           0 B/op          0 allocs/op
Benchmark_UnsafeStringToBytes
Benchmark_UnsafeStringToBytes-6         348893032                3.448 ns/op           0 B/op          0 allocs/op
Benchmark_SafeStringToBytes
Benchmark_SafeStringToBytes-6           419229655                2.861 ns/op           0 B/op          0 allocs/op
PASS
ok      github.com/spacemeshos/go-scale 5.968s

I'm not sure if we should use the unsafe variants avoiding additional allocations, the go runtime will try to avoid allocations anyway if it can. In the case of Bytes2String we are ~ 30% faster, but in the case of String2Bytes we are ~ 20% slower than the safe way to do it.

What is your opinion?

PS.: Results with optimization:

go test -v -benchmem -bench="^Benchmark_" -run="^$" .

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-scale
Benchmark_UnsafeBytesToString
Benchmark_UnsafeBytesToString-6         1000000000               0.3178 ns/op          0 B/op          0 allocs/op
Benchmark_SafeBytesToString
Benchmark_SafeBytesToString-6           518009090                2.318 ns/op           0 B/op          0 allocs/op
Benchmark_UnsafeStringToBytes
Benchmark_UnsafeStringToBytes-6         1000000000               0.4386 ns/op          0 B/op          0 allocs/op
Benchmark_SafeStringToBytes
Benchmark_SafeStringToBytes-6           512238043                2.330 ns/op           0 B/op          0 allocs/op
PASS
ok      github.com/spacemeshos/go-scale 3.713s

I'm not sure how accurate this is, the go compiler/runtime might just completely optimize the benchmark away. Either way according to the Benchmark all versions with and without optimizations trigger 0 allocations per operation.

@fasmat
Copy link
Member

fasmat commented Nov 9, 2022

Using a io.StringWriter when possible improves encoding slightly when using optimizations:

go test -v -benchmem -bench="^BenchmarkEnc" -run="^$" .

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-scale
BenchmarkEncodeStrings_WithStringWriter
BenchmarkEncodeStrings_WithStringWriter-6               39392080                32.31 ns/op           74 B/op          0 allocs/op
BenchmarkEncodeStrings_WithWriterForStrings
BenchmarkEncodeStrings_WithWriterForStrings-6           53234829                51.74 ns/op           40 B/op          0 allocs/op
PASS
ok      github.com/spacemeshos/go-scale 7.676s

without optimization there is almost no difference:

go test -v -benchmem -bench="^BenchmarkEnc" -run="^$" -gcflags=-N .

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-scale
BenchmarkEncodeStrings_WithStringWriter
BenchmarkEncodeStrings_WithStringWriter-6               24001058                56.67 ns/op           62 B/op          0 allocs/op
BenchmarkEncodeStrings_WithWriterForStrings
BenchmarkEncodeStrings_WithWriterForStrings-6           25614274                54.75 ns/op           41 B/op          0 allocs/op
PASS
ok      github.com/spacemeshos/go-scale 6.168s

@fasmat
Copy link
Member

fasmat commented Nov 9, 2022

I updated the benchmarks in a way that hopefully prevents go from optimizing the allocation away. These are the new results:

go test -v -benchmem -bench="^Benchmark_" -run="^$" -gcflags=-N .

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-scale
Benchmark_UnsafeBytesToString
Benchmark_UnsafeBytesToString-6         539234050                2.224 ns/op           0 B/op          0 allocs/op
Benchmark_SafeBytesToString
Benchmark_SafeBytesToString-6           98168997                12.29 ns/op           16 B/op          1 allocs/op
Benchmark_UnsafeStringToBytes
Benchmark_UnsafeStringToBytes-6         350045876                3.431 ns/op           0 B/op          0 allocs/op
Benchmark_SafeStringToBytes
Benchmark_SafeStringToBytes-6           70134423                14.63 ns/op           16 B/op          1 allocs/op
PASS
ok      github.com/spacemeshos/go-scale 6.225s
go test -v -benchmem -bench="^BenchmarkEnc" -run="^$" -gcflags=-N .

goos: linux
goarch: arm64
pkg: github.com/spacemeshos/go-scale
BenchmarkEncodeStrings_WithStringWriter
BenchmarkEncodeStrings_WithStringWriter-6                4509775               312.8 ns/op            69 B/op          0 allocs/op
BenchmarkEncodeStrings_WithWriterForStrings
BenchmarkEncodeStrings_WithWriterForStrings-6            7346796               156.6 ns/op            36 B/op          0 allocs/op
PASS
ok      github.com/spacemeshos/go-scale 4.976s

unsafe.go Outdated Show resolved Hide resolved
encoder_test.go Outdated Show resolved Hide resolved
@fasmat fasmat force-pushed the avoid-to-many-copies-in-string-enc-dec branch from db3a9a9 to d76279f Compare November 9, 2022 16:37
@fasmat fasmat merged commit f2edd70 into master Nov 9, 2022
@fasmat fasmat deleted the avoid-to-many-copies-in-string-enc-dec branch November 9, 2022 16:59
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.

Avoid too many copies in string encoder/decoder
3 participants