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: skip slicebytetostring when argument doesn't escape #6714

Open
bradfitz opened this issue Nov 4, 2013 · 21 comments
Open

cmd/compile: skip slicebytetostring when argument doesn't escape #6714

bradfitz opened this issue Nov 4, 2013 · 21 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 4, 2013

In this function,

func hi() string {
        b := make([]byte, 0, 100) // 1st alloc
        b = append(b, "Hello, "...)
        b = append(b, "world.\n"...)
        return string(b) // 2nd alloc, but could be removed.
}

The final line currently generates a call to runtime.slicebytetostring, causing a copy
of b to be allocated.

But the compiler already knows that b doesn't escape.  It could instead return a string
header re-using the []byte memory.
@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 1:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 2:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 3:

Labels changed: added repo-main.

@josharian
Copy link
Contributor

@josharian josharian commented Dec 24, 2013

Comment 4:

I've poked at this a bit and wanted to get some feedback before I continued.
As written, hi only allocs once, since b is allocated on the stack. A quick benchmem run
bears this out. If b's cap is much larger or unknown at compile time, then yes, two
allocs occur.
I cobbled together a very crude patch that returns a string header in case cases, as
suggested. It is possibly underpowered, but it found only a few opportunities in the
stdlib for such an optimization:
strings/replace.go:502
strings/strings.go:367
strings/strings.go:430
strconv/quote.go:82
strconv/quote.go:349
encoding/base64/base64.go:121
os/env.go:26
path/filepath/path.go:314
net/url/url.go:168
net/url/url.go:211
Running the existing benchmarks, the only alloc count improvements are:
strings BenchmarkByteStringMatch: 2 -> 1
strings BenchmarkHTMLEscapeNew: 2 -> 1
strings BenchmarkCountTortureOverlapping: 4 -> 2
strconv BenchmarkUnquoteHard: 2 -> 1
net/url BenchmarkString: 44 -> 39
Extending to rune slices adds only two call sites:
compress/gzip/gunzip.go:118
testing/quick/quick.go:127
I'd need non-trivial direction and input to bring my patch up to snuff...or more likely,
rewrite it from scratch after some hints. (I am finding my way by touch; all the tests
pass, though, so that's something.)
Given all the above, should I continue to pursue this?
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 28, 2014

Comment 5 by unclejacksons:

Avoiding allocations for bytes to string conversions where possible would be very useful.
This would help in hot loops and in code where lots of items are processed. One less
allocation can make a big difference.
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/gc: skip slicebytetostring when argument doesn't escape cmd/compile: skip slicebytetostring when argument doesn't escape Jun 8, 2015
@OneOfOne
Copy link
Contributor

@OneOfOne OneOfOne commented Mar 15, 2016

Maybe this should change the milestone to 1.7?

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 15, 2016

No, this isn't critical to 1.7 or any particular release. This is not a regression or anything.

gopherbot pushed a commit that referenced this issue Aug 16, 2016
We already had special cases for 0 and 1. Add 2 and 3 for now too.
To be removed if the compiler is improved later (#6714).

This halves the number of allocations and total bytes allocated via
common filepath.Join calls, improving filepath.Walk performance.

Noticed as part of investigating filepath.Walk in #16399.

Change-Id: If7b1bb85606d4720f3ebdf8de7b1e12ad165079d
Reviewed-on: https://go-review.googlesource.com/25005
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Aug 16, 2016

If this is fixed, revert https://golang.org/cl/25005

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Aug 16, 2016

Is anyone working on this? I'd be interested in trying to fix it, though I've never worked on the compiler part of this project before.

@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 16, 2016

I don't think anyone is working on it.

@aclements
Copy link
Member

@aclements aclements commented Aug 17, 2016

Note that "b does not escape" is not a sufficient condition. The real question is "is this the only reference to b's backing array?", which is a related but different question. For example,

func hi() string {
        b := make([]byte, 0, 100) // 1st alloc
        b = append(b, "Hello, "...)
        b = append(b, "world.\n"...)
        s := string(b)
        b[0] = 'X'
        return s
}

b does not escape here, but it's not safe to use stringtoslicebytetmp because b's backing array is still reachable from the stack.

This may be a liveness question, not an escape question. If the last point b's backing array is live is at the string conversion, then it should be safe to reuse it.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 20, 2016

CL https://golang.org/cl/27327 mentions this issue.

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Nov 11, 2016

I'm going to take another crack at this this week.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 11, 2016

OK but too late for Go 1.8. We'll review what you send when we get to Go 1.9.

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Nov 11, 2016

Understood! I still need to get up to speed with e.g. "how to figure out whether a variable escapes" but there has never been a better time to learn.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Feb 8, 2017

See also: #18990

@rkravchik
Copy link

@rkravchik rkravchik commented Oct 10, 2017

@rsc it's late for Go 1.9 too :)

@JAicewizard
Copy link

@JAicewizard JAicewizard commented Feb 16, 2019

is there any news on this?

@DmitriyMV
Copy link

@DmitriyMV DmitriyMV commented Feb 16, 2019

@JAicewizard I think it was postponed because of the introduction of strings.Builder.

@pascaldekloe
Copy link
Contributor

@pascaldekloe pascaldekloe commented Oct 3, 2019

@martisch
Copy link
Contributor

@martisch martisch commented Oct 3, 2019

Small fixed length slices that do not escape are allocated on the stack. I think this has been for a while. Having a variable byte slice and then a string conversion as far as I know still causes two heap allocations if the resulting string escapes even if there are no other references to the byte slice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet