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

strings: Builder copy check causes a dynamic memory allocation #23382

Closed
achille-roussel opened this issue Jan 9, 2018 · 4 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@achille-roussel
Copy link
Contributor

What version of Go are you using (go version)?

go version go1.9.1 darwin/amd64

Does this issue reproduce with the latest release?

N/A

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/achilleroussel/dev"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0p/tfjq063j4hdcjnm0_9qb4bww0000gn/T/go-build417403397=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Regarding 3058d38

The change causes the strings.Builder value to always escape to the heap if any of its methods is called, which means there will be 2 dynamic memory allocations to build a string (one for the byte slice and one for the builder).

The intent of this type was to save on memory allocations caused by the use of bytes.Buffer, now it seems like it's just as inefficient.

Here's a quick benchmark to highlight what I'm talking about:

package main

import "testing"

type A struct{ self *A }

func (a *A) F() { a.self = a }

func BenchmarkStruct(b *testing.B) {
    for i := 0; i != b.N; i++ {
        a := A{}
        a.F()
    }
}
$ go test -v -run _ -bench . -benchmem
goos: darwin
goarch: amd64
BenchmarkStruct-4   	50000000	        24.6 ns/op	       8 B/op	       1 allocs/op
@dsnet
Copy link
Member

dsnet commented Jan 9, 2018

\cc @bradfitz

@dsnet dsnet added this to the Go1.10 milestone Jan 9, 2018
@dsnet dsnet added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 9, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Jan 9, 2018

@randall77, @dr2chase, any tips to appease Go's escape analysis?

@cespare
Copy link
Contributor

cespare commented Jan 9, 2018

The self-referencing-pointer-causing-escaping sounds like it might be the same as #7921 (comment). That seems to be the reason that bytes.Buffer always escapes today.

@dsnet dsnet changed the title strings.Builder copy check causes a dynamic memory allocation strings: Builder copy check causes a dynamic memory allocation Jan 9, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/86976 mentions this issue: strings: prevent copyCheck from forcing Builder to escape and allocate

@golang golang locked and limited conversation to collaborators Jan 9, 2019
russjones added a commit to gravitational/teleport that referenced this issue Sep 6, 2019
Don't use strings.ReplaceAll (introduced in Go 1.12) or strings.Builder
(introduced in Go 1.10) which break Go 1.9.7 builds.

Note that strings.Builder can not be vendored because "go vet" fails on
it (causing our build to fail) until the following is resolved:
golang/go#23382
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants