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 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
Assignees
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 30, 2019

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 label Aug 30, 2019
@mdempsky mdempsky self-assigned this Aug 30, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 3, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 3, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 3, 2019

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Sep 3, 2019

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

This comment has been minimized.

Copy link
Member Author

@mdempsky 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
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
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>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 27, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 27, 2019

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
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>
@gopherbot gopherbot closed this in a0894ea Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.