Skip to content

Commit

Permalink
chore: even more various fixes and small refactorings
Browse files Browse the repository at this point in the history
- Use reflect.Value.Addr().UnsafePointer() instead reflect.Value.UnsafeAddr() for safety reasons.
- Some additional variable renames.
- Properly handle Go array unmarshal.
- Improve slice unmarshalling performance.
- Re-run kres.
- Fix minor linter complains.

Signed-off-by: Dmitriy Matrenichev <dmitry.matrenichev@siderolabs.com>
  • Loading branch information
DmitriyMV committed Aug 2, 2022
1 parent 76e5695 commit 94427a5
Show file tree
Hide file tree
Showing 9 changed files with 121 additions and 26 deletions.
3 changes: 2 additions & 1 deletion .dockerignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# THIS FILE WAS AUTOMATICALLY GENERATED, PLEASE DO NOT EDIT.
#
# Generated on 2022-07-26T15:06:36Z by kres latest.
# Generated on 2022-08-02T12:15:47Z by kres latest.

**
!messages
Expand All @@ -15,6 +15,7 @@
!marshal.go
!marshal_test.go
!person_test.go
!pointer.go
!predefined_types.go
!protobuf_test.go
!type_cache.go
Expand Down
3 changes: 2 additions & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

# THIS FILE WAS AUTOMATICALLY GENERATED, PLEASE DO NOT EDIT.
#
# Generated on 2022-07-26T15:20:24Z by kres latest.
# Generated on 2022-08-02T12:15:47Z by kres latest.

ARG TOOLCHAIN

Expand Down Expand Up @@ -59,6 +59,7 @@ COPY ./map_test.go ./map_test.go
COPY ./marshal.go ./marshal.go
COPY ./marshal_test.go ./marshal_test.go
COPY ./person_test.go ./person_test.go
COPY ./pointer.go ./pointer.go
COPY ./predefined_types.go ./predefined_types.go
COPY ./protobuf_test.go ./protobuf_test.go
COPY ./type_cache.go ./type_cache.go
Expand Down
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# THIS FILE WAS AUTOMATICALLY GENERATED, PLEASE DO NOT EDIT.
#
# Generated on 2022-07-26T15:06:36Z by kres latest.
# Generated on 2022-08-02T12:15:47Z by kres latest.

# common variables

Expand All @@ -11,7 +11,7 @@ ARTIFACTS := _out
REGISTRY ?= ghcr.io
USERNAME ?= siderolabs
REGISTRY_AND_USERNAME ?= $(REGISTRY)/$(USERNAME)
GOLANGCILINT_VERSION ?= v1.47.2
GOLANGCILINT_VERSION ?= v1.47.3
GOFUMPT_VERSION ?= v0.3.1
GO_VERSION ?= 1.18
GOIMPORTS_VERSION ?= v0.1.11
Expand Down
38 changes: 28 additions & 10 deletions array_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,27 +272,45 @@ func testDisallowedTypes[T any](t *testing.T) {
func TestDuration(t *testing.T) {
t.Parallel()

arr := newArray(time.Second*11, time.Second*12, time.Second*13)
buf := must(protoenc.Marshal(arr))(t)
expected := newArray(time.Second*11, time.Second*12, time.Second*13)
buf := must(protoenc.Marshal(expected))(t)

t.Log(hex.Dump(buf))

var target array[time.Duration]
var actual array[time.Duration]

require.NoError(t, protoenc.Unmarshal(buf, &target))
assert.Equal(t, arr.Arr, target.Arr)
require.NoError(t, protoenc.Unmarshal(buf, &actual))
assert.Equal(t, expected.Arr, actual.Arr)
}

func TestTime(t *testing.T) {
t.Parallel()

arr := newArray(time.Unix(11, 0).UTC(), time.Unix(12, 0).UTC(), time.Unix(13, 0).UTC())
buf := must(protoenc.Marshal(arr))(t)
expected := newArray(time.Unix(11, 0).UTC(), time.Unix(12, 0).UTC(), time.Unix(13, 0).UTC())
buf := must(protoenc.Marshal(expected))(t)

t.Log(hex.Dump(buf))

var target array[time.Time]
var actual array[time.Time]

require.NoError(t, protoenc.Unmarshal(buf, &target))
assert.Equal(t, arr.Arr, target.Arr)
require.NoError(t, protoenc.Unmarshal(buf, &actual))
assert.Equal(t, expected.Arr, actual.Arr)
}

func TestSliceToArray(t *testing.T) {
t.Parallel()

expected := newArray(1, 2, 3, 4, 5, 6, 7, 8, 9, 100500)
buf := must(protoenc.Marshal(expected))(t)

t.Log(hex.Dump(buf))

type structWithArray struct {
Arr [10]int `protobuf:"1"`
}

var actual structWithArray

require.NoError(t, protoenc.Unmarshal(buf, &actual))
assert.Equal(t, expected.Arr, actual.Arr[:])
}
2 changes: 1 addition & 1 deletion field_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestEncodeNested(t *testing.T) {

actual := must(protoenc.StructFields(val.Type()))(t)
for _, f := range actual {
f.Field = reflect.StructField{}
f.Field = reflect.StructField{} //nolint:govet
}

expected := []*protoenc.FieldData{
Expand Down
6 changes: 6 additions & 0 deletions marshal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,18 +122,24 @@ func Test2dSlice(t *testing.T) {
t.Parallel()

t.Run("should fail on 2d int slice", func(t *testing.T) {
t.Parallel()

encoded := Slice[int]{Values: [][]int{{1, 2, 3}, {4, 5, 6}}}
_, err := protoenc.Marshal(&encoded)
require.Error(t, err)
})

t.Run("should fail on 2d uint16 slice", func(t *testing.T) {
t.Parallel()

encoded := Slice[uint16]{Values: [][]uint16{{1, 2, 3}, {4, 5, 6}}}
_, err := protoenc.Marshal(&encoded)
require.Error(t, err)
})

t.Run("should ok on 2d byte slice", func(t *testing.T) {
t.Parallel()

encoded := Slice[byte]{Values: [][]byte{{1, 2, 3}, {4, 5, 6}}}
buf := must(protoenc.Marshal(&encoded))(t)

Expand Down
7 changes: 3 additions & 4 deletions pointer.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,21 @@

package protoenc

//TODO: remove this once Go 1.19 lands

import (
"sync/atomic"
"unsafe"
)

// A Pointer is an atomic pointer of type *T. The zero value is a nil *T.
type Pointer[T any] struct {
// TODO: remove this once Go 1.19 lands.
v unsafe.Pointer
}

// Load atomically loads and returns the value stored in x.
func (x *Pointer[T]) Load() *T { return (*T)(atomic.LoadPointer(&x.v)) }

// CompareAndSwap executes the compare-and-swap operation for x.
func (x *Pointer[T]) CompareAndSwap(old, new *T) (swapped bool) {
return atomic.CompareAndSwapPointer(&x.v, unsafe.Pointer(old), unsafe.Pointer(new))
func (x *Pointer[T]) CompareAndSwap(old, newVal *T) (swapped bool) {
return atomic.CompareAndSwapPointer(&x.v, unsafe.Pointer(old), unsafe.Pointer(newVal))
}
3 changes: 1 addition & 2 deletions type_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"reflect"
"strconv"
"sync"
"unsafe"

"google.golang.org/protobuf/encoding/protowire"
)
Expand Down Expand Up @@ -207,7 +206,7 @@ func RegisterEncoderDecoder[T any, Enc func(T) ([]byte, error), Dec func([]byte)
return err
}

*(*T)(unsafe.Pointer(dst.UnsafeAddr())) = v
*(*T)(dst.Addr().UnsafePointer()) = v

return nil
}
Expand Down
81 changes: 76 additions & 5 deletions unmarshal.go
Original file line number Diff line number Diff line change
Expand Up @@ -451,10 +451,9 @@ func (u *unmarshaller) slice(dst reflect.Value, decodedBytes []byte) error {
return err
}

elem := reflect.New(elemType).Elem()

if wiretype < 0 { // Other unpacked repeated types
// Just unpack and append one value from decodedBytes.
elem := reflect.New(elemType).Elem()
if err := u.putInto(elem, protowire.BytesType, 0, decodedBytes); err != nil {
return err
}
Expand All @@ -464,15 +463,21 @@ func (u *unmarshaller) slice(dst reflect.Value, decodedBytes []byte) error {
return nil
}

sw := sequenceWrapper{
seq: dst,
}

defer sw.FixLen()

// Decode packed values from the buffer and append them to the dst.
for len(decodedBytes) > 0 {
rem, err := u.decodeValue(wiretype, decodedBytes, elem)
nextElem := sw.NextElem()

rem, err := u.decodeValue(wiretype, decodedBytes, nextElem)
if err != nil {
return err
}

dst.Set(reflect.Append(dst, elem))

decodedBytes = rem
}

Expand Down Expand Up @@ -575,3 +580,69 @@ func (u *unmarshaller) mapEntry(dstEntry reflect.Value, decodedBytes []byte) err

return nil
}

type sequenceWrapper struct {
seq reflect.Value
idx int
}

func (w *sequenceWrapper) NextElem() reflect.Value {
if w.seq.Kind() == reflect.Array {
result := w.seq.Index(w.idx)
w.idx++

return result
}

if sliceCap := w.seq.Cap(); w.idx == sliceCap {
w.seq.Set(grow(w.seq, 1))
}

result := w.seq.Index(w.idx)
w.idx++

return result
}

func (w *sequenceWrapper) FixLen() {
if w.seq.Kind() == reflect.Array {
return
}

w.seq.SetLen(w.idx)
}

// grow grows the slice s so that it can hold extra more values, allocating
// more capacity if needed. It also returns the new cap.
func grow(s reflect.Value, extra int) reflect.Value {
oldLen := s.Len()
newLen := oldLen + extra

if newLen < oldLen {
panic("reflect.Append: slice overflow")
}

targetCap := s.Cap()
if newLen <= targetCap {
return s.Slice(0, targetCap)
}

if targetCap == 0 {
targetCap = extra
} else {
const threshold = 256
for targetCap < newLen {
if oldLen < threshold {
targetCap += targetCap
} else {
targetCap += (targetCap + 3*threshold) / 4
}
}
}

t := reflect.MakeSlice(s.Type(), targetCap, targetCap)

reflect.Copy(t, s)

return t
}

0 comments on commit 94427a5

Please sign in to comment.