cmd/compile: avoid using large stack to partially zero struct #18636
Comments
dsnet
changed the title
|
CL https://golang.org/cl/35122 mentions this issue. |
|
Same deal on 1.8. It works if B is an int instead of a string. That's a bug with strings - we could treat constant strings as static. For non-constant initialization we allocate a temp, initialize it, then write it to the destination. We shouldn't have to do that. Let's make this bug about the latter. I'll open a separate one for the former. |
|
I didn't realize that strings made a difference. See the changes made in deflatefast.go in http://golang.org/cl/35122 for the specific use-case that was causing trouble. |
|
It seems the culprit is due to populating the field for a slice. |
Ranging over an array causes the array to be copied over to the stack, which cause large re-growths. Instead, we should iterate over slices of the array. Also, assigning a large struct literal uses the stack even though the actual fields being populated are small in comparison to the entirety of the struct (see #18636). Fixing the stack growth does not alter CPU-time performance much since the stack-growth and copying was such a tiny portion of the compression work: name old time/op new time/op delta Encode/Digits/Default/1e4-8 332µs ± 1% 332µs ± 1% ~ (p=0.796 n=10+10) Encode/Digits/Default/1e5-8 5.07ms ± 2% 5.05ms ± 1% ~ (p=0.815 n=9+8) Encode/Digits/Default/1e6-8 53.7ms ± 1% 53.9ms ± 1% ~ (p=0.075 n=10+10) Encode/Twain/Default/1e4-8 380µs ± 1% 380µs ± 1% ~ (p=0.684 n=10+10) Encode/Twain/Default/1e5-8 5.79ms ± 2% 5.79ms ± 1% ~ (p=0.497 n=9+10) Encode/Twain/Default/1e6-8 61.5ms ± 1% 61.8ms ± 1% ~ (p=0.247 n=10+10) name old speed new speed delta Encode/Digits/Default/1e4-8 30.1MB/s ± 1% 30.1MB/s ± 1% ~ (p=0.753 n=10+10) Encode/Digits/Default/1e5-8 19.7MB/s ± 2% 19.8MB/s ± 1% ~ (p=0.795 n=9+8) Encode/Digits/Default/1e6-8 18.6MB/s ± 1% 18.5MB/s ± 1% ~ (p=0.072 n=10+10) Encode/Twain/Default/1e4-8 26.3MB/s ± 1% 26.3MB/s ± 1% ~ (p=0.616 n=10+10) Encode/Twain/Default/1e5-8 17.3MB/s ± 2% 17.3MB/s ± 1% ~ (p=0.484 n=9+10) Encode/Twain/Default/1e6-8 16.3MB/s ± 1% 16.2MB/s ± 1% ~ (p=0.238 n=10+10) Updates #18636 Fixes #18625 Change-Id: I471b20339bf675f63dc56d38b3acdd824fe23328 Reviewed-on: https://go-review.googlesource.com/35122 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
If my repro passes at tip, then I'm happy. I'll file another issue if I run into anything. Thanks! |
Updates #18636 Change-Id: I143c670c3940231e29f1814e0a03165682f53243 Reviewed-on: https://go-review.googlesource.com/43621 Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
|
Re-opening as this still occurs with a minor variation of the above code: type Foo struct {
A [1 << 20]byte
B []byte
}
func run(c chan bool) {
f := new(Foo)
*f = Foo{B: f.B[:0]}
c <- true
}
func main() {
debug.SetMaxStack(1 << 16)
c := make(chan bool)
go run(c)
<-c
} |
|
CL https://golang.org/cl/43621 mentions this issue. |
|
@dsnet your latest reproduce code can now compiled successfully at tip, and go1.14, go1.15 We can close this issue. |
|
@cuonglm thank you for confirming. Closing |
Using
go1.7.3.Consider the following program:
I currently see:
I expect to see:
The
Foostruct contains a large embedded array. We assign to*fwithFoo{B: "hello"}, which causes the stack to grow to accommodate the large embedded array. SinceAwas never named in the struct literal, the compiler should know that it is going to be all zeros and use a memclr directly to the heap, rather copying it through the stack.It is common to use the pattern above in reset logic to reset everything to zero except for the logic that needs to be initialized:
\cc @randall77 @dr2chase
The text was updated successfully, but these errors were encountered: