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

Jump to bottom

cmd/compile: improve escape analysis fidelity #33981

Closed
mdempsky opened this issue Aug 30, 2019 · 7 comments
Closed

cmd/compile: improve escape analysis fidelity #33981

mdempsky opened this issue Aug 30, 2019 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.14

Comments

@mdempsky
Copy link
Member

Currently, when compiling the test case below, escape analysis decides that both i and j escape and need to be heap allocated. However, actually only i escapes; j is safe to stack allocate.

The reason for this is that the escape analysis tags use a single-bit flag EscContentEscapes to mark if anything pointed to by a parameter escape to the heap; so callers can't distinguish g = **p from g = *p.

Contrast with how escapeReturnEncoding uses multiple bits to store a fixnum value, so it can distinguish return p, return *p, return **p, etc.

Moreover, while we currently track these values in a uint16 during escape analysis, they're eventually converted to strings and serialized in export data as strings, so the 16-bits constrained is self-imposed.

package p

var g *int

//go:noinline
func f(p ***int) { g = **p }

func e() {
    var i int
    j := &i      // BAD: 'j' is currently heap allocated, but needn't be
    k := &j
    f(&k)
}
@mdempsky mdempsky added this to the Go1.14 milestone Aug 30, 2019
@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 30, 2019
@mdempsky mdempsky self-assigned this Aug 30, 2019
@gopherbot
Copy link

Change https://golang.org/cl/193178 mentions this issue: cmd/compile: integrate parameter tagging into new escape analysis

@gopherbot
Copy link

Change https://golang.org/cl/193177 mentions this issue: cmd/compile: refactor escape analysis parameter tagging

@gopherbot
Copy link

Change https://golang.org/cl/193179 mentions this issue: cmd/compile: improve fidelity of escape analysis tagging

@mdempsky
Copy link
Member Author

mdempsky commented Sep 3, 2019 •
edited

A little disappointing, but it looks like this only really helps cmd/compile/internal/gc.substArgTypes; in particular, the ... argument no longer escapes to the heap. So the ~30 call sites all benefit.

This is because the improved fidelity for cmd/compile/internal/types.SubstAny (used by gc.substArgTypes) can now report that for the types *[]*Type parameter, only the referenced *Type values escape; the []*Type itself does not escape.

It also allows a few maps to be stack allocated in pprof:

-cmd/vendor/github.com/google/pprof/profile/merge.go:50:18: make(map[sampleKey]*Sample, len(srcs[0].Sample)) escapes to heap
-cmd/vendor/github.com/google/pprof/profile/merge.go:51:18: make(map[locationKey]*Location, len(srcs[0].Location)) escapes to heap
-cmd/vendor/github.com/google/pprof/profile/merge.go:52:18: make(map[functionKey]*Function, len(srcs[0].Function)) escapes to heap
-cmd/vendor/github.com/google/pprof/profile/merge.go:53:18: make(map[mappingKey]*Mapping, len(srcs[0].Mapping)) escapes to heap

@mdempsky
Copy link
Member Author

mdempsky commented Sep 3, 2019

Building all of Kubernetes (go build k8s.io/kubernetes/...) yields slightly better results; 145 improvements in total.

145 escape analysis improvements 
-../../../pkg/mod/github.com/aws/aws-sdk-go@v1.16.26/aws/session/session.go:258:28: moved to heap: opts
-../../../pkg/mod/github.com/aws/aws-sdk-go@v1.16.26/aws/session/session.go:351:13: &aws.Config literal escapes to heap
-../../../pkg/mod/github.com/aws/aws-sdk-go@v1.16.26/service/ec2/customizations.go:90:36: &aws.Config literal escapes to heap
-../../../pkg/mod/github.com/coreos/etcd@v3.3.15+incompatible/etcdserver/server.go:1460:4: &etcdserverpb.AlarmRequest literal escapes to heap
-../../../pkg/mod/github.com/coreos/etcd@v3.3.15+incompatible/etcdserver/server.go:833:60: &etcdserverpb.LeaseRevokeRequest literal escapes to heap
-../../../pkg/mod/github.com/coreos/etcd@v3.3.15+incompatible/etcdserver/v3_server.go:384:4: &etcdserverpb.InternalAuthenticateRequest literal escapes to heap
-../../../pkg/mod/github.com/coreos/etcd@v3.3.15+incompatible/etcdserver/v3_server.go:559:3: &etcdserverpb.RequestHeader literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/runtime@v0.19.0/csv.go:36:29: &bufio.Writer literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/runtime@v0.19.0/csv.go:68:29: &bufio.Writer literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/schema_props.go:102:24: new(Result) escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/schema_props.go:103:24: new(Result) escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/schema_props.go:104:24: new(Result) escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/spec.go:47:39: &SpecValidator literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:522:20: new(Result) escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:523:19: new(Result) escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:524:19: new(Result) escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:539:37: &Result literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:544:37: &Result literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:555:36: &Result literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:560:36: &Result literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:571:36: &Result literal escapes to heap
-../../../pkg/mod/github.com/go-openapi/validate@v0.19.2/validator.go:576:36: &Result literal escapes to heap
-../../../pkg/mod/github.com/google/cadvisor@v0.34.0/utils/cloudinfo/aws/aws.go:60:40: &aws.Config literal escapes to heap
-../../../pkg/mod/github.com/gophercloud/gophercloud@v0.1.0/openstack/client.go:91:34: composite literal escapes to heap
-../../../pkg/mod/github.com/gophercloud/gophercloud@v0.1.0/openstack/client.go:92:34: composite literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:136:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:137:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:161:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:162:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:174:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:175:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:176:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:202:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:203:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:204:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:205:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:294:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:295:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:296:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:297:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:298:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:299:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:300:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:363:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:373:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:383:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:400:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:401:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:402:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:403:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:404:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:455:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:456:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:618:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:661:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/heketi/heketi@v9.0.0+incompatible/pkg/glusterfs/api/types.go:96:19: &validation.FieldRules literal escapes to heap
-../../../pkg/mod/github.com/spf13/pflag@v1.0.3/string_slice.go:33:20: &bufio.Writer literal escapes to heap
-../../../pkg/mod/github.com/spf13/pflag@v1.0.3/string_to_string.go:71:20: &bufio.Writer literal escapes to heap
-cmd/kubeadm/app/phases/copycerts/copycerts.go:67:4: &"k8s.io/apimachinery/pkg/apis/meta/v1".Duration literal escapes to heap
-pkg/controller/endpointslice/reconciler.go:202:10: moved to heap: endpoint
-pkg/kubelet/dockershim/docker_stats.go:37:12: &v1alpha2.ContainerFilter literal escapes to heap
-pkg/kubelet/kuberuntime/kuberuntime_container.go:378:2: moved to heap: value
-pkg/volume/util/resize_util.go:270:3: &mount.SafeFormatAndMount literal escapes to heap
-staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/resources.go:607:4: composite literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:1130:35: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:2060:3: &"k8s.io/api/core/v1".Taint literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:777:3: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:799:3: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:817:3: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:836:3: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:854:3: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/aws/aws.go:867:3: &aws.Config literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/azure/azure_blobDiskController.go:215:44: &"github.com/Azure/azure-sdk-for-go/storage".Container literal escapes to heap
-staging/src/k8s.io/legacy-cloud-providers/azure/azure_blobDiskController.go:267:44: &"github.com/Azure/azure-sdk-for-go/storage".Container literal escapes to heap
-test/e2e/apimachinery/watch.go:373:55: []"k8s.io/apimachinery/pkg/apis/meta/v1".LabelSelectorRequirement literal escapes to heap
-test/e2e/framework/util.go:2078:69: moved to heap: taint
-test/e2e/kubectl/kubectl.go:1033:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1034:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1035:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1036:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1037:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1038:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1039:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1040:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1041:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1042:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1043:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1044:6: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1051:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1052:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1053:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1054:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1055:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1056:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1057:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1058:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1059:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1060:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1061:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1067:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1068:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1069:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1070:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1071:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1072:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1073:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1074:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1075:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1076:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1077:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1087:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1088:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1089:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1090:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1091:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1092:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1093:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1094:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1095:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1096:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1097:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1098:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1099:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1100:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1101:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1107:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1108:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1109:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1110:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1874:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1875:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1876:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1905:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1906:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1907:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1923:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1924:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1925:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1941:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1942:5: composite literal escapes to heap
-test/e2e/kubectl/kubectl.go:1943:5: composite literal escapes to heap
-test/e2e/storage/persistent_volumes-local.go:311:12: &"k8s.io/kubernetes/test/e2e/storage/utils".LocalTestResource literal escapes to heap
-test/e2e/storage/persistent_volumes-local.go:313:5: &localTestVolume literal escapes to heap
-test/e2e/storage/persistent_volumes-local.go:619:12: &"k8s.io/kubernetes/test/e2e/storage/utils".LocalTestResource literal escapes to heap

gopherbot pushed a commit that referenced this issue Sep 10, 2019
@mdempsky
cmd/compile: refactor escape analysis parameter tagging
23bf6af 
No behavior change; just inverting the loop ordering so the
per-parameter behavior is a bit clearer.

Passes toolstash-check.

Updates #33981.

Change-Id: I9bfcd7d0a4aff65a27ced157767ca2ba8038319a
Reviewed-on: https://go-review.googlesource.com/c/go/+/193177
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
gopherbot pushed a commit that referenced this issue Sep 10, 2019
@mdempsky
cmd/compile: better integrate parameter tagging with escape.go
b970487 
This CL moves parameter tagging to before escape analysis is complete,
so we still have access to EscLocation. This will be useful once
EscLocation starts tracking higher-fidelity escape details.

Notably, this CL stops using n.Esc to record parameter escape analysis
details. Now escape analysis only ever sets n.Esc to EscNone or
EscHeap. (It still defaults to EscUnknown, and is set to EscNever in
some places though.)

Passes toolstash-check.

Updates #33981.

Change-Id: I50a91ea1e38c442092de6cd14e20b211f8f818c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/193178
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@mdempsky mdempsky mentioned this issue Sep 25, 2019
Closed
@gopherbot
Copy link

Change https://golang.org/cl/197679 mentions this issue: cmd/compile: introduce EscLeaks abstraction

@gopherbot
Copy link

Change https://golang.org/cl/197680 mentions this issue: cmd/compile: reimplement parameter leak encoding

gopherbot pushed a commit that referenced this issue Oct 7, 2019
@mdempsky
cmd/compile: introduce EscLeaks abstraction
05a805a 
This CL better abstracts away the parameter leak info that was
directly encoded into the uint16 value. Followup CL will rewrite the
implementation.

Passes toolstash-check.

Updates #33981.

Change-Id: I27f81d26f5dd2d85f5b0e5250ca529819a1f11c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/197679
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Oct 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
2 participants
@mdempsky @gopherbot