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: possible append inside a method escape analysis optimization #54563

Open
mateusz834 opened this issue Aug 20, 2022 · 5 comments
Open
Assignees
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.
Milestone

Comments

@mateusz834
Copy link
Contributor

mateusz834 commented Aug 20, 2022

type Builder struct {
	buf []byte
}

func (b *Builder) DoSth() {
	b.buf = append(b.buf, 1)
}

func escapes() {
	b := Builder{make([]byte, 0, 10)}
	b.DoSth()
}

BenchmarkEscapes-4 48139129 37.46 ns/op 16 B/op 1 allocs/op

func doesNotEscape() {
	b := Builder{make([]byte, 0, 10)}
	b.buf = append(b.buf, 1)
	_ = b
}

BenchmarkDoesNotEscape-4 1000000000 0.3443 ns/op 0 B/op 0 allocs/op

I think we might just mark the append function implicitly in compiler as noescaping.
Doing so, but explicitly, removes the allocation:

func (b *Builder) DoSth() {
	b.buf = append2(b.buf, 1)
}

//go:noescape
//go:linkname append2 aa.append2helper
func append2(buf []byte, elems ...byte) []byte

func append2helper(buf []byte, elems ...byte) []byte {
	return append(buf, elems...)
}

BenchmarkNoescapeAppend-4 170689034 7.031 ns/op 0 B/op 0 allocs/op

I think it is safe to do so. I can't think of any case where the slice passed to append must be forced to be heap allocated.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 20, 2022
@mateusz834 mateusz834 changed the title cmd/compile: possible append inside a method heap analysis optimization cmd/compile: possible append inside a method escape analysis optimization Aug 20, 2022
@rittneje
Copy link

rittneje commented Aug 20, 2022

I can't think of any case where the slice passed to append must be forced to be heap allocated.

The other argument(s) to append might need to be though. Consider the following program:

package main

import (
	"fmt"
	_ "unsafe"
)

type Builder struct {
	buf []*byte
}

func (b *Builder) DoSth(n byte) {
	b.buf = append2(b.buf, &n)
}

//go:noescape
//go:linkname append2 main.append2helper
func append2(buf []*byte, elems ...*byte) []*byte

func append2helper(buf []*byte, elems ...*byte) []*byte {
	return append(buf, elems...)
}

func main() {
	b := new(Builder)
	b.DoSth(1)
	b.DoSth(2)
	b.DoSth(3)

	for i := range b.buf {
		fmt.Print(*b.buf[i])
	}
	fmt.Println()
}

If you run this with inlining disabled (-gcflags=-l) then it will output 333 instead of 123.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 20, 2022
@mdempsky mdempsky self-assigned this Aug 22, 2022
@gopherbot
Copy link

gopherbot commented Aug 25, 2022

Change https://go.dev/cl/425462 mentions this issue: cmd/compile/internal/escape: optimize b.buf = append(b.buf, ...)

@mdempsky
Copy link
Member

mdempsky commented Aug 25, 2022

I've prototyped the requested optimization in CL 425462 above. Please test it out on more realistic code bases and report back if it helps. (Beware: I'm reasonably confident that it's correct, but I wouldn't recommend using it for any production workloads yet.)

Also, know that it's currently very restrictive on the expressions that can appear in the append arguments list. I think this can be relaxed, but in the mean time you may need to manually spill the values into temporary variables.

@mdempsky
Copy link
Member

mdempsky commented Aug 25, 2022

For example, compiling the source file below with go tool build -gcflags=-m now reports (*Builder).DoSth ignoring self-assignment in b.buf = append(b.buf, p) and make([]*byte, 0, 10) does not escape, while also still printing the correct output "123".

package main

import (
	"fmt"
)

type Builder struct {
	buf []*byte
}

func (b *Builder) DoSth(n byte) {
	p := &n
	b.buf = append(b.buf, p)
}

func main() {
	b := &Builder{buf: make([]*byte, 0, 10)}
	b.DoSth(1)
	b.DoSth(2)
	b.DoSth(3)

	for i := range b.buf {
		fmt.Print(*b.buf[i])
	}
	fmt.Println()
}

@mateusz834
Copy link
Contributor Author

mateusz834 commented Aug 25, 2022

It seems to work fine.

The only problem that I found is while passing the b.buf to some external function, which appends to that buf, it still escapes.

type Builder struct {
	buf []byte
}

func (b *Builder) DoSth(n byte) {
	b.buf = doSth(b.buf, 3)
}

func doSth(msg []byte, n byte) []byte {
	return append(msg, n)
}

func main() {
	b := &Builder{buf: make([]byte, 0, 10)}
	b.DoSth(1)
}

But it is not a big deal, while using pointers the alloc is removed:

func (b *Builder) DoSth2(n byte) {
	doSth2(&b.buf, 3)
}

func doSth2(msg *[]byte, n byte) {
	*msg = append(*msg, n)
}

func main() {
	b := &Builder{buf: make([]byte, 0, 10)}
	b.DoSth2(1)
}

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.
Projects
None yet
Development

No branches or pull requests

5 participants