Skip to content

proposal: spec&runtime: allows makeslice to return more capacity than asked for #52123

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

Closed
Jorropo opened this issue Apr 3, 2022 · 3 comments
Closed

Comments

@Jorropo
Copy link
Member

Jorropo commented Apr 3, 2022

Context

I am implementing a opt slice optimization pass. And It's made more complex and less effective because the spec defines make([]any, a, b) as:

produces the same slice as allocating an array and slicing it, so these two expressions are equivalent:

make([]int, 50, 100)
new([100]int)[0:50]

My code fuse straight slice operations, so for example I want to turn this code:

s := make([]int, 10)
doStuff(s)
s = append(s, 1,2,3)

Into:

s := make([]int, 10, 13)
doStuff(s)
s = a[:13]
copy(s[len(s):], [...]{1,2,3})

But I cannot because that not up to the spec since calling cap(s) in doStuff would result in 13 not 10 like asked.
So I do this instead:

tempSlice := make([]int, 13, 13)
s := tempSlice[:10:10]
doStuff(s)
s = tempSlice
copy(s[len(s):], [...]{1,2,3})

The definition of append is much friendlier to my case:

If the capacity of s is not large enough to fit the additional values, append allocates a new, sufficiently large underlying array that fits both the existing slice elements and the additional values. Otherwise, append re-uses the underlying array.

There are negligible performance implication between the two examples, the main issue I have is with the extra complexity of writing the optimization pass.

Proposal

Change the wording on the spec to require from implementations to return a slice of at least cap capacity when calling make([]T, len, cap), instead of precisely cap capacity.

Potential break

If some code relied on a make of a precise capacity for a following piece of code (likely append) to be forced to copy, like this:

s := make([]int, 10, 10)
sCopy = append(s, 4, 5, 6)
s = append(s, 1,2,3)

That breaks, previously sCopy[10:] = []int{4, 5, 6} and s[10:] = []int{1, 2, 3}.
After maybe that the second append overwrite the data from the first resulting in sCopy[10:] = []int{1, 2, 3} and s[10:] = []int{1, 2, 3}.

However I have never seen code like that, the patterns I've seen are either by creating a new slice and copying into it.
Or append(append(s[:0:0], s...), 4, 5, 6) which neither break because they take care of copying s away in all cases.

@gopherbot gopherbot added this to the Proposal milestone Apr 3, 2022
@Jorropo
Copy link
Member Author

Jorropo commented Apr 3, 2022

Update:

Some of what I've said previously is wrong.
Mainly the:

tempSlice := make([]int, 13, 13)
s := tempSlice[:10:10]
doStuff(s)
s = tempSlice
copy(s[len(s):], [...]{1,2,3})

And:

There are negligible performance implication between the two examples, the main issue I have is with the extra complexity of writing the optimization pass.

Are wrong.
The current wording of spec, allows you to assume that appending to a slice made by make must copy if the capacity isn't there.

So if the appended slice (s after s = tempSplice in the code I've quoted) overwrite indexes covered by the initial slice, this is also a breaking change and can't be compiled as is.

That has big repercussions on performance mainly now to do this optimization the compiler needs to prove that after appending the lower shared part of the slice isn't overwrote by either one, which can be broken trivially if any one of them escapes for example.
That also has repercussions on compilation performance, it replace what can be a simple dependency analysis with a complex escape analysis and proving that the shared part is safe.

@Jorropo Jorropo changed the title proposal: runtime: allows makeslice to return more capacity than asked for proposal: spec&runtime: allows makeslice to return more capacity than asked for Apr 3, 2022
@mpx
Copy link
Contributor

mpx commented Apr 3, 2022

FYI, there is related discussion on #24204.

@Jorropo
Copy link
Member Author

Jorropo commented Apr 3, 2022

FYI, there is related discussion on #24204.

@mpx
Yeah thx :) I'll scavenge the arguments and repost new findings there.
Closing this issue, clearly a duplicate.

@Jorropo Jorropo closed this as completed Apr 3, 2022
@golang golang locked and limited conversation to collaborators Apr 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants