From 484586c81a0196e42ac52f651bc56017ca454280 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 9 Jan 2018 18:08:43 +0000 Subject: [PATCH] strings: prevent copyCheck from forcing Builder to escape and allocate All credit and blame goes to Ian for this suggestion, copied from the runtime. Fixes #23382 Updates #7921 Change-Id: I3d5a9ee4ab730c87e0f3feff3e7fceff9bcf9e18 Reviewed-on: https://go-review.googlesource.com/86976 Reviewed-by: Ian Lance Taylor --- src/strings/builder.go | 19 ++++++++++++++++++- src/strings/builder_test.go | 12 ++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/strings/builder.go b/src/strings/builder.go index 11bcee1dfcd..ac58f34e1de 100644 --- a/src/strings/builder.go +++ b/src/strings/builder.go @@ -17,9 +17,26 @@ type Builder struct { buf []byte } +// noescape hides a pointer from escape analysis. noescape is +// the identity function but escape analysis doesn't think the +// output depends on the input. noescape is inlined and currently +// compiles down to zero instructions. +// USE CAREFULLY! +// This was copied from the runtime; see issues 23382 and 7921. +//go:nosplit +func noescape(p unsafe.Pointer) unsafe.Pointer { + x := uintptr(p) + return unsafe.Pointer(x ^ 0) +} + func (b *Builder) copyCheck() { if b.addr == nil { - b.addr = b + // This hack works around a failing of Go's escape analysis + // that was causing b to escape and be heap allocated. + // See issue 23382. + // TODO: once issue 7921 is fixed, this should be reverted to + // just "b.addr = b". + b.addr = (*Builder)(noescape(unsafe.Pointer(b))) } else if b.addr != b { panic("strings: illegal use of non-zero Builder copied by value") } diff --git a/src/strings/builder_test.go b/src/strings/builder_test.go index c0c8fa4130f..ecbaeaa5c17 100644 --- a/src/strings/builder_test.go +++ b/src/strings/builder_test.go @@ -180,6 +180,18 @@ func TestBuilderAllocs(t *testing.T) { if allocs > 0 { t.Fatalf("got %d alloc(s); want 0", allocs) } + + // Issue 23382; verify that copyCheck doesn't force the + // Builder to escape and be heap allocated. + n := testing.AllocsPerRun(10000, func() { + var b Builder + b.Grow(5) + b.WriteString("abcde") + _ = b.String() + }) + if n != 1 { + t.Errorf("Builder allocs = %v; want 1", n) + } } func numAllocs(fn func()) uint64 {