-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
cmd/compile: read-only escape analysis and avoiding string -> []byte copies #2205
Comments
Comment 1 by jp@webmaster.ms: It seem to be much easier to add the possibility of zerocopish taking string slices from byte slices/arrays. As far I understand, string slices are actually readonly and cap()'less byte slices: http://research.swtch.com/2009/11/go-data-structures.html |
Owner changed to @rsc. Status changed to Accepted. |
I run into this often, but I should start listing examples. In goprotobuf, text.go calls writeString with both a string and a []byte converted to a string: // writeAny writes an arbitrary field. func writeAny(w *textWriter, v reflect.Value, props *Properties) { v = reflect.Indirect(v) // We don't attempt to serialise every possible value type; only those // that can occur in protocol buffers, plus a few extra that were easy. switch v.Kind() { case reflect.Slice: // Should only be a []byte; repeated fields are handled in writeStruct. writeString(w, string(v.Interface().([]byte))) case reflect.String: writeString(w, v.String()) Note that the function writeString reallly just wants a read-only slice of bytes: func writeString(w *textWriter, s string) { w.WriteByte('"') // Loop over the bytes, not the runes. for i := 0; i < len(s); i++ { // Divergence from C++: we don't escape apostrophes. // There's no need to escape them, and the C++ parser // copes with a naked apostrophe. switch c := s[i]; c { case '\n': w.Write([]byte{'\\', 'n'}) case '\r': w.Write([]byte{'\\', 'r'}) case '\t': w.Write([]byte{'\\', 't'}) case '"': w.Write([]byte{'\\', '"'}) case '\\': w.Write([]byte{'\\', '\\'}) default: if isprint(c) { w.WriteByte(c) } else { fmt.Fprintf(w, "\\%03o", c) } } } w.WriteByte('"') } It doesn't matter that it's frozen (like a string), nor writable (like a []byte). But Go lacks that type, so if instead it'd be nice to write writeAny with a []byte parameter and invert the switch above to be like: switch v.Kind() { case reflect.Slice: // Should only be a []byte; repeated fields are handled in writeStruct. writeString(w, v.Interface().([]byte)) case reflect.String: writeString(w, []byte(v.String())) // no copy! Where the []byte(v.String()) just makes a slice header pointing in to the string's memory, since the compiler can verify that writeAny never mutates its slice. |
See patch and CL description at http://golang.org/cl/6850067 for the opposite but very related case: strconv.ParseUint, ParseBool, etc take a string but calling code has a []byte. |
People who like this bug also like issue #3512 (cmd/gc: optimized map[string] lookup from []byte key) |
FWIW var m map[string]int var b []byte _ = m[string(b)] case must be radically simpler to implement than general read-only analysis. It's peephole optimization, when the compiler sees such code it can generate hacky string object using the []byte pointer. Another example would be len(string(b)), but this seems useless. |
Yes. That's why they're separate bugs. I want issue #3512 first, because it's easy. |
IMO, I think that it's difficult to provide way for converting types since that bytes might be on stack not heap allocator. var s string
// s = "hello" // This should be crash because "hello" might be on stack.
s = string([]byte("hello"))
header := (*reflect.StringHeader)(unsafe.Pointer(&s))
bytesheader := &reflect.SliceHeader{
Data: header.Data,
Len: header.Len,
Cap: header.Len,
}
b := *(*[]byte)(unsafe.Pointer(bytesheader))
b[0] = 0x48 |
Simpler example (tested with Go 1.17.8 / 1.18): package pkg
// ToLower converts ascii string s to lower-case
func ToLower(s string) string {
if s == "" {
return ""
}
buf := make([]byte, len(s))
for i := 0; i < len(s); i++ {
c := s[i]
if 'A' <= c && c <= 'Z' {
c |= 32
}
buf[i] = c
}
return string(buf)
} pkg_test.go package pkg
import "testing"
func BenchmarkToLower(b *testing.B) {
str := "SomE StrInG"
want := "some string"
var res string
for i := 0; i < b.N; i++ {
res = ToLower(str)
}
if res != want {
b.Fatal("ToLower error")
}
}
There is a redundant allocation & copy in |
Change https://go.dev/cl/520259 mentions this issue: |
Change https://go.dev/cl/520395 mentions this issue: |
This CL extends escape analysis in two ways. First, we already optimize directly called closures. For example, given: var x int // already stack allocated today p := func() *int { return &x }() we don't need to move x to the heap, because we can statically track where &x flows. This CL extends the same idea to work for indirectly called closures too, as long as we know everywhere that they're called. For example: var x int // stack allocated after this CL f := func() *int { return &x } p := f() This will allow a subsequent CL to move the generation of go/defer wrappers earlier. Second, this CL adds tracking to detect when pointer values flow to the pointee operand of an indirect assignment statement (i.e., flows to p in "*p = x") or to builtins that modify memory (append, copy, clear). This isn't utilized in the current CL, but a subsequent CL will make use of it to better optimize string->[]byte conversions. Updates #2205. Change-Id: I610f9c531e135129c947684833e288ce64406f35 Reviewed-on: https://go-review.googlesource.com/c/go/+/520259 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
CL 520395 that resolved this issue was reverted in CL 520596, so reopening. |
Change https://go.dev/cl/520599 mentions this issue: |
Change https://go.dev/cl/520600 mentions this issue: |
Hi, |
@jfcg No, at this time only string->[]byte. |
This CL implements the remainder of the zero-copy string->[]byte conversion optimization initially attempted in go.dev/cl/520395, but fixes the tracking of mutations due to ODEREF/ODOTPTR assignments, and adds more comprehensive tests that I should have included originally. However, this CL also keeps it behind the -d=zerocopy flag. The next CL will enable it by default (for easier rollback). Updates #2205. Change-Id: Ic330260099ead27fc00e2680a59c6ff23cb63c2b Reviewed-on: https://go-review.googlesource.com/c/go/+/520599 Auto-Submit: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Than McIntosh <thanm@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
BTW, should the old optimization for |
Change https://go.dev/cl/584118 mentions this issue: |
Now as []byte(string) doesn't always cause heap allocation (CL 520599, #2205) we can make DecodeString simpler and more performant, by not allocating x2 the required memory. goos: linux goarch: amd64 pkg: encoding/hex cpu: AMD Ryzen 5 4600G with Radeon Graphics │ beforehex │ afterhex │ │ sec/op │ sec/op vs base │ DecodeString/256-12 197.9n ± 1% 172.2n ± 1% -13.01% (p=0.000 n=10) DecodeString/1024-12 684.9n ± 1% 598.5n ± 1% -12.61% (p=0.000 n=10) DecodeString/4096-12 2.764µ ± 0% 2.343µ ± 1% -15.23% (p=0.000 n=10) DecodeString/16384-12 10.774µ ± 1% 9.348µ ± 1% -13.23% (p=0.000 n=10) geomean 1.417µ 1.226µ -13.53% │ beforehex │ afterhex │ │ B/s │ B/s vs base │ DecodeString/256-12 1.205Gi ± 1% 1.385Gi ± 1% +14.94% (p=0.000 n=10) DecodeString/1024-12 1.393Gi ± 1% 1.593Gi ± 1% +14.42% (p=0.000 n=10) DecodeString/4096-12 1.380Gi ± 0% 1.628Gi ± 1% +17.97% (p=0.000 n=10) DecodeString/16384-12 1.416Gi ± 1% 1.632Gi ± 1% +15.25% (p=0.000 n=10) geomean 1.346Gi 1.556Gi +15.64% │ beforehex │ afterhex │ │ B/op │ B/op vs base │ DecodeString/256-12 256.0 ± 0% 128.0 ± 0% -50.00% (p=0.000 n=10) DecodeString/1024-12 1024.0 ± 0% 512.0 ± 0% -50.00% (p=0.000 n=10) DecodeString/4096-12 4.000Ki ± 0% 2.000Ki ± 0% -50.00% (p=0.000 n=10) DecodeString/16384-12 16.000Ki ± 0% 8.000Ki ± 0% -50.00% (p=0.000 n=10) geomean 2.000Ki 1.000Ki -50.00% │ beforehex │ afterhex │ │ allocs/op │ allocs/op vs base │ DecodeString/256-12 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ DecodeString/1024-12 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ DecodeString/4096-12 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ DecodeString/16384-12 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ geomean 1.000 1.000 +0.00% Change-Id: I5676e48f222d90786ea18e808cb4ecde9de82597 GitHub-Last-Rev: aeedf3f GitHub-Pull-Request: #67259 Reviewed-on: https://go-review.googlesource.com/c/go/+/584118 Auto-Submit: Ian Lance Taylor <iant@google.com> Reviewed-by: Cherry Mui <cherryyz@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Ian Lance Taylor <iant@google.com>
So now we can take string element addresses without package main
func main() {
s := "go"
// All true.
println(StringData(s) == StringData(s))
println(*StringData(s) == 'g')
println(*StringData(s[1:]) == 'o')
}
func StringData(s string) *byte {
if len(s) == 0 {
return nil
}
return &[]byte(s)[0]
} ;D |
It looks like we don't need to rely on the common package. The `[]byte` to `string` conversions shouldn't be an issue for us. The Go compiler is smart enough to make some optimizations already and there are some work on this area: golang/go#2205
The text was updated successfully, but these errors were encountered: