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: []byte(string) incurs allocation even when it does not escape #20881

Open
rogpeppe opened this issue Jul 1, 2017 · 10 comments
Open
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@rogpeppe
Copy link
Contributor

rogpeppe commented Jul 1, 2017

The following Go program reports that the function makes an allocation to do the string to []byte conversion, even though the compiler reports the conversion as non-escaping.

https://play.golang.org/p/y2d2RiYqK5

It prints:

  48 B/op	       1 allocs/op

Given that the compiler has marked it as non-escaping, I'd expect no allocations.

go version devel +eab99a8 Mon Jun 26 21:12:22 2017 +0000 linux/amd64

@randall77
Copy link
Contributor

It's non-escaping, but its size is unknown. We need to allocate []byte of unknown size on the heap.

@bcmills
Copy link
Contributor

bcmills commented Jul 1, 2017

We shouldn't even need to do that if the slice isn't mutated, right? (How hard would it be to make the escape analysis also do mutability?)

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Jul 1, 2017

For the record I encountered this issue when passing a string to hex.Decode. Mutability analysis would be amazing, but just avoiding the allocation would be nice.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 1, 2017

Dup of #2205 ?

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Jul 3, 2017

Although the particular example would be fixed if #2205 was fixed, I don't think it's quite the same issue. I hadn't realised that all variable-length allocations escaped (strictly speaking that's not true, because []byte(string) where len(string)<=32 is stack-allocated), and I'm not sure that others do either.

I'm probably not the only one who was using -gcflags -m output as a proxy for whether there's an allocation or not. Given that we're (I presume) not about to make some kind of instant-free arena for variable-length items, perhaps another way of addressing this issue might be to change the output printed by the -m flag to make it clearer that this the result of this kind of operation may actually escape to the heap.

@robpike
Copy link
Contributor

robpike commented Jul 4, 2017

Sure, it's escaped to the heap but that's as far as it will get. The guard towers that surround the heap are well-manned and your memory will never get outside the heap's walls. You can relax.

@andybons
Copy link
Member

@rogpeppe is your proposal to change the output of the -m flag to make this clearer? Is this worthy of a proposal or NeedsDecision? Trying to figure out next steps.

@andybons andybons added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 11, 2018
@andybons andybons added this to the Unplanned milestone Apr 11, 2018
@bradfitz
Copy link
Contributor

Unplanned, needs fix, performance.

No decision needed. This is just work, and somewhat tricky work.

@andybons andybons added Performance NeedsFix The path to resolution is known, but the work has not been done. help wanted and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Apr 11, 2018
@andybons
Copy link
Member

Thanks, Brad. Updated.

@josharian
Copy link
Contributor

Given that we're (I presume) not about to make some kind of instant-free arena for variable-length items

Cross-reference: #20533

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

7 participants