From 417a26d20c23ea0aaa8f95325bb30172b3e3e9e9 Mon Sep 17 00:00:00 2001 From: "L. Pereira" Date: Wed, 31 Jul 2024 13:20:12 -0700 Subject: [PATCH] runtime: Simplify slice growing/appending code (#4287) * reflect: rawFieldByNameFunc: copy index slice to avoid later overwrites * runtime: Simplify slice growing/appending code Refactor the slice appending function to rely on the slice growing function, and remove branches/loops to use a branchfree variant. Signed-off-by: L. Pereira * runtime: Remove one branch in sliceAppend() Both branches were equivalent, so guard the overall logic in sliceAppend() with the more general condition. Signed-off-by: L. Pereira * runtime: Simplify slice growing calculation Use `bits.Len()` rather than `32 - bits.LeadingZeros32()`. They're equivalent, but the Len version is a bit easier to read. Signed-off-by: L. Pereira * reflect: Always call sliceGrow() in extendSlice() sliceGrow() will return the old slice if its capacity is large enough. Signed-off-by: L. Pereira --------- Signed-off-by: L. Pereira Co-authored-by: Damian Gryski --- src/reflect/type.go | 4 +-- src/reflect/value.go | 13 +--------- src/runtime/slice.go | 61 +++++++++++++------------------------------- testdata/slice.txt | 4 +-- 4 files changed, 23 insertions(+), 59 deletions(-) diff --git a/src/reflect/type.go b/src/reflect/type.go index 1ac509cbb..1a0d4fc85 100644 --- a/src/reflect/type.go +++ b/src/reflect/type.go @@ -774,7 +774,7 @@ func (t *rawType) rawFieldByNameFunc(match func(string) bool) (rawStructField, [ if match(name) { found = append(found, result{ rawStructFieldFromPointer(descriptor, field.fieldType, data, flagsByte, name, offset), - append(ll.index, int(i)), + append(ll.index[:len(ll.index):len(ll.index)], int(i)), }) } @@ -787,7 +787,7 @@ func (t *rawType) rawFieldByNameFunc(match func(string) bool) (rawStructField, [ nextlevel = append(nextlevel, fieldWalker{ t: embedded, - index: append(ll.index, int(i)), + index: append(ll.index[:len(ll.index):len(ll.index)], int(i)), }) } diff --git a/src/reflect/value.go b/src/reflect/value.go index c8439ccef..56c0dc10d 100644 --- a/src/reflect/value.go +++ b/src/reflect/value.go @@ -1712,18 +1712,7 @@ func extendSlice(v Value, n int) sliceHeader { old = *(*sliceHeader)(v.value) } - var nbuf unsafe.Pointer - var nlen, ncap uintptr - - if old.len+uintptr(n) > old.cap { - // we need to grow the slice - nbuf, nlen, ncap = sliceGrow(old.data, old.len, old.cap, old.cap+uintptr(n), v.typecode.elem().Size()) - } else { - // we can reuse the slice we have - nbuf = old.data - nlen = old.len - ncap = old.cap - } + nbuf, nlen, ncap := sliceGrow(old.data, old.len, old.cap, old.len+uintptr(n), v.typecode.elem().Size()) return sliceHeader{ data: nbuf, diff --git a/src/runtime/slice.go b/src/runtime/slice.go index 2269047a8..7d804b11b 100644 --- a/src/runtime/slice.go +++ b/src/runtime/slice.go @@ -3,42 +3,24 @@ package runtime // This file implements compiler builtins for slices: append() and copy(). import ( + "math/bits" "unsafe" ) // Builtin append(src, elements...) function: append elements to src and return // the modified (possibly expanded) slice. -func sliceAppend(srcBuf, elemsBuf unsafe.Pointer, srcLen, srcCap, elemsLen uintptr, elemSize uintptr) (unsafe.Pointer, uintptr, uintptr) { - if elemsLen == 0 { - // Nothing to append, return the input slice. - return srcBuf, srcLen, srcCap +func sliceAppend(srcBuf, elemsBuf unsafe.Pointer, srcLen, srcCap, elemsLen, elemSize uintptr) (unsafe.Pointer, uintptr, uintptr) { + newLen := srcLen + elemsLen + if elemsLen > 0 { + // Allocate a new slice with capacity for elemsLen more elements, if necessary; + // otherwise, reuse the passed slice. + srcBuf, _, srcCap = sliceGrow(srcBuf, srcLen, srcCap, newLen, elemSize) + + // Append the new elements in-place. + memmove(unsafe.Add(srcBuf, srcLen*elemSize), elemsBuf, elemsLen*elemSize) } - if srcLen+elemsLen > srcCap { - // Slice does not fit, allocate a new buffer that's large enough. - srcCap = srcCap * 2 - if srcCap == 0 { // e.g. zero slice - srcCap = 1 - } - for srcLen+elemsLen > srcCap { - // This algorithm may be made more memory-efficient: don't multiply - // by two but by 1.5 or something. As far as I can see, that's - // allowed by the Go language specification (but may be observed by - // programs). - srcCap *= 2 - } - buf := alloc(srcCap*elemSize, nil) - - // Copy the old slice to the new slice. - if srcLen != 0 { - memmove(buf, srcBuf, srcLen*elemSize) - } - srcBuf = buf - } - - // The slice fits (after possibly allocating a new one), append it in-place. - memmove(unsafe.Add(srcBuf, srcLen*elemSize), elemsBuf, elemsLen*elemSize) - return srcBuf, srcLen + elemsLen, srcCap + return srcBuf, newLen, srcCap } // Builtin copy(dst, src) function: copy bytes from dst to src. @@ -54,29 +36,22 @@ func sliceCopy(dst, src unsafe.Pointer, dstLen, srcLen uintptr, elemSize uintptr // sliceGrow returns a new slice with space for at least newCap elements func sliceGrow(oldBuf unsafe.Pointer, oldLen, oldCap, newCap, elemSize uintptr) (unsafe.Pointer, uintptr, uintptr) { - - // TODO(dgryski): sliceGrow() and sliceAppend() should be refactored to share the base growth code. - if oldCap >= newCap { // No need to grow, return the input slice. return oldBuf, oldLen, oldCap } - // allow nil slice - if oldCap == 0 { - oldCap++ - } + // This can be made more memory-efficient by multiplying by some other constant, such as 1.5, + // which seems to be allowed by the Go language specification (but this can be observed by + // programs); however, due to memory fragmentation and the current state of the TinyGo + // memory allocators, this causes some difficult to debug issues. + newCap = 1 << bits.Len(uint(newCap)) - // grow capacity - for oldCap < newCap { - oldCap *= 2 - } - - buf := alloc(oldCap*elemSize, nil) + buf := alloc(newCap*elemSize, nil) if oldLen > 0 { // copy any data to new slice memmove(buf, oldBuf, oldLen*elemSize) } - return buf, oldLen, oldCap + return buf, oldLen, newCap } diff --git a/testdata/slice.txt b/testdata/slice.txt index d16a0bda9..488170b52 100644 --- a/testdata/slice.txt +++ b/testdata/slice.txt @@ -7,12 +7,12 @@ copy foo -> bar: 3 bar: len=3 cap=5 data: 1 2 4 slice is nil? true true grow: len=0 cap=0 data: -grow: len=1 cap=1 data: 42 +grow: len=1 cap=2 data: 42 grow: len=3 cap=4 data: 42 -1 -2 grow: len=7 cap=8 data: 42 -1 -2 1 2 4 5 grow: len=7 cap=8 data: 42 -1 -2 1 2 4 5 grow: len=14 cap=16 data: 42 -1 -2 1 2 4 5 42 -1 -2 1 2 4 5 -bytes: len=6 cap=6 data: 1 2 3 102 111 111 +bytes: len=6 cap=8 data: 1 2 3 102 111 111 slice to array pointer: 1 -2 20 4 unsafe.Add array: 1 5 8 4 unsafe.Slice array: 3 3 9 15 4