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

cmd/compile: constant string -> []byte and []byte -> string conversions aren't constant folded #26498

Open
FMNSSun opened this issue Jul 20, 2018 · 12 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@FMNSSun
Copy link

FMNSSun commented Jul 20, 2018

What did you see?

Conversions like

string([]byte{...})
[]byte("...")

are not constant folded.

What did you expect?

Conversions of const literals should probably be constant folded because it significantly slows down
things like

for .... {
  p.Write([]byte(":=")
}

This is related to #26264 but the scope of that issues should probably be the difference between Write, WriteByte, WriteString as some conversions not being constant folded is an issue on a different level that has little to do with the performance of these functions per se.

I can't quite tell whether []rune(...) is also not constant folded because I'm not sure what type.[]int32 actually does but for []byte(...) and string(...) there are calls to runtime.stringtoslicebyte(SB) and runtime.slicebytetostring(SB).

What version of Go are you using (go version)?

1.10.3

@mvdan
Copy link
Member

mvdan commented Jul 20, 2018

Could be a duplicate of #10170, although I'm not sure.

cc @dr2chase @josharian

@mvdan mvdan changed the title Constant string -> []byte and []byte -> string conversions aren't constant folded. cmd/compile: constant string -> []byte and []byte -> string conversions aren't constant folded Jul 20, 2018
@mvdan mvdan added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 20, 2018
@martisch
Copy link
Contributor

martisch commented Jul 20, 2018

Note that p.Write could change the byte slice given to it. So a new byteslice needs to be passed each time as byte slices are not constant. That is what stringtoslicebyte does by copying the contents of the string to a new slice.

The slicebytetostring call in the below example could be avoided by the compiler by referencing a constant string.

func mystring() string {
    return string([]byte{'a','b','c'})
}

@FMNSSun
Copy link
Author

FMNSSun commented Jul 20, 2018

Note that p.Write could change the byte slice given to it. So a new byteslice needs to be passed each time as byte slices are not constant.

That's certainly true yes. But []byte("abc") should essentially be the same as []byte{'a','b','c'} but []byte("abc") has additional runtime costs which []byte{'a','b','c'} doesn't have. In another issue #6643 it mentions that this is disabled due to each byte literal being a node in the (A)ST.
Equally, string([]byte{'a','b','c'}) could be turned into "abc" as strings are immutable - at least spec-wise. But to be fair string([]byte{'a,',b','c'}) (and string([]byte("abc")) is essentially non-sense) is probably rather rare - but []byte("foo") is much more frequent because it's more concise:

[]byte("foo")
[]byte{'f','o','o'}

Once there's a fix/improvement for #6643 this will probably be doable with such a fix. Maybe by adding a special syntax for raw []byte literals who knows.

I think there was a list of common mistakes (or similar). Maybe one can add this to the list to avoid []byte(...) in hot loops and either use

s := []byte(...)
for ... {
  f(s)
}

or

for ... {
  f([]byte{...})
}

Technically with const analysis/escape analysis the compiler could also determine whether the function modifies the slice it receives/or leaks references/pointers and automatically transform

for ... {
 f([]byte(...))
}

to

<temp> := []byte(..)
for ... {
  f(<temp>)
}

(one should pull this out of function scope completely to avoid creating a new slice on each function call.)

but I'm sensing issues with plugins/dynamic libraries there.

@martisch
Copy link
Contributor

martisch commented Jul 20, 2018

Thanks for the clarification.

The problem of detecting if a function modifies or leaks []byte array pointers and not to make a copy is i think captured in #6714.

Its seems true that []byte("abc") has more runtime cost than []byte{'a','b','c'} since the later is just a new object call followed by a memcopy (that may be inlined if short since the length is known at compile time) while the former has a bit more work in a generic runtime conversion function that does not know that the length is known at compile time.

One action for this issue might be to make []byte("abc") as efficient in that regard.

Another that occurs to me while looking at the generated code is that newobject clears the allocated memory that is overwritten immediately. There are no pointers in the backing array so this seems unnecessary.

@FMNSSun
Copy link
Author

FMNSSun commented Jul 20, 2018

There might also be return ... cases that might profit from this:

func f(what int) []byte {
 switch what {
  case 0:
   return []byte("abc")
  ....
  default:
   return []byte("cde")
 }
for ... {
  p.Write(f(j))
}

which is probably easier to detect if the function is inlinable because otherwise the analysis/code gen becomes more complicated as you'd probably almost need two versions of the function depending on whether it's used in a context where it's safe to reuse the returned value because it's only used in a read-only fashion and luckily you can't compare slices so nobody can expect that f(0) != f(0).

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jul 20, 2018
@quasilyte
Copy link
Contributor

quasilyte commented Jul 25, 2018

For benchmark like this:

package foo

import "testing"

//go:noinline
func string2bytes() []byte {
	return []byte("hello, world")
}

//go:noinline
func bytesLit() []byte {
	return []byte{'h', 'e', 'l', 'l', 'o', ',', ' ', 'w', 'o', 'r', 'l', 'd'}
}


func BenchmarkOSTRARRAYBYTE(b *testing.B) {
	for i := 0; i < b.N; i++ {
		// _ = bytesLit()
		_ = string2bytes()
	}
}

We have about 25% performance difference:

name             old time/op  new time/op  delta
OSTRARRAYBYTE-8  59.6ns ± 3%  44.2ns ± 6%  -25.94%  (p=0.000 n=10+10)

For even smaller strings, it's bigger (obviously, for bigger strings alloc/memcpy time will dominate).

I'll send a CL that rewrites []byte("...") into []byte{'.', '.', '.'} in a moment.

This doesn't solve #10170 because escape analysis happens before walk.

@gopherbot
Copy link

Change https://golang.org/cl/125796 mentions this issue: cmd/compile/internal/gc: optimize []byte(stringlit)

@gopherbot
Copy link

Change https://golang.org/cl/140698 mentions this issue: cmd/compile: make []byte("...") more efficient

@gopherbot
Copy link

Change https://golang.org/cl/141118 mentions this issue: cmd/compile: optimize loads from readonly globals into constants

gopherbot pushed a commit that referenced this issue Oct 10, 2018
Do []byte(string) conversions more efficiently when the string
is a constant. Instead of calling stringtobyteslice, allocate
just the space we need and encode the initialization directly.

[]byte("foo") rewrites to the following pseudocode:

var s [3]byte // on heap or stack, depending on whether b escapes
s = *(*[3]byte)(&"foo"[0]) // initialize s from the string
b = s[:]

which generates this assembly:

	0x001d 00029 (tmp1.go:9)	LEAQ	type.[3]uint8(SB), AX
	0x0024 00036 (tmp1.go:9)	MOVQ	AX, (SP)
	0x0028 00040 (tmp1.go:9)	CALL	runtime.newobject(SB)
	0x002d 00045 (tmp1.go:9)	MOVQ	8(SP), AX
	0x0032 00050 (tmp1.go:9)	MOVBLZX	go.string."foo"+2(SB), CX
	0x0039 00057 (tmp1.go:9)	MOVWLZX	go.string."foo"(SB), DX
	0x0040 00064 (tmp1.go:9)	MOVW	DX, (AX)
	0x0043 00067 (tmp1.go:9)	MOVB	CL, 2(AX)
// Then the slice is b = {AX, 3, 3}

The generated code is still not optimal, as it still does load/store
from read-only memory instead of constant stores.  Next CL...

Update #26498
Fixes #10170

Change-Id: I4b990b19f9a308f60c8f4f148934acffefe0a5bd
Reviewed-on: https://go-review.googlesource.com/c/140698
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit that referenced this issue Oct 14, 2018
Instead of
   MOVB go.string."foo"(SB), AX
do
   MOVB $102, AX

When we know the global we're loading from is readonly, we can
do that read at compile time.

I've made this arch-dependent mostly because the cases where this
happens often are memory->memory moves, and those don't get
decomposed until lowering.

Did amd64/386/arm/arm64. Other architectures could follow.

Update #26498

Change-Id: I41b1dc831b2cd0a52dac9b97f4f4457888a46389
Reviewed-on: https://go-review.googlesource.com/c/141118
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@griesemer griesemer modified the milestones: Go1.12, Go1.13 Dec 6, 2018
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@gopherbot
Copy link

Change https://golang.org/cl/185477 mentions this issue: cmd/compile: apply optimization for readonly globals on wasm

gopherbot pushed a commit that referenced this issue Aug 28, 2019
Extend the optimization introduced in CL 141118 to the wasm architecture.

And for reference, the rules trigger 212 times while building std and cmd

$GOOS=js GOARCH=wasm gotip build std cmd
$grep -E "Wasm.rules:44(1|2|3|4)" rulelog | wc -l
212

Updates #26498

Change-Id: I153684a2b98589ae812b42268da08b65679e09d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/185477
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Richard Musiol <neelance@gmail.com>
tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
Extend the optimization introduced in CL 141118 to the wasm architecture.

And for reference, the rules trigger 212 times while building std and cmd

$GOOS=js GOARCH=wasm gotip build std cmd
$grep -E "Wasm.rules:44(1|2|3|4)" rulelog | wc -l
212

Updates golang#26498

Change-Id: I153684a2b98589ae812b42268da08b65679e09d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/185477
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Richard Musiol <neelance@gmail.com>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Extend the optimization introduced in CL 141118 to the wasm architecture.

And for reference, the rules trigger 212 times while building std and cmd

$GOOS=js GOARCH=wasm gotip build std cmd
$grep -E "Wasm.rules:44(1|2|3|4)" rulelog | wc -l
212

Updates golang#26498

Change-Id: I153684a2b98589ae812b42268da08b65679e09d1
Reviewed-on: https://go-review.googlesource.com/c/go/+/185477
Run-TryBot: Agniva De Sarker <agniva.quicksilver@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Richard Musiol <neelance@gmail.com>
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

Change https://golang.org/cl/280646 mentions this issue: cmd/compile: loads from readonly globals into const for mips64x

gopherbot pushed a commit that referenced this issue Mar 16, 2021
Ref: CL 141118
Update #26498

Change-Id: If4ea55c080b9aa10183eefe81fefbd4072deaf3a
Reviewed-on: https://go-review.googlesource.com/c/go/+/280646
Trust: Meng Zhuo <mzh@golangcn.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@quasilyte
Copy link
Contributor

quasilyte commented Nov 11, 2021

An updated: both forms []byte("str") and []byte{'s', 't', 'r'} are now identically efficient.
They produce the same machine code in the contexts I've tried.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Status: Triage Backlog
Development

No branches or pull requests

9 participants