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: self assigned append should not escape #51462

Open
dsnet opened this issue Mar 3, 2022 · 7 comments
Open

cmd/compile: self assigned append should not escape #51462

dsnet opened this issue Mar 3, 2022 · 7 comments
Labels
NeedsInvestigation Performance
Milestone

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Mar 3, 2022

Using go1.17.5.

In the following snippet:

bb := &Buffer{buf: make([]byte, 0, 64)}
bb.buf = append(bb.buf)

the buffer for make([]byte, 0, 64) escapes to the heap because escape analysis cannot seem to determine that bb.buf = append(bb.buf) does not cause buf to escape.

I expect the code above to be able to stack allocate make([]byte, 0, 64).

\cc @josharian @mdempsky

@josharian theorizes that we might need a special case in https://github.com/golang/go/blame/master/src/cmd/compile/internal/escape/utils.go

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 3, 2022

Dup of #27772 ?

@dsnet
Copy link
Member Author

@dsnet dsnet commented Mar 3, 2022

Dup of #27772 ?

Possibly. #27772 seems like a more general issue, while this issue targets a specific case that seems more tenable to fixing in the short term.

@beoran
Copy link

@beoran beoran commented Mar 5, 2022

In this case, the append does nothing, though. I feel that this is a case of an incorrect use of append, so that doesn't matter much.

@josharian
Copy link
Contributor

@josharian josharian commented Mar 5, 2022

I believe that it also reproduces with non-trivial uses of append, and that making it non-trivial doesn't alter the escape analysis. This showed up in practice in https://go-review.googlesource.com/c/go/+/349994.

@cagedmantis cagedmantis added the NeedsInvestigation label Mar 7, 2022
@cagedmantis cagedmantis added this to the Backlog milestone Mar 7, 2022
@Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Mar 8, 2022

In this case, the append does nothing, though. I feel that this is a case of an incorrect use of append, so that doesn't matter much.

I sometime see things like this:

// assume b, c and d sizes are known and small enough
a := make([]byte, len(b)+len(c)+len(d))[:0]
a = append(a, b...)
a = append(a, c...)
a = append(a, d...)

It is mesurabely faster than initialising like this:

var a []byte // = nil

While being more readable and less error prone than a copy based version (even tho copy would be somewhat faster here).

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 9, 2022

Change https://go.dev/cl/349994 mentions this issue: bytes: rely on runtime.growslice for growing

gopherbot pushed a commit that referenced this issue Mar 10, 2022
Rather than naively making a slice of capacity 2*c+n,
rely on the append(..., make(...)) pattern to allocate a
slice that aligns up to the closest size class.

Performance:
	name                          old time/op    new time/op    delta
	BufferWriteBlock/N4096       3.03µs ± 6%    2.04µs ± 6%  -32.60%  (p=0.000 n=10+10)
	BufferWriteBlock/N65536      47.8µs ± 6%    28.1µs ± 2%  -41.32%  (p=0.000 n=9+8)
	BufferWriteBlock/N1048576     844µs ± 7%     510µs ± 5%  -39.59%  (p=0.000 n=8+9)

	name                          old alloc/op   new alloc/op   delta
	BufferWriteBlock/N4096       12.3kB ± 0%     7.2kB ± 0%  -41.67%  (p=0.000 n=10+10)
	BufferWriteBlock/N65536       258kB ± 0%     130kB ± 0%  -49.60%  (p=0.000 n=10+10)
	BufferWriteBlock/N1048576    4.19MB ± 0%    2.10MB ± 0%  -49.98%  (p=0.000 n=10+8)

	name                          old allocs/op  new allocs/op  delta
	BufferWriteBlock/N4096         3.00 ± 0%      3.00 ± 0%     ~     (all equal)
	BufferWriteBlock/N65536        7.00 ± 0%      7.00 ± 0%     ~     (all equal)
	BufferWriteBlock/N1048576      11.0 ± 0%      11.0 ± 0%     ~     (all equal)

The performance is faster since the growth rate is capped at 2x,
while previously it could grow by amounts potentially much greater than 2x,
leading to significant amounts of memory waste and extra copying.

Credit goes to Martin Möhrmann for suggesting the
append(b, make([]T, n)...) pattern.

Fixes #42984
Updates #51462

Change-Id: I7b23f75dddbf53f8b8b93485bb1a1fff9649b96b
Reviewed-on: https://go-review.googlesource.com/c/go/+/349994
Trust: Joseph Tsai <joetsai@digital-static.net>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@quasilyte
Copy link
Contributor

@quasilyte quasilyte commented Mar 20, 2022

Leaving it here for the completeness https://go-review.googlesource.com/c/go/+/370578

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Performance
Projects
None yet
Development

No branches or pull requests

8 participants