From e42bc852f86867440d6cf61ae351b2d66b24c272 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 2 Oct 2014 12:53:51 -0700 Subject: encoding/binary: fix error message In the process, simplified internal sizeOf and dataSize functions. Minor positive impact on performance. Added test case. benchmark old ns/op new ns/op delta BenchmarkReadSlice1000Int32s 14006 14122 +0.83% BenchmarkReadStruct 2508 2447 -2.43% BenchmarkReadInts 921 928 +0.76% BenchmarkWriteInts 2086 2081 -0.24% BenchmarkWriteSlice1000Int32s 13440 13497 +0.42% BenchmarkPutUvarint32 28.5 26.3 -7.72% BenchmarkPutUvarint64 81.3 76.7 -5.66% benchmark old MB/s new MB/s speedup BenchmarkReadSlice1000Int32s 285.58 283.24 0.99x BenchmarkReadStruct 27.90 28.60 1.03x BenchmarkReadInts 32.57 32.31 0.99x BenchmarkWriteInts 14.38 14.41 1.00x BenchmarkWriteSlice1000Int32s 297.60 296.36 1.00x BenchmarkPutUvarint32 140.55 151.92 1.08x BenchmarkPutUvarint64 98.36 104.33 1.06x Fixes issue 6818. LGTM=r R=r CC=golang-codereviews https://codereview.appspot.com/149290045 --- src/encoding/binary/binary.go | 70 +++++++++++++++++--------------------- src/encoding/binary/binary_test.go | 23 +++++++++++-- 2 files changed, 53 insertions(+), 40 deletions(-) (limited to 'src') diff --git a/src/encoding/binary/binary.go b/src/encoding/binary/binary.go index b5a377430..466bf97c9 100644 --- a/src/encoding/binary/binary.go +++ b/src/encoding/binary/binary.go @@ -200,18 +200,17 @@ func Read(r io.Reader, order ByteOrder, data interface{}) error { } // Fallback to reflect-based decoding. - var v reflect.Value - switch d := reflect.ValueOf(data); d.Kind() { + v := reflect.ValueOf(data) + size := -1 + switch v.Kind() { case reflect.Ptr: - v = d.Elem() + v = v.Elem() + size = dataSize(v) case reflect.Slice: - v = d - default: - return errors.New("binary.Read: invalid type " + d.Type().String()) + size = dataSize(v) } - size, err := dataSize(v) - if err != nil { - return errors.New("binary.Read: " + err.Error()) + if size < 0 { + return errors.New("binary.Read: invalid type " + reflect.TypeOf(data).String()) } d := &decoder{order: order, buf: make([]byte, size)} if _, err := io.ReadFull(r, d.buf); err != nil { @@ -324,68 +323,64 @@ func Write(w io.Writer, order ByteOrder, data interface{}) error { // Fallback to reflect-based encoding. v := reflect.Indirect(reflect.ValueOf(data)) - size, err := dataSize(v) - if err != nil { - return errors.New("binary.Write: " + err.Error()) + size := dataSize(v) + if size < 0 { + return errors.New("binary.Write: invalid type " + reflect.TypeOf(data).String()) } buf := make([]byte, size) e := &encoder{order: order, buf: buf} e.value(v) - _, err = w.Write(buf) + _, err := w.Write(buf) return err } // Size returns how many bytes Write would generate to encode the value v, which // must be a fixed-size value or a slice of fixed-size values, or a pointer to such data. +// If v is neither of these, Size returns -1. func Size(v interface{}) int { - n, err := dataSize(reflect.Indirect(reflect.ValueOf(v))) - if err != nil { - return -1 - } - return n + return dataSize(reflect.Indirect(reflect.ValueOf(v))) } // dataSize returns the number of bytes the actual data represented by v occupies in memory. // For compound structures, it sums the sizes of the elements. Thus, for instance, for a slice // it returns the length of the slice times the element size and does not count the memory -// occupied by the header. -func dataSize(v reflect.Value) (int, error) { +// occupied by the header. If the type of v is not acceptable, dataSize returns -1. +func dataSize(v reflect.Value) int { if v.Kind() == reflect.Slice { - elem, err := sizeof(v.Type().Elem()) - if err != nil { - return 0, err + if s := sizeof(v.Type().Elem()); s >= 0 { + return s * v.Len() } - return v.Len() * elem, nil + return -1 } return sizeof(v.Type()) } -func sizeof(t reflect.Type) (int, error) { +// sizeof returns the size >= 0 of variables for the given type or -1 if the type is not acceptable. +func sizeof(t reflect.Type) int { switch t.Kind() { case reflect.Array: - n, err := sizeof(t.Elem()) - if err != nil { - return 0, err + if s := sizeof(t.Elem()); s >= 0 { + return s * t.Len() } - return t.Len() * n, nil case reflect.Struct: sum := 0 for i, n := 0, t.NumField(); i < n; i++ { - s, err := sizeof(t.Field(i).Type) - if err != nil { - return 0, err + s := sizeof(t.Field(i).Type) + if s < 0 { + return -1 } sum += s } - return sum, nil + return sum case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128: - return int(t.Size()), nil + return int(t.Size()) } - return 0, errors.New("invalid type " + t.String()) + + return -1 } type coder struct { @@ -595,12 +590,11 @@ func (e *encoder) value(v reflect.Value) { } func (d *decoder) skip(v reflect.Value) { - n, _ := dataSize(v) - d.buf = d.buf[n:] + d.buf = d.buf[dataSize(v):] } func (e *encoder) skip(v reflect.Value) { - n, _ := dataSize(v) + n := dataSize(v) for i := range e.buf[0:n] { e.buf[i] = 0 } diff --git a/src/encoding/binary/binary_test.go b/src/encoding/binary/binary_test.go index c80c90383..8ee595fa4 100644 --- a/src/encoding/binary/binary_test.go +++ b/src/encoding/binary/binary_test.go @@ -289,6 +289,26 @@ func TestUnexportedRead(t *testing.T) { Read(&buf, LittleEndian, &u2) } +func TestReadErrorMsg(t *testing.T) { + var buf bytes.Buffer + read := func(data interface{}) { + err := Read(&buf, LittleEndian, data) + want := "binary.Read: invalid type " + reflect.TypeOf(data).String() + if err == nil { + t.Errorf("%T: got no error; want %q", data, want) + return + } + if got := err.Error(); got != want { + t.Errorf("%T: got %q; want %q", data, got, want) + } + } + read(0) + s := new(struct{}) + read(&s) + p := &s + read(&p) +} + type byteSliceReader struct { remain []byte } @@ -315,8 +335,7 @@ func BenchmarkReadStruct(b *testing.B) { bsr := &byteSliceReader{} var buf bytes.Buffer Write(&buf, BigEndian, &s) - n, _ := dataSize(reflect.ValueOf(s)) - b.SetBytes(int64(n)) + b.SetBytes(int64(dataSize(reflect.ValueOf(s)))) t := s b.ResetTimer() for i := 0; i < b.N; i++ { -- cgit v1.2.1