From 94427a5723dd6f37c2bfd55c63861c97b2de524b Mon Sep 17 00:00:00 2001 From: Dmitriy Matrenichev Date: Tue, 2 Aug 2022 13:44:09 +0300 Subject: [PATCH] chore: even more various fixes and small refactorings - 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 --- .dockerignore | 3 +- Dockerfile | 3 +- Makefile | 4 +-- array_test.go | 38 +++++++++++++++++------ field_test.go | 2 +- marshal_test.go | 6 ++++ pointer.go | 7 ++--- type_cache.go | 3 +- unmarshal.go | 81 ++++++++++++++++++++++++++++++++++++++++++++++--- 9 files changed, 121 insertions(+), 26 deletions(-) diff --git a/.dockerignore b/.dockerignore index 42e01e4..c66fae5 100644 --- a/.dockerignore +++ b/.dockerignore @@ -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 @@ -15,6 +15,7 @@ !marshal.go !marshal_test.go !person_test.go +!pointer.go !predefined_types.go !protobuf_test.go !type_cache.go diff --git a/Dockerfile b/Dockerfile index 506a45f..e5abaa0 100644 --- a/Dockerfile +++ b/Dockerfile @@ -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 @@ -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 diff --git a/Makefile b/Makefile index 35b4ee8..416864e 100644 --- a/Makefile +++ b/Makefile @@ -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 @@ -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 diff --git a/array_test.go b/array_test.go index fc6a66d..4cdbadb 100644 --- a/array_test.go +++ b/array_test.go @@ -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[:]) } diff --git a/field_test.go b/field_test.go index eb5401c..ce43b86 100644 --- a/field_test.go +++ b/field_test.go @@ -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{ diff --git a/marshal_test.go b/marshal_test.go index 459bd55..e3b223d 100644 --- a/marshal_test.go +++ b/marshal_test.go @@ -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) diff --git a/pointer.go b/pointer.go index 0076e04..2e15606 100644 --- a/pointer.go +++ b/pointer.go @@ -4,8 +4,6 @@ package protoenc -//TODO: remove this once Go 1.19 lands - import ( "sync/atomic" "unsafe" @@ -13,6 +11,7 @@ import ( // 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 } @@ -20,6 +19,6 @@ type Pointer[T any] struct { 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)) } diff --git a/type_cache.go b/type_cache.go index 00471cb..72b32bd 100644 --- a/type_cache.go +++ b/type_cache.go @@ -9,7 +9,6 @@ import ( "reflect" "strconv" "sync" - "unsafe" "google.golang.org/protobuf/encoding/protowire" ) @@ -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 } diff --git a/unmarshal.go b/unmarshal.go index d86c1e1..db4c001 100644 --- a/unmarshal.go +++ b/unmarshal.go @@ -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 } @@ -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 } @@ -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 +}