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

Slice: use reflect.Append instead of pre-allocating whole slice. #7

Merged
merged 15 commits into from Aug 28, 2022
Merged
18 changes: 9 additions & 9 deletions .github/workflows/tests.yml
Expand Up @@ -4,15 +4,15 @@ jobs:
test:
strategy:
matrix:
go-version: [1.16.x, 1.17.x, 1.18.x]
go-version: [1.16.x, 1.17.x, 1.18.x, 1.19.x]
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Install Go
uses: actions/setup-go@v2
with:
go-version: ${{ matrix.go-version }}
- name: Checkout code
uses: actions/checkout@v2
- name: Test
run: go test ./... -count=100
- name: Install Go
uses: actions/setup-go@v2
with:
go-version: ${{ matrix.go-version }}
- name: Checkout code
uses: actions/checkout@v2
- name: Test
run: go test ./... -count=100
119 changes: 111 additions & 8 deletions borsh_test.go
Expand Up @@ -1076,13 +1076,6 @@ type StructWithOptionalFields struct {
Hello string
}

func concatByteSlices(slices ...[]byte) (out []byte) {
for i := range slices {
out = append(out, slices[i]...)
}
return
}

type Struct struct {
Foo string
Bar uint32
Expand Down Expand Up @@ -1486,8 +1479,9 @@ type S struct {
}

func TestSet(t *testing.T) {
emptyStruct := struct{}{}
x := S{
S: map[int64]struct{}{124: struct{}{}, 214: struct{}{}, 24: struct{}{}, 53: struct{}{}},
S: map[int64]struct{}{124: emptyStruct, 214: emptyStruct, 24: emptyStruct, 53: emptyStruct},
}
data, err := MarshalBorsh(x)
require.NoError(t, err)
Expand Down Expand Up @@ -1653,3 +1647,112 @@ func TestCustomType(t *testing.T) {

require.Equal(t, x, *y)
}

func TestStringSlice(t *testing.T) {
{
// slice:
x := []string{"a", "b", "c"}
data, err := MarshalBorsh(x)
require.NoError(t, err)

require.Equal(t, concatByteSlices(
[]byte{0x3, 0x0, 0x0, 0x0}, // length

[]byte{0x1, 0x0, 0x0, 0x0}, // length of first string
[]byte("a"),

[]byte{0x1, 0x0, 0x0, 0x0}, // length of second string
[]byte("b"),

[]byte{0x1, 0x0, 0x0, 0x0}, // length of third string
[]byte("c"),
), data)

y := new([]string)
err = UnmarshalBorsh(y, data)
require.NoError(t, err)

require.Equal(t, x, *y)
}
{
// string slice as field:
type S struct {
A []string
}
x := S{
A: []string{"a", "b", "c"},
}
data, err := MarshalBorsh(x)
require.NoError(t, err)

require.Equal(t, concatByteSlices(
[]byte{0x3, 0x0, 0x0, 0x0}, // length of A

[]byte{0x1, 0x0, 0x0, 0x0}, // length of A[0]
[]byte("a"),

[]byte{0x1, 0x0, 0x0, 0x0}, // length of A[1]
[]byte("b"),

[]byte{0x1, 0x0, 0x0, 0x0}, // length of A[2]
[]byte("c"),
), data)

y := new(S)
err = UnmarshalBorsh(y, data)
require.NoError(t, err)

require.Equal(t, x, *y)
}
{
// string slice as optional field (present):
type S struct {
A *[]string `bin:"optional"`
}
slice := []string{"a", "b", "c"}
x := S{
A: &slice,
}
data, err := MarshalBorsh(x)
require.NoError(t, err)

require.Equal(t, concatByteSlices(
[]byte{0x01}, // optionality
[]byte{0x3, 0x0, 0x0, 0x0}, // slice length

[]byte{0x1, 0x0, 0x0, 0x0}, // slice item length (string)
[]byte("a"),

[]byte{0x1, 0x0, 0x0, 0x0}, // slice item length (string)
[]byte("b"),

[]byte{0x1, 0x0, 0x0, 0x0}, // slice item length (string)
[]byte("c"),
), data)

y := new(S)
err = UnmarshalBorsh(y, data)
require.NoError(t, err)

require.Equal(t, x, *y)
}
{
// string slice as optional field (absent):
type S struct {
A *[]string `bin:"optional"`
}
x := S{}
data, err := MarshalBorsh(x)
require.NoError(t, err)

require.Equal(t, concatByteSlices(
[]byte{0x0}, // optionality
), data)

y := new(S)
err = UnmarshalBorsh(y, data)
require.NoError(t, err)

require.Equal(t, x, *y)
}
}
106 changes: 105 additions & 1 deletion decoder.go
Expand Up @@ -276,6 +276,12 @@ type peekAbleByteReader interface {
}

func readNBytes(n int, reader peekAbleByteReader) ([]byte, error) {
if n == 0 {
return make([]byte, 0), nil
}
if n < 0 || n > 0x7FFF_FFFF {
return nil, fmt.Errorf("invalid length n: %v", n)
}
buf := make([]byte, n)
for i := 0; i < n; i++ {
b, err := reader.ReadByte()
Expand All @@ -284,14 +290,27 @@ func readNBytes(n int, reader peekAbleByteReader) ([]byte, error) {
}
buf[i] = b
}

return buf, nil
}

func discardNBytes(n int, reader *Decoder) error {
if n == 0 {
return nil
}
if n < 0 || n > 0x7FFF_FFFF {
return fmt.Errorf("invalid length n: %v", n)
}
return reader.SkipBytes(uint(n))
}

func (dec *Decoder) ReadNBytes(n int) (out []byte, err error) {
return readNBytes(n, dec)
}

func (dec *Decoder) Discard(n int) (err error) {
return discardNBytes(n, dec)
}

func (dec *Decoder) ReadTypeID() (out TypeID, err error) {
discriminator, err := dec.ReadNBytes(8)
if err != nil {
Expand Down Expand Up @@ -551,6 +570,9 @@ func (dec *Decoder) ReadRustString() (out string, err error) {
if err != nil {
return "", err
}
if length > 0x7FFF_FFFF {
return "", io.ErrUnexpectedEOF
}
bytes, err := dec.ReadNBytes(int(length))
if err != nil {
return "", err
Expand Down Expand Up @@ -673,3 +695,85 @@ func indirect(v reflect.Value, decodingNull bool) (BinaryUnmarshaler, reflect.Va
}
return nil, v
}

func reflect_readArrayOfBytes(append bool, d *Decoder, l int, rv reflect.Value) error {
for i := 0; i < l; i++ {
n, err := d.ReadByte()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can save a lot of bounds checking here by

  • doing a bounds check for the entire array
  • allocating the entire slice
  • then filling it

Same for all the other primitive arrays and slices

reflect.Append is worse than pre-allocation when the slice size is known because the end result is the same (in terms of memory usage), but the former is less efficient

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more optimization ideas:

  • On little endian machines without alignment requirements, no conversion is necessary for integer primitive types like uint16`. We could simply unsafely copy into the backing memory buffer of a slice.
  • In all simple copy cases, allocating a slice isn't even necessary – The decoder could unsafely construct a slice that points to a place in the serialized buffer

if err != nil {
return err
}
if append {
rv.Set(reflect.Append(rv, reflect.ValueOf(n)))
} else {
rv.Index(i).SetUint(uint64(n))
}
}
return nil
}

func reflect_readArrayOfUint16(append bool, d *Decoder, l int, rv reflect.Value, order binary.ByteOrder) error {
for i := 0; i < l; i++ {
n, err := d.ReadUint16(order)
if err != nil {
return err
}
if append {
rv.Set(reflect.Append(rv, reflect.ValueOf(n)))
} else {
rv.Index(i).SetUint(uint64(n))
}
}
return nil
}

func reflect_readArrayOfUint32(append bool, d *Decoder, l int, rv reflect.Value, order binary.ByteOrder) error {
for i := 0; i < l; i++ {
n, err := d.ReadUint32(order)
if err != nil {
return err
}
if append {
rv.Set(reflect.Append(rv, reflect.ValueOf(n)))
} else {
rv.Index(i).SetUint(uint64(n))
}
}
return nil
}

func reflect_readArrayOfUint64(append bool, d *Decoder, l int, rv reflect.Value, order binary.ByteOrder) error {
for i := 0; i < l; i++ {
n, err := d.ReadUint64(order)
if err != nil {
return err
}
if append {
rv.Set(reflect.Append(rv, reflect.ValueOf(n)))
} else {
rv.Index(i).SetUint(n)
}
}
return nil
}

// reflect_readArrayOfUint_ is used for reading arrays/slices of uints of any size.
func reflect_readArrayOfUint_(append bool, d *Decoder, l int, k reflect.Kind, rv reflect.Value, order binary.ByteOrder) error {
switch k {
// case reflect.Uint:
// // switch on system architecture (32 or 64 bit)
// if unsafe.Sizeof(uintptr(0)) == 4 {
// return reflect_readArrayOfUint32(append, d, l, rv, order)
// }
// return reflect_readArrayOfUint64(append, d, l, rv, order)
case reflect.Uint8:
return reflect_readArrayOfBytes(append, d, l, rv)
case reflect.Uint16:
return reflect_readArrayOfUint16(append, d, l, rv, order)
case reflect.Uint32:
return reflect_readArrayOfUint32(append, d, l, rv, order)
case reflect.Uint64:
return reflect_readArrayOfUint64(append, d, l, rv, order)
default:
return fmt.Errorf("unsupported kind: %v", k)
}
}
31 changes: 31 additions & 0 deletions decoder_bench_test.go
@@ -0,0 +1,31 @@
package bin

import (
"testing"
)

func BenchmarkDecodeUintSlice(b *testing.B) {
buf := concatByteSlices(
// length:
uint32ToBytes(8, LE),
// data:
uint32ToBytes(0, LE),
uint32ToBytes(1, LE),
uint32ToBytes(2, LE),
uint32ToBytes(3, LE),
uint32ToBytes(4, LE),
uint32ToBytes(5, LE),
uint32ToBytes(6, LE),
uint32ToBytes(7, LE),
)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
got := make([]uint32, 0)
decoder := NewBorshDecoder(buf)
err := decoder.Decode(&got)
if err != nil {
b.Error(err)
}
}
}
38 changes: 29 additions & 9 deletions decoder_bin.go
Expand Up @@ -155,13 +155,21 @@ func (dec *Decoder) decodeBin(rv reflect.Value, opt *option) (err error) {
}
switch rt.Kind() {
case reflect.Array:
length := rt.Len()
l := rt.Len()
if traceEnabled {
zlog.Debug("decoding: reading array", zap.Int("length", length))
zlog.Debug("decoding: reading array", zap.Int("length", l))
}
for i := 0; i < length; i++ {
if err = dec.decodeBin(rv.Index(i), nil); err != nil {
return

switch k := rv.Type().Elem().Kind(); k {
case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
if err := reflect_readArrayOfUint_(false, dec, l, k, rv, LE); err != nil {
return err
}
default:
for i := 0; i < l; i++ {
if err = dec.decodeBin(rv.Index(i), nil); err != nil {
return
}
}
}
return
Expand All @@ -185,10 +193,22 @@ func (dec *Decoder) decodeBin(rv reflect.Value, opt *option) (err error) {
return io.ErrUnexpectedEOF
}

rv.Set(reflect.MakeSlice(rt, l, l))
for i := 0; i < l; i++ {
if err = dec.decodeBin(rv.Index(i), nil); err != nil {
return
rv.Set(reflect.MakeSlice(rt, 0, 0))
switch k := rv.Type().Elem().Kind(); k {
case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
if err := reflect_readArrayOfUint_(true, dec, l, k, rv, LE); err != nil {
return err
}
default:
for i := 0; i < l; i++ {
// create new element of type rt:
element := reflect.New(rt.Elem())
// decode into element:
if err = dec.decodeBin(element, nil); err != nil {
return
}
// append to slice:
rv.Set(reflect.Append(rv, element.Elem()))
}
}

Expand Down