Ser/Des fix, test suite, optimization

Summary:
* Fixes serializer buffer ownership.
* Adds an extensive test suite for Serializer/Deserializer.
* Optimizes away deserializer grow logic by resetting the buffer before deserialization.

Differential Revision: D67991602

fbshipit-source-id: eedf3bb048e43e03b946bb34d7f43c7ab8cb7825

thrift/lib/go/thrift/deserializer.go

@@ -20,35 +20,42 @@ import (
 	"github.com/facebook/fbthrift/thrift/lib/go/thrift/types"
 )
 
+// Default initial size for serializer/deserializer memory buffer.
+// The buffer grows automatically when needed during ser/des.
+// The initial bufer size is meant to fit most messages, in order to
+// completely avoid buffer growth/reallocation and improve ser/des
+// performance for the most common usecase (a.k.a. a <1KB message).
+const defaultMemoryBufferSize = 1024 // 1KB
+
 type Deserializer struct {
 	Transport *MemoryBuffer
 	Protocol  types.Decoder
 }
 
 // NewBinaryDeserializer creates a new deserializer using the binary protocol
 func NewBinaryDeserializer() *Deserializer {
-	transport := NewMemoryBufferLen(1024)
+	transport := NewMemoryBufferLen(defaultMemoryBufferSize)
 	protocol := NewBinaryFormat(transport)
 	return &Deserializer{Transport: transport, Protocol: protocol}
 }
 
 // NewCompactDeserializer creates a new deserializer using the compact protocol
 func NewCompactDeserializer() *Deserializer {
-	transport := NewMemoryBufferLen(1024)
+	transport := NewMemoryBufferLen(defaultMemoryBufferSize)
 	protocol := NewCompactFormat(transport)
 	return &Deserializer{Transport: transport, Protocol: protocol}
 }
 
 // NewCompactJSONDeserializer creates a new deserializer using the JSON protocol
 func NewCompactJSONDeserializer() *Deserializer {
-	transport := NewMemoryBufferLen(1024)
+	transport := NewMemoryBufferLen(defaultMemoryBufferSize)
 	protocol := NewCompactJSONFormat(transport)
 	return &Deserializer{Transport: transport, Protocol: protocol}
 }
 
 // NewSimpleJSONDeserializer creates a new deserializer using the simple JSON protocol
 func NewSimpleJSONDeserializer() *Deserializer {
-	transport := NewMemoryBufferLen(1024)
+	transport := NewMemoryBufferLen(defaultMemoryBufferSize)
 	protocol := NewSimpleJSONFormat(transport)
 	return &Deserializer{Transport: transport, Protocol: protocol}
 }
@@ -65,17 +72,14 @@ func DecodeBinary(data []byte, msg types.Struct) error {
 
 // ReadString deserializes a Thrift struct from a string
 func (t *Deserializer) ReadString(msg types.Struct, s string) error {
-	if _, err := t.Transport.Write([]byte(s)); err != nil {
-		return err
-	}
-	if err := msg.Read(t.Protocol); err != nil {
-		return err
-	}
-	return nil
+	return t.Read(msg, []byte(s))
 }
 
 // Read deserializes a Thrift struct from a byte slice
 func (t *Deserializer) Read(msg types.Struct, b []byte) error {
+	// Reset the internal buffer (while keeping the underlying storage)
+	t.Transport.Reset()
+
 	if _, err := t.Transport.Write(b); err != nil {
 		return err
 	}

thrift/lib/go/thrift/deserializer_test.go

@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package thrift
+
+import (
+	"crypto/rand"
+	"fmt"
+	"reflect"
+	"testing"
+
+	"github.com/facebook/fbthrift/thrift/lib/go/thrift/dummy"
+)
+
+func TestGiantStructDeserialization(t *testing.T) {
+	// Deserializer should be able to deserialize a struct
+	// much larger than the default size memory buffer.
+
+	des := NewBinaryDeserializer()
+
+	giantByteBlob := make([]byte, defaultMemoryBufferSize*10)
+	_, err := rand.Read(giantByteBlob)
+	if err != nil {
+		t.Fatalf("failed to rand read: %v", err)
+	}
+	val := dummy.NewDummyStruct1().
+		SetField8(giantByteBlob).
+		SetField9("giant_struct")
+	pay, err := EncodeBinary(val)
+	if err != nil {
+		t.Fatalf("failed to encode: %v", err)
+	}
+
+	var res dummy.DummyStruct1
+	err = des.Read(&res, pay)
+	if err != nil {
+		t.Fatalf("failed to read: %v", err)
+	}
+	if !reflect.DeepEqual(*val, res) {
+		t.Fatalf("values are not equal: %+v != %+v", *val, res)
+	}
+}
+
+func TestConsequentDeserialization(t *testing.T) {
+	// A single Deserializer instance should be able to
+	// perform multiple sequential deserializations.
+
+	des := NewBinaryDeserializer()
+
+	for i := range 1000 {
+		val := dummy.NewDummyStruct1().
+			SetField9(fmt.Sprintf("sequential_test_value_%d", i))
+		pay, err := EncodeBinary(val)
+		if err != nil {
+			t.Fatalf("failed to encode: %v", err)
+		}
+
+		var res dummy.DummyStruct1
+		err = des.Read(&res, pay)
+		if err != nil {
+			t.Fatalf("failed to read: %v", err)
+		}
+		if !reflect.DeepEqual(*val, res) {
+			t.Fatalf("values are not equal: %+v != %+v", *val, res)
+		}
+	}
+
+	// The internal memory buffer should not have grown at all,
+	// since we were deserializing small structs.
+	if des.Transport.Cap() != defaultMemoryBufferSize {
+		t.Fatalf("deserializer memory buffer grew: %d", des.Transport.Cap())
+	}
+}