spec: set capacity of slice #1642
Labels
Milestone
Comments
You need a better example. Using the result of ReadSlice in bytes.NewBuffer is problematic even with the proposed language change, because a subsequent ReadSlice will invalidate the buffer. That is, the language change only avoids one of many subtle problems you might encounter due to oversharing of data. Labels changed: added languagechange. Owner changed to @griesemer. Status changed to Thinking. |
A solution propsed on gonuts group: https://groups.google.com/d/topic/golang-nuts/OxP76qjf104/discussion Creating a way to mark a slice as a cow (copy on write) slice, so if the user want's to secure that slice from being changed he can call a function that on the next write will make a copy of the slice. This could be done in the same function that checks from index bounds. |
That would make the bounds check dramatically more expensive. There is not a function that checks the bounds. It is just a few instructions, and instructions that can be eliminated when the compiler is smart enough. The copy-on-write could not be, and I'm not even sure the semantics are well-formed. This issue is about being able to write the cap field, nothing more. |
@2: An optionally COW slice is a somehow magic slice with an unpredictable performance. Behind the scenes it may get copied on first write access, right? I think this is exactly what Go tries to avoid wherever possible. Additionally I believe that the origin of the COW slice demand is rooted only in a poorly designed code, not in a real problem. |
I just got bit in a real-world server by this.
I have a slab allocator, and my slab code was giving out sub-regions of its 1MB []byte
to another part of the code.
After a fair bit of debugging, I found the slab code was fine but some other code was
slicing past its allowed []byte subregion.
In a nutshell:
slab := make([]byte, 6) // in reality, 1MB
item := slab[0:2] // I wish I could say slab[0:2:2] or something here
keyMem := item[2:4] // bug, which I wish would've blow up at runtime, with 4 being out of range.
key := []byte("some key")
n := copy(keyMem, key) // should've never made it this far, but this overwrote memory in another item.
This could've been worse if I weren't the author of both pieces of code. As a
programming-at-large defensive measure, it'd be nice if library authors could set the
caps on memory they give out, pushing blame & run-time explosions to the real buggy part.
|
Proposal just for []byte: https://golang.org/cl/7587045/ |
Some existing code relies on cap for checking if two slices share the same backing
array. Example from math/big/nat.go:
// alias returns true if x and y share the same base array.
func alias(x, y nat) bool {
return cap(x) > 0 && cap(y) > 0 && &x[0:cap(x)][cap(x)-1] == &y[0:cap(y)][cap(y)-1]
}
Although this approach may be depending on unspecified behavior (and thus the change
could be allowable as per go1compat.html), I think providing an alternative for such
checks should be considered if changing the cap without using unsafe is allowed.
|
This keeps coming up. Latest rejected CL, which would've sped things up in net/http, etc: https://golang.org/cl/8179043/ Has there been any new thinking on this bug? Tagging it Go1.2 so we can at least formally reject it later. Labels changed: added go1.2. |
The examples here are all cases where we allocate a large buffer, return a slice of that
buffer, and want to restrict the returned slice so that it can not be resliced to become
larger. We want to ensure that the user of the slice can not interfere with other
portions of the large buffer. A slice already prevents you from expanding toward the
start of the buffer, so this is the symmetric operation: prevent you from expanding
toward the end of the buffer.
An intermediate position rather than making a language change would be to add a function
reflect.SetCap.
func (v Value) SetCap(n int) {
v.mustBeAssignable()
v.mustBe(Slice)
s := (*SliceHeader)(v.val)
if n < 0 || n > s.Cap || n > s.Len {
panic("reflect: slice length out of range in SetLen")
}
s.Cap = n
}
|
I think this either needs to be a proper language change or no change. I find it "unseemly" (Java-like, actually) to have the library provide functionality to manipulate the internals of basic data structures. And if we change the language to permit this, we need also a mechanism to detect sharing of the underlying array (e.g., math/big relies on the alias function for performance). |
Ian, yes, that would be nice. Unfortunately compared to the nice performance improvement in https://golang.org/cl/8179043/ , using reflect.SetCap actually makes it worse: + if vv == nil && len(strs) > 0 { + // More than likely this will be a single-element key. + // Most headers aren't multi-valued. + vv, strs = strs[:1], strs[1:] + vv[0] = value + reflect.ValueOf(&vv).Elem().SetCap(1) + if cap(vv) != 1 { + panic("wrong cap") + } + m[key] = vv + } else { + m[key] = append(vv, value) + } ba12:textproto bradfitz$ ~/go/misc/benchcmp before after2 benchmark old ns/op new ns/op delta BenchmarkReadMIMEHeader 7765 8004 +3.08% benchmark old allocs new allocs delta BenchmarkReadMIMEHeader 23 23 0.00% benchmark old bytes new bytes delta BenchmarkReadMIMEHeader 1708 1669 -2.28% The reflect allocations exactly make up for the allocations I was trying to save, and CPU gets worse. Unless there's a cheaper way to use reflect. Robert, even if this were promoted to the language proper (and I still hope it does), we would _still_ need a reflect change, analogous to http://golang.org/pkg/reflect/#Value.SetLen I assume you mean unseemly to *only* have a library provide it. |
Marking as fixed. This is done. Fixed by https://golang.org/cl/10743046 Status changed to Fixed. |
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
The text was updated successfully, but these errors were encountered: