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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* reflect: Always call sliceGrow() in extendSlice()

sliceGrow() will return the old slice if its capacity is large enough.

Signed-off-by: L. Pereira <[email protected]>

---------

Signed-off-by: L. Pereira <[email protected]>
Co-authored-by: Damian Gryski <[email protected]>

Author: lpereira

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)),
 					})
 				}
 

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,

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++
-	}
-
-	// grow capacity
-	for oldCap < newCap {
-		oldCap *= 2
-	}
+	// 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))
 
-	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
 }

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