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: improve escape analysis of make([]T, len, cap) where len is non-constant #37975

Closed
TriAnMan opened this issue Mar 20, 2020 · 3 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@TriAnMan
Copy link

TriAnMan commented Mar 20, 2020

$ go version
go version go1.14.1 windows/amd64

$ cat test.go
package main

func main() {
	i := 2
	s := make([]int, 2, 3)
	s = make([]int, i, 3)
	s = make([]int, i, 1)
	s[0] = 1
}

$ go run -gcflags -m=2 test.go
# command-line-arguments
.\test.go:3:6: can inline main as: func() { i := 2; s := make([]int, 2, 3); s = make([]int, i, 3); s = make([]int, i, 1); s[0] = 1 }
.\test.go:6:10: make([]int, i, 3) escapes to heap:
.\test.go:6:10:   flow: {heap} = &{storage for make([]int, i, 3)}:
.\test.go:6:10:     from make([]int, i, 3) (non-constant size) at .\test.go:6:10
.\test.go:7:10: make([]int, i, 1) escapes to heap:
.\test.go:7:10:   flow: {heap} = &{storage for make([]int, i, 1)}:
.\test.go:7:10:     from make([]int, i, 1) (non-constant size) at .\test.go:7:10
.\test.go:5:11: make([]int, 2, 3) does not escape
.\test.go:6:10: make([]int, i, 3) escapes to heap
.\test.go:7:10: make([]int, i, 1) escapes to heap
panic: runtime error: makeslice: cap out of range

goroutine 1 [running]:
main.main()
        C:/cygwin64/home/work/projects/test/test.go:7 +0x68
exit status 2

In this program, the compiler's escape analysis judges that the array allocated by make escapes to the heap, when len is variable. But actually this array can be allocated on stack because we can't make len greater than cap in runtime and cap is constant.

@ivzhh
Copy link

ivzhh commented Mar 21, 2020

Logic check stack on master branch (commit hash: 287d67e3dd3972b1d1006b06e0d57929540a1591
)

func isSmallMakeSlice(n *Node) bool: cmd/compile/internal/gc/walk.go:364
func mustHeapAlloc(n *Node) bool: cmd/compile/internal/gc/esc.go:190
func (e *Escape) newLoc(n *Node, transient bool) *EscLocation: cmd/compile/internal/gc/escape.go:1066

The test for smallintconst(l) fails and escapes this slice into heap.

func isSmallMakeSlice(n *Node) bool {
	if n.Op != OMAKESLICE {
		return false
	}
	l := n.Left
	r := n.Right
	if r == nil {
		r = l
	}
	t := n.Type

	return smallintconst(l) && smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)
}

If cap is not set, then len == cap, so in any case, the compile only needs to check whether r (aka cap) is constant. The validity of len, as suggested by @TriAnMan, is checked at runtime. So we can always use:

- return smallintconst(l) && smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)
+ return smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 23, 2020
@gopherbot
Copy link

Change https://golang.org/cl/226278 mentions this issue: cmd/compile: make isSmallMakeSlice checks slice cap only

@ivzhh
Copy link

ivzhh commented Mar 29, 2020

Seems related to #20533 and this cl.

@golang golang locked and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

5 participants