-
Notifications
You must be signed in to change notification settings - Fork 275
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
perf: use fast unsafe bytes->string convertion #525
Changes from all commits
922a658
e334dd3
3a5d7f0
588fa2f
b91f8b6
5c36703
75e7273
3203669
b519af0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package common | ||
|
||
import ( | ||
"reflect" | ||
"unsafe" | ||
) | ||
|
||
// UnsafeStrToBytes uses unsafe to convert string into byte array. Returned bytes | ||
// must not be altered after this function is called as it will cause a segmentation fault. | ||
func UnsafeStrToBytes(s string) []byte { | ||
var buf []byte | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about the following to avoid copying the slice internals by directly casting the string:
I'm not sure if there are any constraints preventing that but this could be a further optimization There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the maximum process address space value I grabbed this suggestion from here: Original source: https://groups.google.com/g/golang-nuts/c/Zsfk-VMd_fU/m/O1ru4fO-BgAJ |
||
sHdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) | ||
bufHdr := (*reflect.SliceHeader)(unsafe.Pointer(&buf)) | ||
bufHdr.Data = sHdr.Data | ||
bufHdr.Cap = sHdr.Len | ||
bufHdr.Len = sHdr.Len | ||
return buf | ||
} | ||
|
||
// UnsafeBytesToStr is meant to make a zero allocation conversion | ||
// from []byte -> string to speed up operations, it is not meant | ||
// to be used generally, but for a specific pattern to delete keys | ||
// from a map. | ||
func UnsafeBytesToStr(b []byte) string { | ||
return *(*string)(unsafe.Pointer(&b)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
package common | ||
|
||
import ( | ||
"runtime" | ||
"strconv" | ||
"testing" | ||
"time" | ||
|
||
"github.com/stretchr/testify/suite" | ||
) | ||
|
||
func TestStringSuite(t *testing.T) { | ||
suite.Run(t, new(StringSuite)) | ||
} | ||
|
||
type StringSuite struct{ suite.Suite } | ||
|
||
func unsafeConvertStr() []byte { | ||
return UnsafeStrToBytes("abc") | ||
} | ||
|
||
func (s *StringSuite) TestUnsafeStrToBytes() { | ||
// we convert in other function to trigger GC. We want to check that | ||
// the underlying array in []bytes is accessible after GC will finish swapping. | ||
for i := 0; i < 5; i++ { | ||
b := unsafeConvertStr() | ||
runtime.GC() | ||
<-time.NewTimer(2 * time.Millisecond).C | ||
b2 := append(b, 'd') | ||
s.Equal("abc", string(b)) | ||
s.Equal("abcd", string(b2)) | ||
} | ||
} | ||
|
||
func unsafeConvertBytes() string { | ||
return UnsafeBytesToStr([]byte("abc")) | ||
} | ||
|
||
func (s *StringSuite) TestUnsafeBytesToStr() { | ||
// we convert in other function to trigger GC. We want to check that | ||
// the underlying array in []bytes is accessible after GC will finish swapping. | ||
for i := 0; i < 5; i++ { | ||
str := unsafeConvertBytes() | ||
runtime.GC() | ||
<-time.NewTimer(2 * time.Millisecond).C | ||
s.Equal("abc", str) | ||
} | ||
} | ||
|
||
func BenchmarkUnsafeStrToBytes(b *testing.B) { | ||
for i := 0; i < b.N; i++ { | ||
UnsafeStrToBytes(strconv.Itoa(i)) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package iavl | ||
|
||
import ibytes "github.com/cosmos/iavl/internal/bytes" | ||
|
||
var ( | ||
unsafeToStr = ibytes.UnsafeBytesToStr | ||
unsafeToBz = ibytes.UnsafeStrToBytes | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that this is helpful because, from my understanding, copying during
[]byte
tostring
conversion when accessing a map by key should be optimized by the Go compiler.Sources:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if the benchmark would remain the same as it is right now if the
map[string]
changes are revertedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check, however for consistency I prefer to keep the casting here.